[olamy]

Signed-off-by: Olivier Lamy olamy@apache.org

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

• Each commit in the pull request should have a meaningful subject line and body.
• 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.

• I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004

• In any other case, please file an Apache Individual Contributor License Agreement.

[lprimak]

With this PR, I get the below output.

I would still say that's "too many times"
I think that the extension should only be called once, and it's getting called 3 times now (was 4 times before this fix)

[INFO] --- surefire:3.6.1-SNAPSHOT:test (default-test) @ maven-testng-reproducer ---
[INFO] Using auto detected provider org.apache.maven.surefire.junitplatform.JUnitPlatformProvider
[INFO] TestNG is present. Adding per default org.junit.support:testng-engine:1.1.0
[INFO] 
[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
MyTestNGExtension: onStart called for suite: Command line suite
MyTestNGExtension: onFinish called for suite: Command line suite
MyTestNGExtension: onStart called for suite: Command line suite
MyTestNGExtension: onFinish called for suite: Command line suite
[INFO] Running com.flowlogix.maven.jupiter.JupiterTest
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.016 s -- in com.flowlogix.maven.jupiter.JupiterTest
MyTestNGExtension: onStart called for suite: Command line suite
[INFO] Running com.flowlogix.maven.testng.NextGenTest
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.003 s -- in com.flowlogix.maven.testng.NextGenTest
MyTestNGExtension: onFinish called for suite: Command line suite
[INFO] 
[INFO] Results:
[INFO] 
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0

[olamy]

Thanks for testing!
In your case, 3 is currently expected: the testng-engine discovers tests by running TestNG in dry-run mode, which notifies ISuiteListeners, and the JUnit Platform always discovers before executing. (see comment here #3462 (comment) and javadoc here https://github.com/junit-team/testng-engine/blob/78e75694529cca30bb741d4363c318ecaa2058d5/src/main/java/org/junit/support/testng/engine/TestNGTestEngine.java#L82-L89 )

So 1 isn't reachable from Surefire alone. Surefire still discovers once more than it needs to during the class scan, though, and if possible, I'll look at getting that down to 2.
In the meantime, your listener can skip the dry runs with if (Boolean.getBoolean("testng.mode.dryrun")) return; and you'll get exactly one call.

[lprimak]

Thanks for the explanation, and the testng.mode.dryrun workaround works for me. Still, listeners can't tell a discovery pass from a real suite without knowing that internal property, which is surprising. I opened an issue in testng-engine to discuss avoiding or signaling this: junit-team/testng-engine#440

Meanwhile, 2 calls from Surefire (one discovery + one execution) is a reasonable interim state, and this PR is a clear improvement over 4.

[olamy]

Thanks for testing! I pushed a change so the class scan no longer asks the TestNG engine, which brings it down to 2. The remaining extra call is the testng-engine's dry run during discovery (source). The JUnit Platform is always discovered before execution, so Surefire can't avoid it.
Have a squeeze and let me know.

[lprimak]

Confirmed. Only called 2 times with the PR in it's current state.
Maybe we wait for JUnit team to respond to the new issue?
I am good either way, this PR is better than the status quo, and we can improve later when/if JUnit team resolves the issue.

Thanks for testing! I pushed a change so the class scan no longer asks the TestNG engine, which brings it down to 2. The remaining extra call is the testng-engine's dry run during discovery (source). The JUnit Platform is always discovered before execution, so Surefire can't avoid it. Have a squeeze and let me know.

[github-actions]

@olamy Please assign appropriate label to PR according to the type of change.