Junit5: Extension Point Request: ability to catch a test outcome and fail a test

[vmassol]

See https://gitter.im/junit-team/junit5?at=5c659880ef98455ea4364c08 for details.

Right now it's possible to catch a test failure in handleTestExecutionException() and swallow it. But the other way around is not possible: it seems it's not possible to catch a test, perform some checks and decide to fail the test under some conditions.

I used to implement this in a JUnit4 RunListener and I tried to implement it in JUnit5 TestExecutionListener but I was told it's not the right way.

FTR here's my try:

   @Override
    public void executionFinished(TestIdentifier testIdentifier, TestExecutionResult testExecutionResult)
    {
...
            throw new AssertionError("There should be no content output to the console by the test! Instead we got [" + outputContent + "]");
...    
    }

However this leads to the following test output:

Test ignored.

java.lang.AssertionError: There should be no content output to the console by the test! Instead we got [...]

Notice the "Test ignored".

[sbrannen]

Have you considered implementing AfterEachCallback?

If necessary, you could even register it as a global extension.

[vmassol]

Main problem is that I can't use the service loader mechanism in my use case since I need to be able to disable the "feature" based on a Maven property. So I need to configure surefire to register the listener. I can't use the service loader mechanism even for TELs for that same reason.

Actually I could by trying to read and resolve the pom.xml in the current directory but that becomes awfully complex.

[sbrannen]

How about managing the Maven property via a JVM system property and then having your extension look up the same JVM system property to determine if it should take any action -- much like other auto-registered extensions do not take any action unless a specific annotation is present?

[vmassol]

How about managing the Maven property via a JVM system property and then having your extension look up the same JVM system property to determine if it should take any action -

This is exactly what we do :) But when you're inside your IDE and you execute a unit test, this system property is not set and it depends on the module where the test is (some modules will have the property set to true and others set to false).

[vmassol]

For the curious ones, this is what we were doing with JUnit4 RunListener:

      <!-- Verify that unit tests don't output anything to stdout/stderr -->
      <plugin>
        <groupId>org.apache.maven.plugins</groupId>
        <artifactId>maven-surefire-plugin</artifactId>
        <configuration>
          <systemProperties combine.children="append">
            <!-- This property makes it simple for some module to exclude the check (for example for functional
                 tests) -->
            <property>
              <name>xwiki.surefire.captureconsole.skip</name>
              <value>${xwiki.surefire.captureconsole.skip}</value>
            </property>
          </systemProperties>
          <properties>
            <property>
              <name>listener</name>
              <value>org.xwiki.test.CaptureConsoleRunListener</value>
            </property>
          </properties>
        </configuration>
        <dependencies>
          <dependency>
            <groupId>org.xwiki.commons</groupId>
            <artifactId>xwiki-commons-tool-test-simple</artifactId>
            <version>${project.version}</version>
          </dependency>
        </dependencies>
      </plugin>

And then the RL code:
https://github.com/xwiki/xwiki-commons/blob/2dedbb20f39b2653337de835aad3c8282baadee7/xwiki-commons-tools/xwiki-commons-tool-test/xwiki-commons-tool-test-simple/src/main/java/org/xwiki/test/CaptureConsoleRunListener.java

[vmassol]

And to be complete I'm currently experimenting with the following using ShrinkWrap and inside a TEL (I don't like the fact that I get an ignored test but I can live with that):

...
    private static final String CAPTURECONSOLESKIP_PROPERTY = "xwiki.surefire.captureconsole.skip";

    private static final boolean SKIP = Boolean.parseBoolean(System.getProperty(CAPTURECONSOLESKIP_PROPERTY, FALSE));
...

    /**
     * @return true if the check should be skipper or false otherwise. The {@code xwiki.surefire.captureconsole.skip}
     *         System property is checked first and if it doesn't exist, the Maven property of the same name is read
     *         from the current {@code pom.xml}
     */
    private boolean shouldSkip()
    {
        if (SKIP) {
            return true;
        }

        if (this.skip == null) {
            ConfigurableMavenResolverSystem system = Maven.configureResolver().workOffline();
            system.loadPomFromFile("pom.xml");
            // Hack around a bit to get to the internal Maven Model object
            ParsedPomFile parsedPom =
                ((MavenWorkingSessionContainer) system).getMavenWorkingSession().getParsedPomFile();
            this.skip = Boolean.valueOf(parsedPom.getProperties().getProperty(CAPTURECONSOLESKIP_PROPERTY, FALSE));
        }

        return this.skip;
    }

[sbrannen]

It sounds to me like you may benefit from the JUnit Platform's support for tracking output to STDOUT and STDERR, but within an extension in JUnit Jupiter.

yes?

[sormuras]

_Déjà vu..._ https://gitter.im/junit-team/junit5 ;-)

[vmassol]

@sormuras yep :)

It sounds to me like you may benefit from the JUnit Platform's support for tracking output to STDOUT and STDERR, but within an extension in JUnit Jupiter.

Our objective is to fail a test if it outputs content to the stdout or stderr (it's not to capture the content).

Thanks

[sormuras]

For the record:

Our goal is to fail the test when there's some output done by the test

Ah! That makes sense. In fact, that should be an option for any test run. Globally. Like the new --fail-if-no-tests option of the ConsoleLauncher.

Seeing a --fail-if-any-tests-writes-to-standard-streams option or something similar.

[sbrannen]

Well, tracking something allows you to detect presence or absence.

So what's the difference?

Isn't it like testing whether a boolean is true or false?

[sormuras]

Does "JUnit Platform's support for tracking output to STDOUT and STDERR" remember which test wrote something? If yes, it could be collected and an extra test could verify that collection is empty or fail with a detailed report which test mis-behaved.

[vmassol]

@sormuras about:

Seeing a --fail-if-any-tests-writes-to-standard-streams option or something similar.

Right but this issue was about adding a generic extension point to be able to catch a test result and fail the test on some condition. Similar to what's currently possible with catching a test result and swallowing an exception ie make the test pass.

[vmassol]

So what's the difference?

Making it work in an easy way? If you show me how to do what I need easily then I'm all for it :) If it's about making our build complex by capturing the test output in some files and then register a custom maven plugin (or use a new enforcer rule in the enforcer maven plugin) to check the file content or existence, then it starts to be complex for a simple need, don't you think?

[sbrannen]

Does "JUnit Platform's support for tracking output to STDOUT and STDERR" remember which test wrote something?

Yes. It's tied to the TestDescriptor.

But there's no way to access that from within Jupiter.

[sbrannen]

So what's the difference?

Making it work in an easy way? If you show me how to do what I need easily then I'm all for it :) If it's about making our build complex by capturing the test output in some files and then register a custom maven plugin (or use a new enforcer rule in the enforcer maven plugin) to check the file content or existence, then it starts to be complex for a simple need, don't you think?

I'm not suggesting adding complexity.

The Platform support for STDOUT/STDERR stores the output as a String. So you could test if the String is empty. I think that's pretty simple.

The issue is that an extension in Jupiter cannot currently access that information. Only a TestExecutionListener can currently access that.

[vmassol]

The Platform support for STDOUT/STDERR stores the output as a String

Ok, didn't realize it could be stored as a String.

The issue is that an extension in Jupiter cannot currently access that information. Only a TestExecutionListener can currently access that.

Ok so I could use that in my current TEL. It wouldn't bring me much advantage though since capturing stdout/stderr is really easy in my TEL. Actually it could even be more of a problem since I'd need to use the dedicated JUnit config params (there are 2 AFAICS, for stdout and stderr) instead of the single XWiki one. However it wouldn't solve the issue of throwing the exception to fail the test.

Could you tell me how to access it from my TEL (I'm curious)? I guess it should be available from https://github.com/junit-team/junit5/blob/master/junit-platform-engine/src/main/java/org/junit/platform/engine/TestExecutionResult.java but I can't see it at first glance.

Thanks

[sbrannen]

Could you tell me how to access it from my TEL (I'm curious)? I guess it should be available from https://github.com/junit-team/junit5/blob/master/junit-platform-engine/src/main/java/org/junit/platform/engine/TestExecutionResult.java but I can't see it at first glance.

You'd access those entries under their respective keys in the ReportEntry passed to TestExecutionListener#reportingEntryPublished(TestIdentifier, ReportEntry).

[sbrannen]

However it wouldn't solve the issue of throwing the exception to fail the test.

That's correct.

[tcfurrer]

I've been wanting to do the same thing for my project for a long time. Except with the added twist that I want to set a limit on the amount of stdout/stderr per test, so that tests which output a small amount of text can still pass, but tests which produce ridiculous amounts of text will fail.

So I agree with the request here for a general ability for extensions to do this sort of thing.

[Frontrider]

What I'm trying to do now, is to fail the tests that took too long to execute (a requirement that we got), but all the assertion errors that I throw in the afterEachcallback are captured, and ignored, so the tests are passing, even tough they should fail to notify the devs that it's too slow.

[sbrannen]

@Frontrider, you sorted out those issues with an extension being able to fail the tests -- right?

If so, it might be worth mentioning here just to avoid misleading people. 😉

[Frontrider]

Yep.

[vmassol]

@sbrannen @sormuras Hi. On the XWik project we tried upgrading to JUnit 5.5.2 (from 5.4.2) and found that this breaks our TestExecutionListener that is supposed to mark the test as failing. With JUnit 5.5.x it seems our exception is caught and just logged and no longer fails the test...

The problem is that I don't see a workaround. Anything you could suggest for us?

Right now and because of this we cannot upgrade to JUnit 5.5.x (https://jira.xwiki.org/browse/XCOMMONS-1739) which is a pity.

Thanks!

[marcphilipp]

Why not an AfterEachCallback?

[vmassol]

Actually I'll try using an Extension implementing AfterEachCallback and register it automatically using the service loader mechanism. I'll report back here.

[vmassol]

@marcphilipp yes, that was suggested above and there was some complexity but my code now automatically reads from the current pom.xml so it should be ok. Thanks

[vmassol]

I now remember the issue with Extension and service loader.... The feature is disabled by default which means that XWiki developers will not have it enabled by default in their IDEs when they write and execute tests. It'll work in Maven since we can set the property to enable it for the surefire/failsafe plugnis but it won't work OOB in their IDEs....

[vmassol]

Note: It's not a critical limitation since our build would check it. It was just nice to have it also OOB in the IDE.

[marcphilipp]

If you enable it in src/test/resources/junit-platform.properties it will be used in IDEs and in Maven.

[vmassol]

If you enable it in src/test/resources/junit-platform.properties it will be used in IDEs and in Maven.

hoho.... interesting, didn't know.

However, this won't work in a multimodule project since we would need to have that property placed in all the 300 modules we have, which is a bit tedious ;)

[marcphilipp]

It may be overkill, but you could add another resource folder to each project in the parent POM:

<testResources>
    <testResource>
        <directory>src/test/resources</directory>
    </testResource>
    <testResource>
        <directory>${maven.multiModuleProjectDirectory}/src/test/resources</directory>
    </testResource>
</testResources>

... or use the maven-remote-resources-plugin plugin.

[vmassol]

@marcphilipp Thanks for the idea. It's a bit overkill especially since this would created directory dependencies between maven modules (something we don't do as a best practice).

I've now made the change and it should be ok. There's still a small limitation in that I had to set the junit.jupiter.extensions.autodetection.enabledproperty in the surefire config of our the top level pom.xml so that all modules inherit from it. However, this will work only if submodule poms are written correctly, i.e. if they use <systemPropertyVariables combine.children="append"> in their maven surefire configuration (for modules that also introduce system properties), as otherwise the custom Extension won't be loaded and that will be silently and without anyone noticing ;)

But we should be able to live with it FTM :) Any idea when and if the junit.jupiter.extensions.autodetection.enabledproperty will be on by default in JUnit5?

Thanks again

EDIT: FTR the changes done in case it can help someone: https://github.com/xwiki/xwiki-commons/commit/d2d0904b0c82e674f7ac1f022433b008ed186077#diff-600376dffeb79835ede4a0b285078036R1853 (discard the change done to IsolatedTestRunner.java that was committed by mistake ;)).

[marcphilipp]

Any idea when and if the junit.jupiter.extensions.autodetection.enabledproperty will be on by default in JUnit5?

Since it would be a potentially breaking change I don't see it happening before in a feature release.

I'm glad it works now. Can we close this issue or is there anything left to do?

[vmassol]

Can we close this issue or is there anything left to do?

I guess so. It would still be nice to have a proper API to change the result of a test but I guess throwing an exception is acceptable (if it can be considered an API/contract and that it's not broken in the future as it was for a listener for ex).

Thanks

[marcphilipp]

Yes, that's definitely part of the API contract. We've always said a listener should not be able to influence the outcome because it might lead to inconsistencies (e.g. the test might already have been reported as successful to other listeners) so it wasn't officially supported in the first place. For a callback it's a different story and we have tests in place that verify exceptions lead to failing tests. Thanks for the feedback!