TestNG 6.10+ skips tests due to duplicate listener: org.pitest.testng.TestNGAdapter

[MartinKanters]

Issue description

I am having issues with some versions of pitest together with some versions of TestNG.
In the pitest logs I see the following error: "Ignoring duplicate listener : org.pitest.testng.TestNGAdapter" exactly as many times as mutants there are made minus 1.
The result is that pitest mutants are surviving because the test stops due to a SkipException (org.testng.SkipException: skipping). This gives a lot of false positives and errors in the logs.

Versions

		TestNG
		6.9.13.6	6.10	6.11
pi test	1.1.4	OK	OK	OK
	1.1.5	OK	NOK	NOK
	1.1.11	OK	NOK	NOK

Test project

I have made the following minimal project to test it: https://github.com/MartinKanters/pitest-testng-issues

Log excerpt

stdout  : [TestNG] [WARN] Ignoring duplicate listener : org.pitest.testng.TestNGAdapter
stderr  : org.testng.SkipException: skipping
        at org.pitest.testng.TestNGAdapter.onTestStart(TestNGAdapter.java:74)
        at org.testng.internal.Invoker.runTestListeners(Invoker.java:1739)
        at org.testng.internal.Invoker.runTestListeners(Invoker.java:1714)
        at org.testngstderr  : .internal.Invoker.invokeMethod(Invoker.java:640)
        at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:869)
        at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1193)
...

### Related TestNG issue

I believe this issue is related, but I am not sure whether the issue should be fixed in only TestNG or in pitest as well as I don't know the projects well enough.

[hcoles]

Thanks for reporting this - there looks to have been some changes in TestNG that prevent pitest removing listeners. Unfortunately pitest needs to recycle a single TestNG instance to make things work with JMockit.

There also look to have been some changes that mean tests can no longer be skipped by throwing a SkipException from an ITestListener.

I've had a play using a mutable listener to avoid having to add/remove one. This is hideous but seems to work.

I've almost got the skipping working as well.

Not confirmed yet but I strongly suspect the updated code will not work with older versions of TestNG.

[MartinKanters]

Thanks for picking it up so quickly!

Too bad that they don't offer a way to remove listeners, perhaps we should reach out to TestNG to include this functionality? It doesn't seem that hard.
Nice to see that you have almost overcome the skipping problem as well.

Let me know when I can help with testing.

[hcoles]

Having a minimal project that reproduces the issue speeds things up hugely - thanks.

I think removing listeners is a pretty niche use case so I'd be surprised if the TestNG team wish to add it to their api. Pitest only does it because it JMockit forces the use of a single static instance of TestNG - it would normally be much more sensible to just create a new instance.

Fortunately it looks like I can get things working without a change on the TestNG side.

[MartinKanters]

Alright! It helped me a lot as well figuring out which versions of pitest and TestNG were conflicting.

Great to hear that you can probably get it to work without any change on their side.

[krmahadevan]

@hcoles

Thanks for reporting this - there looks to have been some changes in TestNG that prevent pitest removing listeners.

I have been part of the TestNG codebase and try and contribute every now and then. I dont remember seeing an API that ever used to let you do it directly. All said and done, you can still invoke one of the below methods to remove listeners (because the underlying list is currently being exposed atleast as part of 6.11 the latest released version it is)

• org.testng.TestNG#getReporters
• org.testng.TestNG#getTestListeners
• org.testng.TestNG#getSuiteListeners

Would that work for you ?

There also look to have been some changes that mean tests can no longer be skipped by throwing a SkipException from an ITestListener.

I doubt if this is the case. Here's a sample that I created, which when executed, does in fact skip the tests.

package org.rationale.emotions;

import org.testng.ITestResult;
import org.testng.SkipException;
import org.testng.TestListenerAdapter;
import org.testng.annotations.Listeners;
import org.testng.annotations.Test;

@Listeners(SkipTestsSample.MyListener.class)
public class SkipTestsSample {
    @Test
    public void hello() {
        System.err.println("Hello world");
    }

    public static class MyListener extends TestListenerAdapter {

        @Override
        public void onTestStart(ITestResult result) {
            throw new SkipException("Forcing the test to skip");
        }
    }
}

[hcoles]

@krmahadevan

Pitest currently calls getTestListeners().remove(xx) but this no longer works as TestNG now makes a defensive copy of the list.

https://github.com/cbeust/testng/blob/master/src/main/java/org/testng/TestNG.java#L824

The listener pitest creates starts throwing a SkipException for all subsequent onTestStart calls after the first Exception is received.

This used to work but this catch-and-printstacktrace block

https://github.com/cbeust/testng/blob/master/src/main/java/org/testng/internal/Invoker.java#L1746

Now results in TestNG still executing the test methods.

Throwing a SkipException in an InvokedMethodListener seems to still work.

[krmahadevan]

@hcoles - My bad.. my IDE was playing games and I was referring to some old version of TestNG. Yeah you are right in terms of the listeners removal being stopped.

[hcoles]

@krmahadevan Was the change in behaviour on throwing SkipException in a ITestListener intentional? If not I can raise a defect.

[krmahadevan]

@hcoles - To be honest, I don't quite know. You can perhaps file this as a bug on the TestNG issues. We can then have it vetted with Julien and Cedric there to find out the actual intention.

[juherr]

@hcoles Please open an issue. Add/Remove a listener worked before but it was a side effect.
I didn't think someone was using it but if you really need it we will have to think how to add the feature nicely ;)

[hcoles]

@juherr Thanks but I've managed to code round the issue.

[hcoles]

Released in 1.2.0

[MartinKanters]

Verified that the fix is working. Thanks for your quick response to this issue!