[Gradle + TestNG] Need for a new Listener to notify TestNG xml listener initialization failures

sravanmedarapu commented 1 year ago

TestNG Version

7.8.0

Note: only the latest version is supported

Context: Gradle + TestNG

Gradle useTestNG configuration:(Expand)

```kotlin tasks.test { useTestNG { listeners.add("com.listeners.DefaultSuiteListener") // registered first before registering other listeners from xml suites("src/test/kotlin/config/functional_tests.xml") } } ```

TestNG XML configuration:(Expand)

```xml ```

Expected behavior

On TestNG Listeners initialisation failure, already configured listeners(if any) need to be notified, something like ListenerConfigStatusNotifier/ListenerSetupReporter

Actual behavior

TestNG throws TestNGException and gradle logs this exception only in debug mode(gradle test --debug) or one need to implement and register gradle API's TestListener to gradle lifecycle, however this is not flexible option as gradle API internal to build.gradle.

I'm happy to take up this request once we agree on the need for it.

Is the issue reproducible on runner?

[x] Shell
[ ] Maven
[x] Gradle
[ ] Ant
[ ] Eclipse
[ ] IntelliJ
[ ] NetBeans

Test case sample

Please, share the test case (as small as possible) which shows the issue

here is the sample project to reproduce and demonstrate the problem. https://github.com/sravanmedarapu/GradleTestNgScratchpad/tree/main

krmahadevan commented 1 year ago

@sravanmedarapu - For a second, lets take gradle out of the equation. Can you please help share a sample that just uses the TestNG API and create a main method that can be used to reproduce the problem? I would like to start from there. That way we know if its the build tool integration with TestNG that is broken, or if its really TestNG.

I say this because I think Gradle embeds a testng version in it and that is perhaps not really the latest version of TestNG

sravanmedarapu commented 1 year ago

@krmahadevan here is the sample.

Providing context on the Gradle flow, here's how Gradle launches TestNG and handles exceptions:

Gradle initiates TestNG to execute tests. If any problems arise with the listener or suite setup/execution during this process, the TestNG API should be able to inform about these failures.
Gradle catches exceptions thrown by TestNG and logs them in debug mode or notifies the Gradle listener. However, this Gradle listener is limited to the build script (build.gradle or build.gradle.kts) and cannot be extended to Kotlin/Java projects to perform additional actions, such as reporting to Slack, Elastic, etc.

Please note that the Gradle listener in the build script can be utilized for various purposes related to the build process, but it may not be directly accessible in external Kotlin/Java code. However, other mechanisms, such as custom TestListeners within TestNG, can be used to achieve additional actions beyond the build script's scope.

krmahadevan commented 1 year ago

@sravanmedarapu - Thanks for sharing that sample. When you run the main method TestNG explicitly fails at instantiation. I think this is the same behaviour using Maven as well.

Shouldn't Gradle also be doing the same thing ?

We could build a ListenerConfigStatusNotifier/ListenerSetupReporter but the same instantiation problem could happen with them as well right? So where would this end?

@juherr your thoughts ?

krmahadevan commented 1 year ago

The below sample project can be used to see this in action.

testng-sample.zip

[INFO] --- compiler:3.11.0:testCompile (default-testCompile) @ testng-sample ---
[INFO] Changes detected - recompiling the module! :dependency
[INFO] Compiling 2 source files with javac [debug target 11] to target/test-classes
[INFO] 
[INFO] --- surefire:3.1.2:test (default-test) @ testng-sample ---
[INFO] Using auto detected provider org.apache.maven.surefire.testng.TestNGProvider
[INFO] 
[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.testng.AppTest
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
[ERROR] 
Couldn't find a constructor in class org.testng.MyListener
[INFO] 
[INFO] Results:
[INFO] 
[INFO] Tests run: 0, Failures: 0, Errors: 0, Skipped: 0
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  3.115 s
[INFO] Finished at: 2023-07-26T17:38:24+05:30
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.1.2:test (default-test) on project testng-sample: 
[ERROR] 
[ERROR] Please refer to /Users/krishnamahadevan/temp/testng-sample/target/surefire-reports for the individual test results.
[ERROR] Please refer to dump files (if any exist) [date].dump, [date]-jvmRun[N].dump and [date].dumpstream.
[ERROR] There was an error in the forked process
[ERROR] 
[ERROR] Couldn't find a constructor in class org.testng.MyListener
[ERROR] org.apache.maven.surefire.booter.SurefireBooterForkException: There was an error in the forked process
[ERROR] 
[ERROR] Couldn't find a constructor in class org.testng.MyListener
[ERROR]         at org.apache.maven.plugin.surefire.booterclient.ForkStarter.fork(ForkStarter.java:628)

juherr commented 1 year ago

TestNG throws exceptions when something goes wrong outside the test perimeter.

Here, the bootstrap is failing and testng throws an exception as expected.

If something should be fixed, it should be in the integration. Maybe a dedicated exception could be used if it helps.

sravanmedarapu commented 1 year ago

@krmahadevan / @juherr Agree that maven is bit more clear on reporting/logging the problem.

Gradle could do a better job in reporting/logging, will check gradle team on simplifying the logging. However Gradle could only control logging/exception handing.

Shouldn't Gradle also be doing the same thing ?
Absolutely, TestNG provides notification/listener mechanisms at various levels, such as Execution, Suite, Test, and more. Incorporating a initialize/pre-setup layer with a comprehensive listener would indeed cover the entire lifecycle.

Current TestNG.run() implementation(Click)
```java public void run() { initializeEverything(); // <=== Not covered with listeners sanityCheck(); // <=== Not covered with listeners // Everything else covered with listeners runExecutionListeners(true /* start */); runSuiteAlterationListeners(); m_start = System.currentTimeMillis(); List suiteRunners = runSuites(); m_end = System.currentTimeMillis(); if (null != suiteRunners) { generateReports(suiteRunners); } runExecutionListeners(false /* finish */); ... } ```

We could build a ListenerConfigStatusNotifier/ListenerSetupReporter but the same instantiation problem could happen with them as well right? So where would this end?

Indeed, having a Listener mechanism in TestNG grants more control over additional activities beyond just logging or throwing exceptions. It allows for custom actions such as reporting build problems to Teamcity, storing relevant events in a database or Elastic, and sending notifications to Slack or other communication channels.

krmahadevan commented 1 year ago

@sravanmedarapu - The question is more about how does one deal with instantiation failures on the newer bootstrap listeners, because from what I see, we can still end up with problems instantiating them via reflection. I am more interested in that part of it.
So the question is definitely not about the intent of introducing newer listeners, but the concern is more around the problem will exist even there as well.

Also even if TestNG does provide such a listener, how would a Gradle driven test consume it because from what you shared so far I understand that:

Gradle gobbles all exceptions raised by TestNG and logs them in debug mode.
The Gradle provided listeners which can eavesdrop into such problems are confined to be accessible only from within the realm of a Gradle build file.

The crux of the problem seems to be with Gradle gobbling exceptions from TestNG (Not sure what would the rationale behind that be, especially when TestNG is the test runner that it is piggy backing for test execution). I was hoping that if we can get that part sorted out from Gradle wherein they don't swallow exceptions and instead let them bubble up and fail the build, maybe the issue would get resolved (Please bear if that sounds naive, but that's the level of my knowledge with respect to Gradle and its internals)

@juherr

Maybe a dedicated exception could be used if it helps.

Currently we throw a TestNGException . We can throw something else, but that doesn't change anything because Gradle is basically catching Throwable. So anything we throw is going to be gobbled up.

# Created at 2023-07-26T18:37:29.347
org.testng.TestNGException: 
Couldn't find a constructor in class org.testng.MyListener
        at org.testng.internal.objects.InstanceCreator.newInstance(InstanceCreator.java:57)
        at org.testng.ITestObjectFactory.newInstance(ITestObjectFactory.java:10)
        at org.testng.TestNG.setListenerClasses(TestNG.java:684)
        at org.testng.TestNG.configure(TestNG.java:1549)
        at org.testng.TestNG.configure(TestNG.java:1768)

sravanmedarapu commented 1 year ago

@krmahadevan here is the draft of what I am thinking https://github.com/testng-team/testng/compare/master...sravanmedarapu:testng:master

This should work with TestNG + Gralde flow or whoever using TestNG runner, happy to test with bootstrap listeners with bit of direction.

we can still end up with problems instantiating them via reflection

It's just not about logging only, as briefed earlier Gradle has TestListener to notify these events which can be used for logging but it's limited to gradle build script. My request is to bring this ability into TestNG API i.e. kotlin/java projects which gives flexibility to do extra activities by utilising framework.

Also even if TestNG does provide such a listener, how would a Gradle driven test consume it because from what you shared so far I understand that:

Gradle gobbles all exceptions raised by TestNG and logs them in debug mode.

The Gradle provided listeners which can eavesdrop into such problems are confined to be accessible only from within the realm of a Gradle build file.

In general, Listeners remain within the suite perimeter, at least from the perspective of XML configuration. TestNG's lifecycle includes various listeners to notify about Tests, Suites, execution failures, and statistics. Given that Listeners are an integral part of this lifecycle, it's reasonable to expect a dedicated one for handling Listener failures as well.

<suite name="Suite">
  <listeners>
    <listener class-name="ListenerWithRuntimeError"/>
  </listeners>
  <test name="Test">
      <classes>
          <class name="SampleTestClassSample"/>
      </classes>
  </test>
</suite>

Introducing this new listener would not only prove beneficial for our team but also for the wider TestNG user community. Would request you to give consideration to this request.

krmahadevan commented 1 year ago

@sravanmedarapu - I am not sure if you are getting the fundamental question that I am asking.

What would happen if TestNG fails to instantiate this new listener? TestNG will continue to throw an exception which is again going to be gobbled up by Gradle. So what are we solving here? This is what I would like to understand.

Introducing another listener into the mix is NOT going to solve the problem of Gradle swallowing TestNG triggered exceptions thrown from listener instantiation failures. How would that issue be addressed by this PR is what I am trying to figure out here.

krmahadevan commented 1 year ago

In short, what I would like to know is :

If TestNG fails with instantiating the existing listeners, this new listener would get notified.
Who would get notified if TestNG fails to create an instance of this new listener ? Given the fact that this new listener is also a TestNG known listener.

krmahadevan commented 1 year ago

@juherr - Please chime in, incase I am missing something obvious here. Basically the ask is to figure out a way to be notified of TestNG bootstrap failures.

We have two options for such notifications:

We can throw a special exception. But I guess it wouldnt work with Gradle because its gobbling all Throwable
We can introduce a special call back. But this would mean that TestNG should instantiate this call back and reflection is the only way of doing it. TestNG can fail to instantiate this new callback as well. So we are back to square one.

juherr commented 1 year ago

In my opinion, the "fix" can only be done in 3rd party integration where the runner lifecycle is managed.

In case of gradle, it should not be too complicated to create a custom plugin using the TestNG api if the embeded one is not good enough.

sravanmedarapu commented 1 year ago

@krmahadevan Apologies for not being precise here.

Yes, If TestNG fails to instantiating existing listeners(listeners mentioned in XML files), the proposed ListenerSetupReporter will be notified because new listener registered first before TestNG initialise XML listeners(using gradle API useTestNG build script).

If TestNG fails with instantiating the existing listeners, this new listener would get notified.

This exception will gobbled by gradle but can be covered by Gradle TestListener to log.

Who would get notified if TestNG fails to create an instance of this new listener ? Given the fact that this new listener is also a TestNG known listener.

krmahadevan commented 1 year ago

@sravanmedarapu - A TestNG listener can be wired in using any means, but eventually the instantiation is always going to be via reflection. So any failure in instantiating a TestNG listener should be noticeable via the TestListener from gradle which I believe can be configured to be aborted as well.

Please refer to the comment from https://github.com/testng-team/testng/issues/2937#issuecomment-1653520145

I think we are fixing the problem in the wrong place. The origin of the problem is NOT testng but the integration between Gradle and TestNG. IMO, the fix should also be there and NOT at TestNG.

krmahadevan commented 1 year ago

I believe that is essentially what is being said in this issue as well, which you created at Gradle project front https://github.com/gradle/gradle/issues/25857#issuecomment-1653298479

sravanmedarapu commented 1 year ago

@krmahadevan and @juherr , thank you for taking the time to review my argument.

I understand that I may not have been able to persuade enough with my points. Nevertheless, I will leave the draft of my proposal here for future reference and consideration.

krmahadevan commented 10 months ago

@juherr - I was thinking of closing this off, since the issue is not within TestNG. Your thoughts ?

juherr commented 10 months ago

@krmahadevan yes, agree

krmahadevan commented 10 months ago

Closing this issue since there's nothing to be done from TestNG side.

testng-team / testng