Updated Status in afterInvocation() not considered

[krmahadevan]

Closes #1600

Fixes #1600 .

Did you remember to?

• Add test case(s)
• Update CHANGES.txt

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.

[juherr]

I'm just thinking a TestNG#addListeners(ITestNGListener... listeners) could be awesome.

[krmahadevan]

Its use would be limited to just us for writing our unit tests, and to those that use the TestNG APIs directly (perhaps the IDE plugins and the build tool plugins). I dont see much of a use for this in other places. But yes, I agree, it would be simple change.

[juherr]

I don't understand well. What are the value of holder.status and testResult.getStatus() before and after handleInvocationResults, and before/after your fix?

Maybe testResult.getStatus() == holder.status can replace wasResultUnaltered?

[krmahadevan]

No that wouldn't work. holder.status represents the computed status by us, taking into account the expectedExceptions attribute of an @Test annotation. So for an e.g., if an @Test method threw a org.testng.SkipException then its status would be FAILURE (because it threw an exception) but our computed status for holder.status would be SKIP (since we have a special meaning for SkipException).
So if we merely stuck to evaluating testResult.getStatus() == holder.status we end up using testResult.getStatus() which is incorrect.

The testResult.getStatus() value should be considered only if it was altered by the user defined IInvokedMethodListener implementations (which is what #1600 is asking for).

In all other cases, we should only consider holder.status

[xlenz]

Since 6.13 throw new SkipException("Skipping the test case"); won't work. I have to set status before throwing exception:

iTestResult.setStatus(TestResult.SKIP);
throw new SkipException("Skipping the test case");

Just want to make sure :)

[juherr]

It doesn't shock me in the listener context and I think it is a better practice.
BTW, it is a regression, so can you open a new issue?

[xlenz]

@juherr It is a regression, but I'm not sure if it is a planned change or not.
Let me clarify...
previously I had in beforeInvocation:
throw new SkipException("Skipped due to previous failure of: " + failedTestName);
So I have TestResult.SKIP in afterInvocation.

After upgrading to 6.13 it stopped working, because iTestResult.getStatus() is now TestResult.FAILURE instead of TestResult.SKIP in afterInvocation.

[juherr]

Regressions are never expected :)

And TestNG should be able to catch the skip exception and set the status accordingly.

A new issue is a better location for the discussion and we will be able to close it if we want ;)