Segregate native & user listeners before ordering

[krmahadevan]

Closes #2771

Fixes #2771 .

Did you remember to?

• Add test case(s)
• Update CHANGES.txt
• Auto applied styling via ./gradlew autostyleApply

We encourage pull requests that:

• Add new features to TestNG (or)
• Fix bugs in TestNG

If your pull request involves fixing SonarQube issues then we would suggest that you please discuss this with the
TestNG-dev before you spend time working on it.

Note: For more information on contribution guidelines please make sure you refer our Contributing section for detailed set of steps.

[juherr]

I don't get the link between the issue and the need to have an order in listeners. Could you explain?

[krmahadevan]

Test specific listener is altering the state of a test.
There is a native testng listener which determines the state of the test to decide on if the overall result passed or failed.

Now we are ensuring that we maintain insertion order but that is specific only to test listeners and not applicable to native listeners.

[juherr]

Ok that's clear.

Why not having only the mandatory listeners in the list? In mean exclude the reporters at least.

Otherwhise, why not introducing an annotation with a priority value and allow users to plug their listener in the level they want?

[krmahadevan]

@juherr

Why not having only the mandatory listeners in the list? In mean exclude the reporters at least.

I just kept the intent simple of segregating all native listeners (which includes listeners as well).

Otherwise, why not introducing an annotation with a priority value and allow users to plug their listener in the level they want?

Because it can still be interfered by a user, by defining the same priority for their listeners.

My intent was to just have them separated and ensure that the TestNG listeners always honour what a user's listener may have done in terms of changing the state of one or more tests.

[krmahadevan]

ping @juherr - Can you please let me know if I can go ahead with merging this PR ?

[krmahadevan]

ping @juherr

[juherr]

Sorry but I still don't understand the issue or the fix.
Could tell me what is the native testng listener you are referring to previously?
Why is IntelliJ involved in the fix?

[krmahadevan]

Sorry but I still don't understand the issue or the fix. Could tell me what is the native testng listener you are referring to previously? Why is IntelliJ involved in the fix?

• Native TestNG listeners - Those listeners which are wired in by default by TestNG and which a user does not have an option to un-wire.
• IntelliJ is involved because when a user runs the tests from within IntelliJ then the behaviour has to be consistent in terms of the ordering. So for a UI run, we pretend as if the IntelliJ built listener is also like a built-in TestNG listener so that the user experience does not suffer.

@juherr - Please let me know if that answers the question.

[juherr]

I don't understand everything but what I propose is:

• Rename the new configuration in something like INTERNAL_LISTENER_PACKAGES where org.testng,com.intellij.rt.testng will be the default value
• Parse the configuration as a list of packages and apply the logic on the list
• Explain in ListenerOrderDeterminer why we need a specific treatment for internal listeners

WDYT?

[krmahadevan]

I don't understand everything but what I propose is:

• Rename the new configuration in something like INTERNAL_LISTENER_PACKAGES where org.testng,com.intellij.rt.testng will be the default value
• Parse the configuration as a list of packages and apply the logic on the list
• Explain in ListenerOrderDeterminer why we need a specific treatment for internal listeners

WDYT?

The current implementation is doing all of this already except that, instead of taking in a package, I am dealing with hard coded listener names. I resorted to this because I didn't want any user created listener under the package org.testng to be regarded as a TestNG's built in listener. TestNG knows all its internal listeners, and the current implementation is explicitly calling out all of them as an elaborate list of fully qualified class names rather than resorting to package names.

For the documentation part, I have added javadocs at the ListenerOrderDeterminer class level. Do you want them to be more elaborate? They are not going to be used by users and they don't have any role to play in the user experience apart from ensuring that the TestNG's internal listeners are invoked (before in the case of beforeXX events and after in the case of afterXX events)

[juherr]

The current implementation is doing all of this already except that, instead of taking in a package, I am dealing with hard coded listener names. I resorted to this because I didn't want any user created listener under the package org.testng to be regarded as a TestNG's built in listener.

But doing it under com.intellij.rt.testng is possible without effort.

TestNG knows all its internal listeners, and the current implementation is explicitly calling out all of them as an elaborate list of fully qualified class names rather than resorting to package names.

I still think reporters should be excluded from the list because they are not supposed to be involved in the test lifecycle or change states.

For the documentation part, I have added javadocs at the ListenerOrderDeterminer class level. Do you want them to be more elaborate? They are not going to be used by users and they don't have any role to play in the user experience apart from ensuring that the TestNG's internal listeners are invoked (before in the case of beforeXX events and after in the case of afterXX events)

The current javadoc explains the what but not the why. It is dedicated not to users but to devs who will update the base code later. Just explain why internal listeners must be in a specific order.
But if ConfigurationListener or TestRunner.ConfigurationListener must be run before or after listeners, it just highlights that they should not be listeners but called directly.

[krmahadevan]

But doing it under com.intellij.rt.testng is possible without effort.

Yes and I didn't find any other way in which I can accommodate user behaviour within an IDE. I first thought I would hard code this, but then realised that if the IntelliJ listener changed its package, then this behaviour would be broken. If needed, I can have this removed, but this would be a sub-standard user experience.

I am open to any suggestions that can streamline this.

I still think reporters should be excluded from the list because they are not supposed to be involved in the test lifecycle or change states.

The api does not restrict this and so I accommodated the reporting listeners as well.

The current javadoc explains the what but not the why. It is dedicated not to users but to devs who will update the base code later. Just explain why internal listeners must be in a specific order.

Added this documentation and pushed the change.

But if ConfigurationListener or TestRunner.ConfigurationListener must be run before or after listeners, it just highlights that they should not be listeners but called directly.

Yes, that's a valid point. But right now they are too intertwined and I can de-couple them in a separate PR (and am glad that via this PR, this discrepancy came out)

[juherr]

Yes, that's a valid point. But right now they are too intertwined and I can de-couple them in a separate PR (and am glad that via this PR, this discrepancy came out)

Could you have a try in another branch? I think it should be quick to inject 2 object instances and call them when needed.

[krmahadevan]

Yes, that's a valid point. But right now they are too intertwined and I can de-couple them in a separate PR (and am glad that via this PR, this discrepancy came out)

Could you have a try in another branch? I think it should be quick to inject 2 object instances and call them when needed.

@juherr I thought I could also do that, but it looks like ConfigurationListener gets injected into the TestRunner's config listeners set and that gets exposed to callers outside of TestRunner. So I don't know where all this is going to impact and hence I felt it would be easy for me to just deal with that as a separate PR.

[juherr]

@krmahadevan Please check https://github.com/juherr/testng/tree/bugfix/github-2864 where I made the expected refactoring. It should fix the issue. Could you check and confirm?

The feature you add here is great and just needs to be polished before the merge.

[krmahadevan]

https://github.com/juherr/testng/tree/bugfix/github-2864

@juherr - Thanks for taking the time to create this changeset. It looks good. I ran the tests locally as well and all is well. Can you please have this changeset raised as a PR and lets merge it. I will rebase off of master after the merge and then build upon that

[krmahadevan]

@juherr - I think now this PR is in a shape to be merged. I have built upon your changes and did the same thing of removing out ExitCodeListener and so now, were left with only reporters. So I got rid of all my ordering changes and added a test to ascertain the fix. Please help take a look.

[juherr]

Once ExitCodeListener is called directly, why do we need to add it to the listener lists?

No more problems with IntelliJ listeners?

[krmahadevan]

Once ExitCodeListener is called directly, why do we need to add it to the listener lists?

Because it also implements ITestListener which is needed by TestRunner and I don't want to keep passing around stuff.

No more problems with IntelliJ listeners?

That would still be there because we are now not ordering anything at all. Do you want that to be considered ? If yes, then I would need to bring back the Listener Order determining logic.

[juherr]

I don't want to keep passing around stuff.

But you should because it is part of the internal logic.

That would still be there because we are now not ordering anything at all.

It is ordered by insertion order, right? IntelliJ is supposed to add its own listener at the beginning and everything should work. I'd like a confirmation if I'm right.
Otherwise, we can consider a change based on your previous work where we can apply a custom sort logic for listeners (whitelist packages by default)

[krmahadevan]

But you should because it is part of the internal logic.

This is going to be a much more invasive changeset against the original one. I will make the change.

It is ordered by insertion order, right? IntelliJ is supposed to add its own listener at the beginning and everything should work. I'd like a confirmation if I'm right.

yes we work on insertion order, but if the user's listener got wired in after intellij's UI listener, then it wont know of the changes that the user's listener made to the test state and so, it displays a test as passed even though the user's listener explicitly marked it as failed.

Otherwise, we can consider a change based on your previous work where we can apply a custom sort logic for listeners (whitelist packages by default)

Yes. It looks like a stripped down version of the listener orderer is required to get this done. I will update this PR with those changes.

[krmahadevan]

@juherr - Basic question. How is this round about way of seggregating the listeners (the code has become really more verbose) better than what was being done in this PR earlier (merely seggregating the listeners as inbuilt and foreign) ? I feel that this implementation is shaping up to be more convoluted (atleast in the case of ExitCodeListener object because it gets instantiated in TestNG and needs to make its way into a TestInvoker which means I now have to pass it in so many layers.

What exactly are we gaining by adding this complication ?

[krmahadevan]

@juherr - I have addressed all the review comments. Please check now.

[juherr]

What exactly are we gaining by adding this complication ?

We learned that internal listeners must not be treated like user listeners because they are part of the logic.
Adding them directly is more evident and inscribes it in stone.
We learned that some kinds of listeners must be in a specific order to work well.

The next step will be to give a way for users to specify their own order logic.
We should warn Jetbrains that they should use the new feature if they want to have their listeners at the expected position.

[juherr]

Expect the too-precise name, everything else is great! 👍

[krmahadevan]

The next step will be to give a way for users to specify their own order logic.

Maybe by having them annotated using a custom annotation ?

We should warn Jetbrains that they should use the new feature if they want to have their listeners at the expected position.

By default its taken care of via the JVM argument.

[juherr]

Maybe by having them annotated using a custom annotation ?

Yes, it could be an option.
Before doing that we should provide a way to inject the logic (define an interface and provide the instance implementation when the engine is build).

[krmahadevan]

Before doing that we should provide a way to inject the logic (define an interface and provide the instance implementation when the engine is build).

So you are basically asking for a way to help a user inject how ListenerOrderDeterminer should order listeners right?

• Insertion order (This is what we do today)
• User specified order (Obtain comparator from the user via configuration or as a special listener (similar to annotation transformer) using which we basically order the tests to the user's preference)
• We still will exclude the preferential listeners from this ordering.

I think ListenerOrderDeterminer will be able to easily do this because everywhere the call gets patched through to this implementation. So now it should become easy for us to support ordering.

[juherr]

@krmahadevan yes, that's exactly what I currently have in mind.

[krmahadevan]

@krmahadevan yes, that's exactly what I currently have in mind.

@juherr Just to ensure that we don't forget this, I have created an issue for this. #2874
Please help add any additional context that you think should be considered when building this. I would like to take a jab at this perhaps over the weekend.

[asolntsev]

Hi @krmahadevan
Sorry for giving my feedback too late, but... I don't see that listeners are segregated.

1. When I run test from IDEA, I see IDEATestNgConfigurationListener among my listeners:

2. When I run test from Gradle, I see org.gradle...TestNGTestResultProcessorAdaptor
   

In both cases, they are mixed with my listeners; the order is unpredictable.

So issue #2771 is not solved.

[krmahadevan]

@asolntsev -

When I run test from IDEA, I see IDEATestNgConfigurationListener among my listeners:

The listeners are being segregated into native and user listeners and then ordered such that the native listeners always get added at the end and the user defined listeners come in the front. This way we can ensure that whatever behaviour change was introduced by the user listeners get honoured by the native listeners ( user listener alters test state and that gets honoured by the native listener).
The change was never about excluding the native listeners but just to ensure that they are ordered such that they always get invoked at the end.

When I run test from Gradle, I see org.gradle...TestNGTestResultProcessorAdaptor

For this can you please try setting the JVM argument testng.preferential.listeners.package and see if that ensures that the Gradle listener gets pushed down in the order? You might also want to consider adding a comma separated list of packages that represent such listeners which should be pushed down the order [ For e.g., TestNG considers com.intellij.rt.* as a default value for this property]]

You can find the same in the documentation as well https://testng.org/#_jvm_arguments_in_testng