testng-team / testng

TestNG testing framework
https://testng.org
Apache License 2.0
1.98k stars 1.02k forks source link

Subsequent skipped configurations do not set a throwable #3166

Open aws-sde opened 3 weeks ago

aws-sde commented 3 weeks ago

TestNG Version

7.10.2

Expected behavior

All failed or skipped configuration methods should have a throwable set on their test result.

Actual behavior

Only the first failed or skipped configuration method has a throwable set in its test result. Subsequent configuration methods do get skipped (due to the initial failure/skip), but they do not set the throwable.

This has the consequence of intermittently leaving the throwable null on test results for skipped test methods. When TestInvoker detects a configuration failure, it marks the test method as skipped, and sets its throwable to be the configuration method's throwable. There are two problems with this:

  1. Failed and skipped configurations are stored in a set based on a ConcurrentHashMap, which does not guarantee ordering.
  2. Exception.getExceptionDetails naïvely uses the throwable of the first match for a configuration method's test result.

Is the issue reproducible on runner?

Test case sample

I'm not able to reproduce onTestSkipped showing a null throwable with this, but it absolutely happens in my actual code base. In any case, I think this sufficiently demonstrates that the throwable is null for the other configuration.

package org.example;

import org.testng.SkipException;
import org.testng.annotations.*;

@Listeners(SkipListener.class)
public class AppTest {
    @BeforeClass
    public void firstBeforeClass() {
        throw new SkipException("Skipping firstBeforeClass");
    }

    @BeforeClass(dependsOnMethods = "firstBeforeClass")
    public void secondBeforeClass() {}

    @Test
    public void appHasAGreeting() {}
}
package org.example;

import org.testng.IConfigurationListener;
import org.testng.ITestListener;
import org.testng.ITestResult;

public class SkipListener implements IConfigurationListener, ITestListener {
    @Override
    public void onConfigurationSkip(ITestResult tr) {
        System.out.printf("Configuration %s skipped: %s%n", tr.getMethod().getMethodName(), tr.getThrowable());
    }

    @Override
    public void onTestSkipped(ITestResult result) {
        System.out.printf("Test %s skipped: %s%n", result.getMethod().getMethodName(), result.getThrowable());
    }
}
Gradle suite > Gradle test > org.example.AppTest STANDARD_OUT
    Configuration firstBeforeClass skipped: org.testng.SkipException: Skipping firstBeforeClass
    Configuration secondBeforeClass skipped: null

Gradle suite > Gradle test > org.example.AppTest > appHasAGreeting SKIPPED

Gradle suite > Gradle test > org.example.AppTest STANDARD_OUT
    Test appHasAGreeting skipped: org.testng.SkipException: Skipping firstBeforeClass
krmahadevan commented 3 weeks ago

@aws-sde Since you seem to have spent a good amount of time trying to debug this issue, would you like to raise a pull request to get this fixed? We can help you with the review and merge

aws-sde commented 3 weeks ago

Yes, I'll try to find time to submit a fix hopefully next weekend.