Update version of TestNG to the latest
Opened this issue · 9 comments
We are currently using 6.9.10 and the latest is 6.9.12. One fix this addresses is that SeLionAssert verify failures show as passed in the emailable-report instead of failed due to a break in 6.9.X at some point. Setting the test result in TestNG's Reporter class so that is shows it in the emailable report is fixed in 6.9.11
this is fixed in develop and develop-1.2.0
@ILikeToNguyen - fyi - I'm re-opening this for follow-up. It looks like the change breaks SeLion's soft assert and session sharing. If you run the default "client" test suite there will be failures. This needs more investigation. Hopefully, we can fix this and don't need to revert to the older version of TestNG.
Reason why our existing code doesn't throw an AssertionException for our softasserts with a bump from TestNG 6.9.10:
testng-team/testng@8fba949#diff-b3080bc9d91f177f63b5fd4d9c501364R1326
That line changed after 6.9.10.
Before it only checked that a throwable was set which we did in SeLionSoftAssert.assertAll(). Since the code when in that if statement then, the test case was treated as a failure and testng framework threw that throwable exception.
However, after the change, it is checking for status in addition to throwable. You would think that status would be set to failed because in SeLionSoftAssert.assertAll(), we also set the status to failure on the ITestResult object. But TestNG overwrites this value based on other logic instead of checking if it got set to anyting but success.
How? You will see here is where a test method is invoked here
https://github.com/cbeust/testng/blob/6.9.13/src/main/java/org/testng/internal/Invoker.java#L646
In this line, we have set the testresult's m_status to fail and the testresult's m_throwable to the assertionexecption through the Reporter holding a reference to ITestResult object.
Testng doesnt read the status value and sets it to success and uses its own logic to determing if the test result failed by setting it to failed in a catch on line 661. Therefore the only way a test can fail is if it throws an exception. (I feel that this is incorrect behavior, I think it should read testResult's status value first before setting it to SUCCESS. I will put a PR into TestNG and see what they think, if they intended to give users the ability to set the test results via Reporter.getCurrentTestResult().
Perfecto has also made the assumption that you can dynamically set the testResults this way.
https://community.perfectomobile.com/series/26523-testng/posts/1096829-testng-changing-test-results-dynamically
So we eventually get to handleInvocationResults which now has the status set incorrectly on m_status
https://github.com/cbeust/testng/blob/6.9.13/src/main/java/org/testng/internal/Invoker.java#L684
Thus that modified if check will now fail since the status is set falsely to SUCCESS.
So the solution is that in assertAll, we can't just set Reporter's ITestResult to FAILED, we must throw that exception out and let the framework set the result to failed and catch the exception and set the m_throwable for us.
Here is the Pull Request to TestNG to have it fixed at the source instead of trying to work around it.
Throwing an exception instead of setting the testResults works in the simple case of SeLionSoftAsserts when we call assertAll() directly in the test. However, if we use SeLionAsserts.verify and rely on the listener to call assertAll() for the user, that doesnt work with us throwing an exception. Has to do with throwing the exception from a Listener in an afterInvocation.
@ILikeToNguyen thanks for the update. looking forward to the TestNG version with this fix. :)
I am seeing another issue with RuntimeReporter where same test is reported twice in final report. Seems to be happening due to this bug in testng. I tested locally on my fork with TestNG v6.10 and it seems to be working fine. I am not sure if I need to open another issue for RuntimeReporter since it is related to TestNG version upgrade?
@vikram1711 Confirming your find -- the current 2.0.0-SNAPSHOT version of SeLion is using TestNG 6.9.10. I have not seen the same test reported twice in the final report. Is this the version combinations you were using where you observed the behavior? Furthermore, you saw it go away with SeLion 2.0.0-SNAPSHOT and TestNG 6.10?
Ok, so I tried both Maven and TestNG eclipse plugin with v6.9.10. Maven run is working fine and I don't see any duplicates in report but when I am running the suite using eclipse plugin, I am seeing this issue.
Issue can be reproduced-
2.0.0-SNAPSHOT and TestNG v6.9.10
Issue can't be reproduced-
2.0.0-SNAPSHOT and TestNG v6.10
@vikram1711 Thanks for the additional details!