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

[SUREFIRE-1935] Upgrade to JUnit Platform 1.8, start Launcher via LauncherSession #389

Merged
merged 1 commit into from Jan 20, 2022

Conversation

papegaaij
Copy link
Contributor

A LauncherSession (JUnit Platform 1.8 feature) is started when the Launcher is first used and closed when all tests have run. This allows integrations to run pre and post fixtures.

@Tibor17
Copy link
Contributor

Tibor17 commented Sep 14, 2021 via email

@papegaaij
Copy link
Contributor Author

You should not take everything into one session. There are reruns which have to be isolated trom the main run. And I am convinced that the reruns also have to be isolated in each.

I did think about this, but as the contract of LauncherSession is very unclear about this, I decided to put it all in one session. The documentation of LauncherSession states The LauncherSession API is the main entry point for client code that wishes to repeatedly discover and execute tests using one or more test engines. IMHO, running the same test multiple times also falls under 'repeatedly discover and execute tests'. I guess this really depends on the way it is actually used which one is correct.

It will require some work to open a new session for each failure-iteration, but I can probably do that later this week.

@papegaaij
Copy link
Contributor Author

The PR is updated to start a new LauncherSession for every iteration during the failure reruns.

@slawekjaranowski
Copy link
Member

please rebase with current code

@papegaaij
Copy link
Contributor Author

Done

@papegaaij
Copy link
Contributor Author

Any progress on this?

@Tibor17
Copy link
Contributor

Tibor17 commented Jan 19, 2022

@papegaaij
We are in hurry with many PRs.
Pls let us review again against Jira...

@papegaaij
Copy link
Contributor Author

We have been running a patched build for several months now with this change and it works very well. The downside to this solution in relation to the jira issue is that it only helps when the framework actually makes use of the launcher sessions. This is still very rare as it is a new feature. It is however the recommended way (by JUnit) of doing this and they seem reluctant to make other changes in this regard.

@Tibor17
Copy link
Contributor

Tibor17 commented Jan 19, 2022

I understand that you are aiming for loading heavy resources only once.
On the other hand the unit tests have a rule where each test/method has isolated context.
How can we ensure such rule in this patch?
The interface Extension is marker interface and has many implementations. We only need to guarantee that the session you have implemented would not make e.g. BeforeEachCallback stateful across all tests (before-each represents stateless resource).

@papegaaij
Copy link
Contributor Author

The LauncherSession is explicitly meant to provide a state over all tests. It is completely separate from Extension. This patch will only effect projects that already use these LauncherSessions. For that, they have to implement LauncherSessionListener and put the classname in a service file for the ServiceLoader to pick it up. If a project makes use of LauncherSessionListener, they will, without this patch, see a new session being started for every test, which actually is incorrect behavior. The javadoc on LauncherSession explains this: The LauncherSession API is the main entry point for client code that wishes to repeatedly discover and execute tests using one or more test engines. This is exactly what Surefire does, hence it should use LauncherSession to do this.

What a user does with a LauncherSessionListener implementation is up to the user. The javadoc does not give any guidelines. The Gradle documentation actually points to this interface for once-per-vm setup and teardown: https://docs.gradle.com/enterprise/test-distribution-gradle-plugin/#junit_5_8_and_later

@Tibor17
Copy link
Contributor

Tibor17 commented Jan 19, 2022

@papegaaij
Pls squash both commits. I have made the review but I prefer one commit in the final review. Thx.
Looks fine so far.

@papegaaij
Copy link
Contributor Author

@Tibor17 I've rebased the PR and squashed the commits.

@Tibor17 Tibor17 self-requested a review January 19, 2022 16:34
}
catch ( Exception e )
{
throw new SurefireReflectionException( e );
Copy link
Contributor

Choose a reason for hiding this comment

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

pls use throw new SurefireReflectionException( e.grtLocalMessage(), e );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SurefireReflectionException only has one constructor, the one taking a Throwable cause.

Copy link
Contributor

Choose a reason for hiding this comment

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

@papegaaij
ok, no problem.

@Tibor17
Copy link
Contributor

Tibor17 commented Jan 19, 2022

LGTM
Some test has timed out in the surefire-api module but I am convinced it has nothing to do with this change.
Let's wait for the CI to complete.

@Tibor17 Tibor17 merged commit 97afea1 into apache:master Jan 20, 2022
@Tibor17
Copy link
Contributor

Tibor17 commented Jan 20, 2022

Thx for contributing!

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