testng
testng copied to clipboard
testng-team
Failing reports should result in test failure
TestNG Version
The latest
Expected behavior
Failing reports should result in test execution failure. An alternative option is to have a property to "ignore errors from reporters"
Actual behavior
TestNG ignores the error and pretends everything works as usual.
Here's a sample test failure that looks like a passing test case. I guess there's an error in the test, however, TestNG ignores the error, so it hides the bug:
TestNG > Listeners > test.multiplelisteners.TestMaker > run STANDARD_ERROR
[TestNG] Reporter test.multiplelisteners.SimpleReporter@56865f38 failed
java.lang.RuntimeException: java.lang.NoSuchFieldException: m_configuration
at test.multiplelisteners.SimpleReporter.generateReport(SimpleReporter.java:31)
at org.testng.TestNG.generateReports(TestNG.java:1101)
at org.testng.TestNG.run(TestNG.java:1044)
at test.multiplelisteners.TestMaker.run(TestMaker.java:26)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.testng.internal.invokers.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:136)
at org.testng.internal.invokers.TestInvoker.invokeMethod(TestInvoker.java:593)
at org.testng.internal.invokers.TestInvoker.invokeTestMethod(TestInvoker.java:175)
at org.testng.internal.invokers.MethodRunner.runInSequence(MethodRunner.java:49)
at org.testng.internal.invokers.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:826)
at org.testng.internal.invokers.TestInvoker.invokeTestMethods(TestInvoker.java:148)
at org.testng.internal.invokers.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:147)
at org.testng.internal.invokers.TestMethodWorker.run(TestMethodWorker.java:129)
at java.util.ArrayList.forEach(ArrayList.java:1249)
at org.testng.TestRunner.privateRun(TestRunner.java:794)
at org.testng.TestRunner.run(TestRunner.java:596)
at org.testng.SuiteRunner.runTest(SuiteRunner.java:420)
at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:414)
at org.testng.SuiteRunner.privateRun(SuiteRunner.java:375)
at org.testng.SuiteRunner.run(SuiteRunner.java:319)
at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:53)
at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:96)
at org.testng.TestNG.runSuitesSequentially(TestNG.java:1220)
at org.testng.TestNG.runSuitesLocally(TestNG.java:1142)
at org.testng.TestNG.runSuites(TestNG.java:1071)
at org.testng.TestNG.run(TestNG.java:1039)
at org.gradle.api.internal.tasks.testing.testng.TestNGTestClassProcessor.runTests(TestNGTestClassProcessor.java:141)
at org.gradle.api.internal.tasks.testing.testng.TestNGTestClassProcessor.stop(TestNGTestClassProcessor.java:90)
at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.stop(SuiteTestClassProcessor.java:61)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
at org.gradle.internal.dispatch.ContextClassLoaderDispatch.dispatch(ContextClassLoaderDispatch.java:33)
at org.gradle.internal.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:94)
at com.sun.proxy.$Proxy2.stop(Unknown Source)
at org.gradle.api.internal.tasks.testing.worker.TestWorker.stop(TestWorker.java:133)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
at org.gradle.internal.remote.internal.hub.MessageHubBackedObjectConnection$DispatchWrapper.dispatch(MessageHubBackedObjectConnection.java:182)
at org.gradle.internal.remote.internal.hub.MessageHubBackedObjectConnection$DispatchWrapper.dispatch(MessageHubBackedObjectConnection.java:164)
at org.gradle.internal.remote.internal.hub.MessageHub$Handler.run(MessageHub.java:414)
at org.gradle.internal.concurrent.ExecutorPolicy$CatchAndRecordFailures.onExecute(ExecutorPolicy.java:64)
at org.gradle.internal.concurrent.ManagedExecutorImpl$1.run(ManagedExecutorImpl.java:48)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
at org.gradle.internal.concurrent.ThreadFactoryImpl$ManagedThreadRunnable.run(ThreadFactoryImpl.java:56)
at java.lang.Thread.run(Thread.java:748)
Caused by: java.lang.NoSuchFieldException: m_configuration
at java.lang.Class.getDeclaredField(Class.java:2070)
at test.multiplelisteners.SimpleReporter.generateReport(SimpleReporter.java:24)
... 56 more
Just in case, the field SuiteRunner#m_configuration was renamed to configuration in 878c315716d6e7a75082a76e7b0f2ce6b7b019b8 in 2016, and I guess it resulted in an unnoticed test failure :(
@vlsi - I am not sure I am convinced that errors in reports should trigger test failures.
Lets consider the below scenario.
I have included lets say 5 reporting options in my suite and my tests (lets say there's a large set of tests) run to completion successfully. Now just because a reporter threw an exception, it would be unfair on the user to have their execution marked as failure. This becomes even more true/relevant when the reporting mechanism being consumed by a user is built by someone else.
So IMO, this should not be the case.
@juherr - WDYT ?
This becomes even more true/relevant when the reporting mechanism being consumed by a user is built by someone else.
It can be represented as "configuration method failure" or something like that. In other words, "failing report" might be represented similar to a regular test failure.
The current behavior is "pretend nothing happened" which is confusing.
Apparently, we do not want to ignore unexpected errors, especially when we verify TestNG quality.
Now just because a reporter threw an exception, it would be unfair on the user to have their execution marked as failure
That depends on the nature of the failure. If the reporter missed important data, then it might indeed be a true bug detected in the test run.
I agree there might be a case for "ignore reporting errors", however, I would appreciate "surface errors by default" rather than "hide errors by default".
I understand there's backward compatibility, and changing defaults is sad.
So the way out here seems to introduce a property to control the behavior and flip the default in 8.0 or so. Then TestNG test suite could run with strict mode, so the reporting failures could be detected in CI.
That depends on the nature of the failure. If the reporter missed important data, then it might indeed be a true bug detected in the test run.
I believe reports are not part of tests and failures in them should be reported to the user, but never break test executions and causing hardships to the users.
however, I would appreciate "surface errors by default" rather than "hide errors by default".
I am fine with adding a configuration parameter, which would be turned OFF by default and which can be turned ON if needed (not vice versa because there's no easy way of conveying this behavior to the user)
Oh, I see your point. Please feel free to close the issue or keep it open then.
If the flag is not going to be the default in the future, I am not interested in implementing it.
I believe reports are not part of tests and failures in them should be reported to the user, but never break test executions and causing hardships to the users.
@krmahadevan I disagree: every failure which is not in a test should crash the run because it is not expected. Otherwise, users will have false-positive results and will miss test failure as we did for many years.
For sure, the behavior change should be configurable, and first, the default value could be the current behavior. Didn't check but in every case, the return code must highlight the error (even if I think nobody is checking the value).
@juherr
every failure which is not in a test should crash the run because it is not expected.
There are two ways of looking at it. Consider this scenario. I have 5 types of reporters wired in (a few of them are 3rd party reporters and may have some bug in them that is yet to be fixed). Now my execution is exposing that defect in one of those reporters, but other reporters are working fine.
Are you suggesting that just because one of the reporters had a problem in it, we need to invalidate an execution (which may have run for hours together and user would still have gotten the test results via the other reporters that were part of the same execution) ?
To me that is bad experience. Reports are just ways to report the execution results. Just because that reporting failed doesnt mean that my execution itself failed.
When a reporter fails, here's how I read it as : TestNG executed all the tests and they all passed, but I failed to report the results in a proper way because of some problem
the return code must highlight the error (even if I think nobody is checking the value).
I think surefire plugin checks this no, and based on that only they report pass/fail no ?
To me that is bad experience. Reports are just ways to report the execution results. Just because that reporting failed doesnt mean that my execution itself failed.
I understand your point of view but users have many workarounds like removing the failing reporter from the execution.
Are you suggesting that just because one of the reporters had a problem in it, we need to invalidate an execution (which may have run for hours together and user would still have gotten the test results via the other reporters that were part of the same execution) ?
Right, maybe we can store all reporters exceptions and throw them at the end of the run. Could be a better experience. But I prefer a crash instead of a false-positive (and I suppose QA managers too :p)
I think surefire plugin checks this no, and based on that only they report pass/fail no ?
No idea. They should :)
I understand your point of view but users have many workarounds like removing the failing reporter from the execution.
But that would be only after an execution would have failed right ?
Right, maybe we can store all reporters exceptions and throw them at the end of the run. Could be a better experience.
What if we just threw out warnings at the end, if we encountered problems in reports ? That way, the user would get to know that there were problems in 1 or more reporters, and the execution would also have run to completion.
and I suppose QA managers too
Not when it involves a nightly run that ran for a few hours only to learn in the morning that the execution failed because a reporter conked.
Not when it involves a nightly run that ran for a few hours only to learn in the morning that the execution failed because a reporter conked
Shit happens :D
What if we just threw out warnings at the end, if we encountered problems in reports ? That way, the user would get to know that there were problems in 1 or more reporters, and the execution would also have run to completion.
We can start with error messages (it is more than warnings because something really failed in the execution). The problem with error messages is that nobody reads them, so they will never open issues and bugs won't be fixed. That's why I'd prefer a real failure of the run next, which can be fixed by a configuration. Then, if users prefer to change the default behavior, it's their responsibility.