Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
TEST_CASE_FIXTURE: A new fixture macro for allowing persistent fixtures throughout a TEST_CASE #2885
Changes from 13 commits
a07b444
3386614
a4ddfa1
031cd55
390a39e
bbb7313
8c777bc
2680cb0
e83ff3f
9faa54e
c2984ee
faccd2d
a6e06b0
3c7920a
bf4575a
39a4b82
72ce2bd
cc25e46
f6fcfbb
d079e83
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 likein the fixture constructor and explain that
TEST_CASE_FIXTURE
takes 2 seconds, whileTEST_CASE_METHOD
needs 2/4/6/8/... seconds to execute.There was a problem hiding this comment.
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_FIXTURE
s have separate instances of fixture, i.e., the fixture is not static globally, just for the paths defined in oneTEST_CASE
.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.)There was a problem hiding this comment.
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