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-1710] skipAfterFailureCount in JUnit5 provider #503

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Tibor17
Copy link
Contributor

@Tibor17 Tibor17 commented Mar 28, 2022

This is my old change from my shelf. Providing it open. The ITs are broken and the impl should be fixed.
Implements the config parameter for JUnit5 skipAfterFailure.

Following this checklist to help us incorporate your
contribution quickly and easily:

  • Make sure there is a JIRA issue filed
    for the change (usually before you start working on it). Trivial changes like typos do not
    require a JIRA issue. Your pull request should address just this issue, without
    pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [SUREFIRE-XXX] - Fixes bug in ApproximateQuantiles,
    where you replace SUREFIRE-XXX with the appropriate JIRA issue. Best practice
    is to use the JIRA issue title in the pull request title and in the first line of the
    commit message.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean install to make sure basic checks pass. A more thorough check will
    be performed on your pull request automatically.
  • You have run the integration tests successfully (mvn -Prun-its clean install).

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

@Tibor17
Copy link
Contributor Author

Tibor17 commented Mar 28, 2022

@andyRokit
@mpkorstanje
This is the implementation for https://issues.apache.org/jira/browse/SUREFIRE-1743 and https://issues.apache.org/jira/browse/SUREFIRE-1710 but it still does not work properly. Whatever value is set in skipAfterFailure, it still stops the execution with 2 failures. If you guys have a guess, pls ping us.

@Tibor17
Copy link
Contributor Author

Tibor17 commented Mar 28, 2022

The class FailFastJUnit5IT should be renamed...

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Mar 28, 2022

Cheers. I don't mind taking a look but at a glance this will only work with JUnit Jupiter rather then every TestEngine on the JUnit Platform. The current implementation is tied to JUnit Jupiter APIs.

It won't work for example with JUnit 4 (through JUnit 5s Vintage module) or Cucumber.

@mpkorstanje
Copy link
Contributor

Overall I think it would be more effective to first resolve junit-team/junit5#1880 before trying to make it work in surefire.

@Tibor17
Copy link
Contributor Author

Tibor17 commented Mar 29, 2022

Yes, it would be worth to have the fix of JUnit5 issue I reported.
Unfortunately, the JUnit5 team does not care ;-)

@mpkorstanje
Copy link
Contributor

I believe the JUnit team uses community engagement as a measure of importance and interest.

Now it seems the conversation never moved on after the brainstorming. I think they are waiting for other people - possibly me, you or someone else - to reply to the brainstorm suggestions or provide their own. And then move on towards an implementation.

I've had no problems getting other features merged provided I did the heavy lifting. So I don't think this is a lack of care or interest. It is just not the most important thing compared to everything else. Especially with no or limited apparent community interest.

I do suspect that if someone has a sufficient need or interest in this feature it will be implemented. With that in mind I think it'd be better not to implement this in an ad-hoc fashionen. It will hopefully make some contributor come out of the woodwork.

Or if not, the feature wasn't really needed.

On a personal note: Due to circumstances I've had to significantly reduce the scope of my involvement. I think it is unlikely I'll be able to invest time getting this implemented on the JUnit side.

@Tibor17
Copy link
Contributor Author

Tibor17 commented Mar 31, 2022

@marcphilipp
Hi Marc,

I have tried to implement the feature pleaseStop() by other way than we would expect in junit-team/junit5#1880
I have no idea what's wrong with my two SPIs. But I guess the native support would be booletproof in JUnit5 engines and API.

@marcphilipp
Copy link
Contributor

Overall I think it would be more effective to first resolve junit-team/junit5#1880 before trying to make it work in surefire.

I agree. The only other solution I see is to do this from the Maven JVM, i.e. terminating the fork, instead of doing it in the provider.

@Tibor17
Copy link
Contributor Author

Tibor17 commented Mar 31, 2022

This feature is implemented in all providers JUnit4 and TestNG. The same principle - we are counting the errors inside and outside of the JVM. If you have one fork then of course you have to count the failures/errors inside. The first happens in the concurrent model that wins and notification is sent and the framework like JUnit4 or TestNG is called with the method pleaseStop() and the framework stops "scheduling" new tests. Stopping the JVM is too naive.

@Vampire
Copy link

Vampire commented Feb 2, 2023

If this is going to make it into surefire without junit-team/junit5#1880 being fixed and used, I think stuff you called "junit5" should be renamed to "jupiter".

Spock 2+ for example is also a JUnit 5 engine, but would not be handled by your solution as far as I have seen from a very quick look. Probably the same for other engines like Vintage that runs JUnit 4 tests as JUnit 5 engine. At least providing some Jupiter services in common-junit5 makes me think it only supports the Jupiter engine and should thus be common-jupiter. :-)

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

Successfully merging this pull request may close these issues.

4 participants