fix: aligns lifecycle calls in JUnit 5 with JUnit 4 behavior #310
Conversation
|
@bartoszmajsak Can you review this please? |
|
Thanks @lprimak! Merging #301 was giving me a bit of mixed feeling seeing this without any tests, but here we are :) I wonder though - do you think we could add some of the reproducers you created (and maybe more tests as we evolve) to be part of repo/build? This way we can avoid such challenges going forward. |
| invocation.skip(); | ||
| } else { | ||
| invocation.proceed(); |
There was a problem hiding this comment.
Why are we reversing the logic here? We don't want to have @BeforeAll executed as Arquillian test?
There was a problem hiding this comment.
Seems to me we are going backwards to the same issue we had two years back - it depends. It makes sense for both use cases, sometimes.
I think we need another annotation for this and decide what should be the default - run on the client side or on the server side? And warn before undefined order if both variants combined (multiple BeforeEach annotated methods). Maybe if RunAsClient is used, default should be just the remote side, if not, the opposite. But even then it makes sense in the other way.
Or am I wrong?
There was a problem hiding this comment.
Not the same case David :). This is the simplest case
There was a problem hiding this comment.
@bartoszmajsak the logic is reversed to accomplish the fix for the issue. These methods have to be only executed on the client and not in container as they are executed that way in JUnit 4
I’ll try to add the test for this as well
There was a problem hiding this comment.
I have added the tests. However, they are excluded for now. This requires integration test setup with app server as far as I am aware, and it's not done in Arquillian just yet, so I am not sure how to introduce that kind of tests.
Saying that, it's there and works when enabled :)
|
@bartoszmajsak added tests |
Well, the change was so simple :) However, with this PR, status quo is maintained and nothing is broken, so there's that... |
junit5/core/pom.xml
Outdated
| <!-- This is commented out because it is not set up to run | ||
| with current Arquillian testing environment--> | ||
| <!-- JUnit 5 Container / Payara support --> | ||
| <!-- <dependency> | ||
| <groupId>org.jboss.arquillian.junit5</groupId> | ||
| <artifactId>arquillian-junit5-container</artifactId> | ||
| <version>${project.version}</version> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>fish.payara.arquillian</groupId> | ||
| <artifactId>arquillian-payara-server-remote</artifactId> | ||
| <version>2.3.3</version> | ||
| <scope>test</scope> | ||
| </dependency>--> | ||
|
|
There was a problem hiding this comment.
If not needed then let's remove it :)
There was a problem hiding this comment.
I will put the new tests into another PR so you can decide what to do with them, and remove from this PR.
@kifj has ported tests so now this code is tested - thank you!
Above can move this PR forward
junit5/core/src/test/java/org/jboss/arquillian/junit5/lifecycle/CreateFileTestCase.java
Outdated
Show resolved
Hide resolved
lprimak
force-pushed
the
FixLifecycleCallbacks
branch
from
07b050a to
c50018a
Compare
- @BeforeAll / @afterall callbacks are always called on the client, consistent with JUnit 4 behavior - restored calls to @beforeeach / @AfterEach in client mode, were only being called on the server
lprimak
force-pushed
the
FixLifecycleCallbacks
branch
from
c50018a to
46d4c9e
Compare
|
@bartoszmajsak ready to go! |
bartoszmajsak
changed the title
Short description of what this resolves:
Fixes problems encountered in #288 and #301
Fixes #309
Note: Use "ignore whitespace changes" to review