Ability to retry skipped tests in TestNG

itkhanz commented 3 weeks ago

Hi, I am using Gradle build tool and test-retry-gradle-plugin to run my test suite using TestNG. TestNG provides the option to explicitly mark the test as skipped during runtime via throwing a SkipException

throw new SkipException(exceptionMessage);

This is particularly helpful when we do not want to continue test execution after a certain condition occurs while running the test case, so we throw this exception, which will mark this test as skipped and is then appropriately shown as skipped in test results.

I would like to be able to retry tests that are marked as skipped. Having an ability to retry these skipped tests will be particulary useful to help dealing with flaky tests. Could you please add this feature as enhancement to retry-plugin so we can also rerun tests that are marked as skipped? Currently at the moment, this plugin aparently only supports to rerun tests that are marked as failed.

pshevche commented 3 weeks ago

@itkhanz , thank you for bringing this up. I don't think that adding an option to retry skipped tests is the best solution here. Especially because such a flag would apply to all skipped tests, and in most cases there is no point in re-running a skipped test because the condition to abort it wouldn't change between runs.

Is there a reason why you don't mark those tests as failed? It sounds like the subject you test is in an unexpected state, so shouldn't a test fail instead of being skipped?

It also seems like your tests depend on each other. Do I understand it correctly that one of the tests would fail and all consecutive tests will be marked as skipped? In this case, you can explore the following option. Keep everything as-is now: fail one test, mark the rest as skipped, and retry the entire test class using class-retry filters.

itkhanz commented 3 weeks ago

Hi, Yes there is a reason as to why I would like to have this ability to rerun tests that are skipped. The reason is, I have configuration/hook methods like @BeforeMethod in a BaseTest for common setup that always gets executed before the actual test starts, so the actual test class inherits BaseTest. If for some reason, there is a failure in this configuration methods annotated with @BeforeMethod or @BeforeClass in a parent class, then Gradle/TestNG marks the actual test as skipped and separately marks the configuration method as failed. This causes noise in test results, where I am not really interested in the status of configuration method, and instead I want the result/status of my actual test to be shown only as skipped or failed. Here is an example to reproduce this issue:

import lombok.extern.slf4j.Slf4j;
import org.testng.SkipException;
import org.testng.annotations.BeforeMethod;

@Slf4j
public class ParentTest {
    @BeforeMethod(alwaysRun = true)
    public void test_setup() {
      log.info("Configuration BeforeMethod of ParentTest");
       throw new SkipException("Configuration BeforeMethod of ParentTest SKIPPED");
       //Assert.fail("Configuration BeforeMethod of ParentTest FAILED");
    }
}

------------------------------------------------------------------------------------------------

import lombok.extern.slf4j.Slf4j;
import org.testng.annotations.Test;

@Slf4j
public class DebugTest extends ParentTest {
    @Test
    public void test_debug() {
      log.info("I am the actual test");
    }
}

So in this case, if I mark the configuration method to be skipped because of some condition, then the console results are correctly shown as:

DCS Appium Automation Suite > App tests > com.dcs.appium.testing.tests.DebugTest STANDARD_OUT
    2024-06-25 08:35:52.243 [INFO] ParentTest.test_setup:12 - Configuration BeforeMethod of ParentTest

DCS Appium Automation Suite > App tests > com.dcs.appium.testing.tests.DebugTest > test_debug STARTED
  DCS Appium Automation Suite > App tests > com.dcs.appium.testing.tests.DebugTest - test_debug

DCS Appium Automation Suite > App tests > com.dcs.appium.testing.tests.DebugTest > test_debug SKIPPED

DCS Appium Automation Suite > App tests > com.dcs.appium.testing.tests.DebugTest STANDARD_OUT
    2024-06-25 08:35:52.244 [ERROR] TestListener.onTestSkipped:48 - org.testng.SkipException: Configuration BeforeMethod of ParentTest SKIPPED

  0 passing (639ms)
  1 pending

---------------------------------------------------------------
|  Results: SUCCESS (1 tests, 0 passed, 0 failed, 1 skipped)  |
---------------------------------------------------------------

However, now lets say that instead of throwing SkipException, I fail the configuration @BeforeMethod by replacing the statement of SkipException with Assert.fail("Configuration BeforeMethod of ParentTest FAILED"); then this would result in the duplicate tests in console, with configuration method markes as failed, while the actual test is marked as skipped. As far as I know, this is by design in TestNG where any failure if occurs in configuration methods, causes the actual tests to be marked as skipped. Here is how the results will then be printed in console. See the logs below:-

DCS Appium Automation Suite > App tests > com.dcs.appium.testing.tests.DebugTest STANDARD_OUT
    2024-06-25 08:50:37.332 [INFO] ParentTest.test_setup:12 - Configuration BeforeMethod of ParentTest

DCS Appium Automation Suite > App tests > com.dcs.appium.testing.tests.DebugTest > test_setup STARTED
  DCS Appium Automation Suite > App tests > com.dcs.appium.testing.tests.DebugTest ✘ test_setup

DCS Appium Automation Suite > App tests > com.dcs.appium.testing.tests.DebugTest > test_setup FAILED
    java.lang.AssertionError: Configuration BeforeMethod of ParentTest FAILED
        at org.testng.Assert.fail(Assert.java:111)
        at com.dcs.appium.testing.tests.ParentTest.test_setup(ParentTest.java:13)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:568)
        at org.testng.internal.invokers.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:141)
        at org.testng.internal.invokers.MethodInvocationHelper.invokeMethodConsideringTimeout(MethodInvocationHelper.java:71)
        at org.testng.internal.invokers.ConfigInvoker.invokeConfigurationMethod(ConfigInvoker.java:400)
        at org.testng.internal.invokers.ConfigInvoker.invokeConfigurations(ConfigInvoker.java:333)
        at org.testng.internal.invokers.TestInvoker.runConfigMethods(TestInvoker.java:833)
        at org.testng.internal.invokers.TestInvoker.invokeMethod(TestInvoker.java:600)
        at org.testng.internal.invokers.TestInvoker.invokeTestMethod(TestInvoker.java:230)
        at org.testng.internal.invokers.MethodRunner.runInSequence(MethodRunner.java:63)
        at org.testng.internal.invokers.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:992)
        at org.testng.internal.invokers.TestInvoker.invokeTestMethods(TestInvoker.java:203)
        at org.testng.internal.invokers.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:154)
        at org.testng.internal.invokers.TestMethodWorker.run(TestMethodWorker.java:134)
        at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
        at org.testng.TestRunner.privateRun(TestRunner.java:739)
        at org.testng.TestRunner.run(TestRunner.java:614)
        at org.testng.SuiteRunner.runTest(SuiteRunner.java:421)
        at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:413)
        at org.testng.SuiteRunner.privateRun(SuiteRunner.java:373)
        at org.testng.SuiteRunner.run(SuiteRunner.java:312)
        at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
        at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:95)
        at org.testng.TestNG.runSuitesSequentially(TestNG.java:1274)
        at org.testng.TestNG.runSuitesLocally(TestNG.java:1208)
        at org.testng.TestNG.runSuites(TestNG.java:1112)
        at org.testng.TestNG.run(TestNG.java:1079)
        at org.gradle.api.internal.tasks.testing.testng.TestNGTestClassProcessor.runTests(TestNGTestClassProcessor.java:153)
        at org.gradle.api.internal.tasks.testing.testng.TestNGTestClassProcessor.stop(TestNGTestClassProcessor.java:96)
        at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.stop(SuiteTestClassProcessor.java:62)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:568)
        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 jdk.proxy1/jdk.proxy1.$Proxy2.stop(Unknown Source)
        at org.gradle.api.internal.tasks.testing.worker.TestWorker$3.run(TestWorker.java:193)
        at org.gradle.api.internal.tasks.testing.worker.TestWorker.executeAndMaintainThreadName(TestWorker.java:129)
        at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:100)
        at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:60)
        at org.gradle.process.internal.worker.child.ActionExecutionWorker.execute(ActionExecutionWorker.java:56)
        at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:119)
        at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:66)
        at worker.org.gradle.process.internal.worker.GradleWorkerMain.run(GradleWorkerMain.java:69)
        at worker.org.gradle.process.internal.worker.GradleWorkerMain.main(GradleWorkerMain.java:74)

DCS Appium Automation Suite > App tests > com.dcs.appium.testing.tests.DebugTest > test_debug STARTED
  DCS Appium Automation Suite > App tests > com.dcs.appium.testing.tests.DebugTest - test_debug

DCS Appium Automation Suite > App tests > com.dcs.appium.testing.tests.DebugTest > test_debug SKIPPED

DCS Appium Automation Suite > App tests > com.dcs.appium.testing.tests.DebugTest STANDARD_OUT
    2024-06-25 08:50:37.334 [ERROR] TestListener.onTestSkipped:48 - java.lang.AssertionError: Configuration BeforeMethod of ParentTest FAILED

  0 passing (651ms)
  1 pending
  1 failing

---------------------------------------------------------------
|  Results: FAILURE (2 tests, 0 passed, 1 failed, 1 skipped)  |
---------------------------------------------------------------

2 tests completed, 1 failed, 1 skipped

FAILURE: Build failed with an exception.

As you can see in this scenario, the actual test test_debug is shown as skipped while the configuration method test_setup is shown separately as failed.

Intrestingly, I realized now that the gradle-retry plugin successfully retries the actual test method that is marked as skipped, which previously I thought would not be retried as it is not marked as failed but skipped. But the reason for having the ability to retry tests that are marked as skipped is because of the duplicate result counts. As you can see from the below logs that if a test fails on retry again, then the result count shows 4 tests instead of 2.


---------------------------------------------------------------
|  Results: FAILURE (4 tests, 0 passed, 2 failed, 2 skipped)  |
---------------------------------------------------------------

4 tests completed, 2 failed, 2 skipped

FAILURE: Build failed with an exception.

This would not be a problem if i instead make the configuration methods Skipped, and then i get the correct test results count in terminal and if gradle-retry-plugin will allow me to retry those skipped tests, then this will solve this issue. Unfortunately, gradle-retry-plugin does not allow me to retry the tests that are marked as skipped, and hence I am currently resorting to failing the condiguration methods if something goes unexpected in them.

I understand that this results count duplication may be due to the design implementation of TestNG or Gradle, but if i am able to retry the tests by marking them as skipped, then i will get the correct results count as well as I can then retry them to resolve test flakiness.

I hope i was able to explain the context.

pshevche commented 3 weeks ago

@itkhanz , thank you for explaining this. I see a couple of issues with this setup, and I'll try to provide a recommendation.

How do you detect that something is wrong if you skip failures in @BeforeMethod methods? As we see in your reproducer, the test task succeeded because tests were marked as skipped. This sounds alarming, as it hides issues in tests or production code. For observability reasons, it makes more sense to me to fail the method and deal with flakiness.
I don't see an easy way to differentiate between tests skipped on failures (which would be retried) and tests skipped because of @Ignore annotation, for example.

I see how the reporting is confusing, but I still don't think that you should hide test failures behind SkipException. This has the potential of leaking actual issues to production. If you don't do that, there is also no reason to retry skipped tests. And as you correctly pointed out, the retry plugin can adequately deal with this scenario.

To have better reporting, I'd recommend giving https://github.com/junit-team/testng-engine and Gradle Build Scan a try. Build Scan offers a much richer reporting of test executions and retries (which are not supported by the default Gradle report).

For example, with default TestNG runner, a flaky setup will be reported like this (scan):

If using the JUnit runner, a flaky setup will be attributed to the test class like this (scan):

This should alleviate your concern about wrong test counts in the console log.

pshevche commented 3 weeks ago

To sum it up, I think you should fail the setup/cleanup methods if something goes wrong and let the retry plugin handle those failures. Secondly, you could investigate other reporting tools that Gradle provides for a better visualization of retries.

itkhanz commented 3 weeks ago

To sum it up, I think you should fail the setup/cleanup methods if something goes wrong and let the retry plugin handle those failures. Secondly, you could investigate other reporting tools that Gradle provides for a better visualization of retries.

Thank you for quite deep and helpful response. I agree its, better to not hide these failures of configuration method and let them fail. For dealing with test results count, i will give a try to JUnit engine for TestNG, and see how it integrates with my existing setup as I am using TestNG quite heavily so there may be some restrictions.

pshevche commented 3 weeks ago

To sum it up, I think you should fail the setup/cleanup methods if something goes wrong and let the retry plugin handle those failures. Secondly, you could investigate other reporting tools that Gradle provides for a better visualization of retries.

Thank you for quite deep and helpful response. I agree its, better to not hide these failures of configuration method and let them fail. For dealing with test results count, i will give a try to JUnit engine for TestNG, and see how it integrates with my existing setup as I am using TestNG quite heavily so there may be some restrictions.

Sounds good. I'll close this issue for now. Feel free to re-open if we can help you out in any other way.

gradle / test-retry-gradle-plugin

Ability to retry skipped tests in TestNG #290