Support class level config failure policy

[krmahadevan]

Closes #2862

Fixes #2862 .

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]

Did you considere to add the new annotation and its behavior in a listener logic?
It will improve the feature coverage of the spi and reduce the complexity of the engine (kind of because supporting a generic behavior is maybe more complex).

[krmahadevan]

Did you considere to add the new annotation and its behavior in a listener logic?

The logic is powered by a marker annotation. I am not quite sure as to how I would be able to move it into a listener. Any samples or pseudo code that you can share to give me a better context around what you are hinting at?

[juherr]

You can have a look at the listener that manages the ignore annotation.
I think you may reproduce a similar thing via IHookable.

[krmahadevan]

@juherr

You can have a look at the listener that manages the ignore annotation.

I have taken a look at the listener and I dont think its going to work in this case because:

• Via listeners we can only alter the state of a method (config/test) but in this PR, we need TestNG to be able to ignore the result of a method execution.
• Dealing with a config execution result and then deciding what to do with config failures and config skips, in my opinion is part of the core engine and I am not sure how we can refactor it so that it becomes plug and play.
• I dont want to expose more internals of TestNG so that we can do this via listeners.

I think you may reproduce a similar thing via IHookable.

No we cannot do that. Via IHookable, we can either have a config executed or maybe deal with the exceptions that it raised or change its result. In this PR, we are not doing any of this, but having TestNG ignore failures at a configuration method and have it move forward.

[krmahadevan]

ping @juherr

[juherr]

What do you think to add a failurePolicy param on configuration annotations?
And then @DisregardConfigFailure will just configure the param.

[juherr]

U mean you want this to be added as an attribute to the annotations instead of having it as a separate annotation on its own ?

What is the advantage of having failurePolicy as an attribute vs it being a completely standalone annotation on its own? The current annotations are configurable at runtime via annotation transformers and this behavior is NOT meant to be configurable via transformers because that kind of defeats the entire purpose (we have the suite attribute for that) and we would like this to be hard wired into the java class which can ONLY be reverted via a code change (similar to what @ignore does today). Please let me know your thoughts on that @juherr

@krmahadevan
As I understand you want to configure the behavior of the configuration methods.
It means, from a design point of view, that it should be hosted on the configuration method model.
The way the user describes it is another subject: it could be a new annotation, a new attribute, or both.
That is currently more a feeling than a choice. Let me know if I need to think more about it and explain.

[krmahadevan]

@juherr

As I understand you want to configure the behavior of the configuration methods.

Perfectly aligned with you on this.

It means, from a design point of view, that it should be hosted on the configuration method model.

Not necessarily true given the fact that the existing model is NOT tied to the annotations at all but is more aligned at the overall test execution.

The way the user describes it is another subject: it could be a new annotation, a new attribute, or both.
That is currently more a feeling than a choice. Let me know if I need to think more about it and explain.

This is where I am trying to get more context in terms of what is the issue with the current implementation of using an annotation driven model to express that intent, given the fact that

• the current option available to the user is NOT driven by the annotation.
• Is global in nature and so I would like the new approach to be immutable and completely local in nature.

The reason why I am pursuing a new annotation vs a new attribute in the existing annotation is because:

• I would like this behaviour to be immutable
• I don't want to build custom logic that would ensure that any of the current TestNG listeners don't see this behaviour (putting this feature as an attribute in the existing annotations is going to expose it to the TestNG listeners)

[juherr]

The engine and its model are not coupled to annotations but the naming of the model refers to annotation for legacy reason.
Annotations are the way the users describe the meta-model which is then converted into the engine model.

You can have both a new annotation for userland and an attribute in the domain for the engine.
The engine must not deal with introspection to look for description because it was already done before.

[krmahadevan]

The engine and its model are not coupled to annotations but the naming of the model refers to annotation for legacy reason. Annotations are the way the users describe the meta-model which is then converted into the engine model.

You can have both a new annotation for userland and an attribute in the domain for the engine. The engine must not deal with introspection to look for description because it was already done before.

I didn't understand what you are suggesting. Can you please explain a bit more since I am not able to grasp what you are expecting here.

When you say engine, what are you referring to? I am not quite sure I understand this part, especially when TestNG doesn't seem to have such distinct components. I would like this feature to be a purely annotation driven approach which cannot/should not be toggled just as how we have @Ignore.

[juherr]

As you know, TestNG has many phases:

1. Build the suite model (from annotation and/or suite file)
2. Update the suite model
3. Select tests from the model
4. Run the model by the suite/test engine
5. Report the result

The run phase has sub-phases but that is not the subject here.

As I understand the fix, it needs to be applied in phase 4 during the run.
At the same time, you propose to configure it by annotations which are managed in phase 1.
If phase 4 manage directly an annotation (what is technically possible), it breaks the architecture and will produce debt.
@Ignore feature is plugged into the phase 2 and just updates the model.

In other words, the fix is ok except the org.testng.annotations.DisregardConfigFailure lookup into ConfigInvoker

[krmahadevan]

@juherr - Thank you for explaining this. Now I understand the concern. Let me see what I can come up with that adheres the model that you called out.

[krmahadevan]

@juherr - I have re-worked on this PR to adhere to the expectations you had called out earlier. Can you please help take a look ?

[krmahadevan]

@juherr - Gentle reminder on this.

[juherr]

The change is nice: good job! 👍

[krmahadevan]

The change is nice: good job! 👍

And I learnt about the layered model that you explained while working on this. That was a new learning