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
JUnit 5 extension #887
JUnit 5 extension #887
Changes from all commits
a057302
7936f90
fff8dd0
fbfac67
92e2026
5304cda
09719a9
6b7bad6
728a50b
3a939bf
18a16e2
8d01abe
475d349
88dc17a
68759c6
cd143e3
fdd6d0b
1b32a44
bb1a146
f3e0a5f
99417cf
e80512e
c2c416b
4f39e1b
f18010e
71307f0
c7c77c5
060d694
f323fd8
271a85b
abba31a
e3008bb
633197f
c876afa
80292b8
d1c5abe
e7f2839
a6f9c40
835c68e
1af5190
421c583
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.
This works for
TEST_INSTANCE
as constant key in case of parallel execution?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 be a singleton, right? Meaning a
store.get(TEST_INSTANCE)
should always return the same instance?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.
@marcphilipp / @sbrannen What's your stance on this in case of parallel execution?
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.
The
ExtensionContext
passed toTestInstancePostProcessors
is the one of the test class. Thus, unless I'm missing something, this should not work with parallel test execution.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.
@marcphilipp okay... Do you have a suggestion on how to implement this? The
testInstance
is stored to access outer instances in case of@Nested
test cases.I adopted this code from the MockitoExtension. So their code is broken too. We probably should notify them.
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.
What about
and
?
Not a working example, just as an idea for discussion.
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 don't see how this helps. In case of parallel execution the class names will be the same. So it's the same problem as with the constant, isn't it?
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.
Only if parallel execution happens inside the nested class.
I was assuming, parallel execution will be on outer class level.
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 think we need to wait for junit-team/junit5#1618
But I also want to dig into the junit code to be sure this does really not work in parallel mode.
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.
We can also merge and add a disclaimer, that parallel execution isn't supported ATM. Better than leaving this PR blocked for a longer time.
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.
Any reason to use field instead of getter?
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.
The
StoreAdapter
class is a private class inside ofTestcontainersExtension
. So all fields are visible fromTestcontainersExtension
and there is no gain from an information hiding perspective if we add getters.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.
The problem I see is that it is inconsistent with the rest of our code base because we always use getters :)
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 would return any field annotated with
@Container
and throw and error if it is notStartable
, otherwise it will simply ignore it