IHookable and IConfigurable callback discrepancy

[krmahadevan]

Closes #2704

Ensure that TestNG reports scenarios wherein
User defines callbacks for configurations/test methods
But fails to invoke those callbacks explicitly
And also fails in changing test status to a user
recognized status (PASS|FAILURE|SKIP)

Fixes #2704 .

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]

Really cool changes, good job! 👍

[krmahadevan]

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

[krmahadevan]

@juherr - Fixed all review comments. Please check.

[krmahadevan]

@juherr - Fixed and responded to your questions. Can you please help take a look

[juherr]

Except the exception flow which looks a bit complex to follow, LGTM

[lprimak]

This change broke Arquillian, see arquillian/arquillian-core#427

[lprimak]

In the future, these kinds of changes should bump up the major version.
This is not the only change that warrants a bump in major version (ex: JDK 11 requirement,)

[krmahadevan]

In the future, these kinds of changes should bump up the major version.

This was a bug fix and the behaviour was being streamlined. Not sure why this would need a major version.

This is not the only change that warrants a bump in major version (ex: JDK 11 requirement,)

Feedback taken. Will ensure that when we change these sort of pre requisites we will do it as a major version and not as a minor version. Thank U for calling this out.

[juherr]

@krmahadevan Maybe we can make it configurable?

@lprimak Could you explain what is the use case which is failing with this fix? Even if we provide a workaround, don't you think there is an issue in the test suite?

[krmahadevan]

@krmahadevan Maybe we can make it configurable?

Sure we can. But I would like to understand why do we need to do that, given the fact that this is undocumented behaviour and we were ensuring that we fixed the discrepancy.

[lprimak]

@lprimak Could you explain what is the use case which is failing with this fix? Even if we provide a workaround, don't you think there is an issue in the test suite?

See my original comment #2713 (comment)

The issue isn't that this PR is wrong (it's ok) but the behavior breakage is such that it should not have gone into a minor version upgrade.
I think Arquillian can be fixed to reflect the new (and better) behavior but again this isn't for a minor version bump in the future, that's all, since this is a breaking change

[juherr]

@lprimak In fact, we are not considering that a change of behavior is critical when it is not documented or it doesn't break our own test suite.
For sure, I can understand you disagree with that.

About the current subject, thanks to your feedback, we will provide a new key to configure this behavior.