Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TEST_CASE_FIXTURE: A new fixture macro for allowing persistent fixtures throughout a TEST_CASE #2885

Merged
merged 20 commits into from
Aug 5, 2024

Conversation

KStocky
Copy link
Contributor

@KStocky KStocky commented Jul 7, 2024

Description

This PR introduces a new TEST_CASE macro called TEST_CASE_FIXTURE. TEST_CASE_FIXTURE offers the same functionality as TEST_CASE_METHOD except for one difference. The object on which the test method is invoked is only created once for all invocations of the test case. The object is created just after the testCaseStarting event is broadcast and the object is destroyed just before the testCaseEnding event is broadcast.

The main motivation for this new functionality is to allow TEST_CASEs to do expensive setup and teardown once per TEST_CASE, without having to resort to abusing event listeners or static function variables with manual initialization.

GitHub Issues

This PR hopes to be an implementation of #1602

docs/cmake-integration.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed this section and placed it in with the test-fixtures documentation since it is a macro for creating a test fixture.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find out what the naming scheme of examples is... Happy to rename this once I know what the name should be!

docs/test-fixtures.md Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jul 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.13%. Comparing base (33e24b1) to head (f6fcfbb).

Additional details and impacted files
@@            Coverage Diff             @@
##            devel    #2885      +/-   ##
==========================================
- Coverage   91.14%   91.13%   -0.01%     
==========================================
  Files         198      198              
  Lines        8487     8512      +25     
==========================================
+ Hits         7735     7757      +22     
- Misses        752      755       +3     

docs/test-fixtures.md Outdated Show resolved Hide resolved
}
}
```
This test case will be executed twice as there are two leaf sections. On the first run `val` will be `0` and on the second run `val` will be `1`. This is useful if you would like to share some expensive setup code with all runs of your test case which can't be done at static initialization time.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not particularly happy about the example/explanation combo. My first reaction was "well, how's that different from using static variable here".

I can't give you better version right now, but I think this should be focused on the performance difference from TEST_CASE_METHOD, maybe have something like

// Pretend doing hard work
std::this_thread::sleep_for(2s);

in the fixture constructor and explain that TEST_CASE_FIXTURE takes 2 seconds, while TEST_CASE_METHOD needs 2/4/6/8/... seconds to execute.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably also explain that two different TEST_CASE_FIXTUREs have separate instances of fixture, i.e., the fixture is not static globally, just for the paths defined in one TEST_CASE.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have redone the example and the documentation for this feature now. I feel like it now better highlights why a test writer would use this over TEST_CASE_METHOD.

@horenmar
Copy link
Member

As an FYI, I am (rather obviously) not finished with the review, but my bus ride is over and I'll have to return to this later.

docs/test-fixtures.md Outdated Show resolved Hide resolved
@@ -112,6 +112,14 @@ namespace Catch {
TestCaseHandle(TestCaseInfo* info, ITestInvoker* invoker) :
m_info(info), m_invoker(invoker) {}

void testCaseStarting() const {
Copy link
Member

@horenmar horenmar Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big fan of this name. It is consistent with the event names, but it does not describe the reason why it exists well. How about "prepareTestCase" instead?

(And same for testCaseEnding below.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup as below, I have renamed these prepareTestCase and tearDownTestCase

Copy link
Member

@horenmar horenmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I am convinced that the functionality is useful, but I want some changes before merging.

Fix the license on the file
Improve the example to make the use cases much clearer. Also fix up the documentation
…ASE_PERSISTENT_FIXTURE. Also adding the example to the list of examples
@horenmar horenmar merged commit f7cd0ba into catchorg:devel Aug 5, 2024
78 of 79 checks passed
@horenmar
Copy link
Member

horenmar commented Aug 5, 2024

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants