testng-team / testng

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

TestNG softAssert in @AfterMethod makes all tests skip #1038

Closed rouke-broersma closed 8 years ago

rouke-broersma commented 8 years ago

I use a SoftAssert that I throw after my test method, and I use a dataProvider in my test methods running parallel through maven failsafe, it's running parallel per method. It doesn't matter if I use parallel=true on my dataprovider or not. Whenever an assert is thrown using my aftermethod, all tests that start in that thread fail immediately because of the dataProvider.

DataProvider:

@DataProvider(name = "environment", parallel = true)
public Object[][] getEnvironments() { return propertiesHelper.getBrowsers() ; }

The dataprovider returns a list of enums, and not null. TestNG also sees no errors in the dataprovider (confirmed through debugging)

Aftermethod:

@AfterMethod(alwaysRun = true)
public void throwSoftAssert() {

    if(SoftAssertThreadLocal.get() == softAssert && softAssert != null) {
            softAssert.assertAll();
    }
}

This is what I see in the log, as can be seen from when my assert fails every test in the thread [pool-2-thread-3] is started and stopped within 1ms, it is then shown as skipped in the test report.

13:23:58.318 [pool-2-thread-3] FATAL nl.adaption.it.selenium_test.util.assertion.SoftAssert - Is seaFreight revenue created? copying D:\Jenkins\workspace\Selenium\workspace\selenium test runner\target\screenshot_Is_seaFreight_revenue_created__1462361038604.png 13:23:58.612 [pool-2-thread-3] INFO nl.adaption.it.selenium_test.tests.dossier.financial.revenue.ITgenerateVesselRevenue - ### STOPPING TEST: whenGeneratingVesselRevenue_RevenuesShouldBeCreated ### 13:23:59.565 [pool-2-thread-3] INFO nl.adaption.it.selenium_test.tests.contract.ITtransportContract - ### STARTING TEST: whenCreatingValidTransportContractWithAgreementAndScenario_ActivitiesShouldBeAutomaticallyCreated ### 13:23:59.566 [pool-2-thread-3] INFO nl.adaption.it.selenium_test.tests.contract.ITtransportContract - ### STOPPING TEST: whenCreatingValidTransportContractWithAgreementAndScenario_ActivitiesShouldBeAutomaticallyCreated ### 13:23:59.569 [pool-2-thread-3] INFO nl.adaption.it.selenium_test.tests.relation.ITuploadRelationLogo - ### STARTING TEST: whenUploadingNewRelationLogo_SpinnerShouldAutomaticallyClose ### 13:23:59.569 [pool-2-thread-3] INFO nl.adaption.it.selenium_test.tests.relation.ITuploadRelationLogo - ### STOPPING TEST: whenUploadingNewRelationLogo_SpinnerShouldAutomaticallyClose ### 13:23:59.571 [pool-2-thread-3] INFO nl.adaption.it.selenium_test.tests.app_currency.ITcreateAppCurrency - ### STARTING TEST: whenCreatingAppCurrencyWithValidData_AndClickingSaveAndClose_AppCurrenciesPageShouldBecomeVisible ### 13:23:59.571 [pool-2-thread-3] INFO nl.adaption.it.selenium_test.tests.app_currency.ITcreateAppCurrency - ### STOPPING TEST: whenCreatingAppCurrencyWithValidData_AndClickingSaveAndClose_AppCurrenciesPageShouldBecomeVisible ### 13:23:59.573 [pool-2-thread-3] INFO nl.adaption.it.selenium_test.tests.app_currency.ITeditAppCurrency - ### STARTING TEST: whenEditingAppCurrencyWithValidData_AndClickingSaveAndClose_AppCurrenciesPageShouldBecomeVisible ### 13:23:59.573 [pool-2-thread-3] INFO nl.adaption.it.selenium_test.tests.app_currency.ITeditAppCurrency - ### STOPPING TEST: whenEditingAppCurrencyWithValidData_AndClickingSaveAndClose_AppCurrenciesPageShouldBecomeVisible ###

juherr commented 8 years ago

Assertions are not supposed to be checked in a after method.

Move it into the test method and it should solve your problem.

rouke-broersma commented 8 years ago

I'm not checking my assertions in my after, I'm simply throwing my collected softassert results. if I can't do it in the aftermethod, That means I have to throw my softassertion at the end of every test manually, which is tedious as fuck, and easily forgotten. And if you forget you wouldn't even notice at first.. Perhaps not even for a long time. Putting this in the aftermethod makes much more sense to me. This not working seems like a bug to me, one aftermethod failing should have no effect on the other tests.

juherr commented 8 years ago

In fact, softAssert.assertAll() means "checking assertions" .

SoftAssert is just a way to check many assertions at the same time, what is not possible with 2 successive asserts (the 2nd won't be checked if the 1st fails)

rouke-broersma commented 8 years ago

You're being extremely pedantic for no reason. There is no good way to make sure all softAsserts are "checked", so this was my solution. Even though this is not the intended use, it should still work, all further tests being skipped because of an assert failing in an aftermethod is CLEARLY a bug regardless of my in your eyes incorrect use of softAsserts. There is no logical reason for the AssertionError to make all my further tests skip. Tell me, why do you think this is not a bug?

cbeust commented 8 years ago

@Mobrockers : It's a design goal of TestNG that failure in a configuration method (before or after) is different from a test failure. These methods are meant to set up and tear down your tests, if they crash, your test suite is probably compromised, hence the skipping.

However, I understand your complaint about having to check all these asserts repeatedly at the end of each test method. It sounds like we would need some kind of test wrappers that would be still considered as part of the tests but that could be systematically called after each test method.

There is no such thing in TestNG at the moment.

juherr commented 8 years ago

@Mobrockers http://stackoverflow.com/a/25068591/4234729 may help

@cbeust I didn't try, but maybe something could be done by users with an IHookable?

@Listeners(MyListener.class)
public class MyTest {
   @MySpecificAnnotation
   private SoftAssert softAssert;
}

public class MyListener implements IHookable {
  public void run(final IHookCallBack icb, ITestResult testResult) {
     // 1. Looking for SoftAssert into test class
     // 2. Instanciate SoftAssert
     icb.callback(testResult);
     // 3. catch softAssert.assertAll()
     // 4. custom job on testResult according the assertAll result
  }
}
rouke-broersma commented 8 years ago

@cbeust I understand, unfortunately this was a request from someone else so I do not think I am able to remove this setup. Do you think that if I use factories to initialize my dataProvided tests, and run my tests parallel per instance, testng will no longer treat my tests as belonging to the same suite and thus not skip, or is there no way around that currently? The thing I'm most confused by in all this is that tests in other threads seem unaffected, only tests started in the same thread after the failed aftermethod are skipped from what I can tell.

cbeust commented 8 years ago

@juherr IHookable crossed my mind too but I haven't tried.

@Mobrockers Have you looked into IHookable?

rouke-broersma commented 8 years ago

@cbeust I will ook into it tomorrow at work and report back to you.

rouke-broersma commented 8 years ago

@juherr @cbeust Using factories to instantiate my classes seems to not cause my tests to be skipped on failure so that works, that however is still against the TestNG design I suppose so I have also tried implementing the IHookable solution, but I didn't manage to get it working. The softAssert object in my IHookable testlistener is not the same as the one that contains the error I want to throw, so the test never fails with what I've tried so far. Could it be that the IHookable listener is running in a different thread than the testclass? I am using a ThreadLocal to store my SoftAssert object so I would think that if they run in the same thread that I should be able to use the ThreadLocal to find my SoftAssert.

This is my listener code right now:

public class SoftAssertListener extends TestListenerAdapter implements IHookable{

    private SoftAssert softAssert;
    @Override
    public void onStart(ITestContext testContext) {
        super.onStart(testContext);
        this.softAssert = SoftAssertThreadLocal.get();
    }

    @Override
    public void run(IHookCallBack callBack, ITestResult testResult) {

          callBack.runTestMethod(testResult);
          if(softAssert != null) {
              softAssert.assertAll();
          }
     }
}
rouke-broersma commented 8 years ago

Nevermind it appeared to work but when running on my complete set of tests it no longer does. I guess we're going to have to live with the skipped tests until I find a way to get IHookable working, so help is appreciated greatly.

rouke-broersma commented 8 years ago

Managed to get it working, thanks! I had it like below before, but instead of putting the SoftAssert in a variable first, I tried calling on it directly through ThreadLocal.get(), which was not working for some reason and so I switched to some other ideas like the above, which now that I think about it obviously does not work. The below works so far, i'll see if it holds up with my other listeners and doesn't cause any weird issues.

@Override
public void run(IHookCallBack callBack, ITestResult testResult) {

    SoftAssert softAssert = SoftAssertThreadLocal.get();

    callBack.runTestMethod(testResult);

    if(softAssert != null) {

        SoftAssertThreadLocal.unset();
        softAssert.assertAll();
    }
}

edit: It plays nice with my hard-soft assert as well from what I can tell (I have a class that acts as a hardassert by pulling my single SoftAssert from my ThreadLocal and calling assertAll on assert failure) Example if anyone is curious:

public class HardAssert extends org.testng.asserts.SoftAssert {

    private SoftAssert softAssert;

    public HardAssert() {

        softAssert = SoftAssertThreadLocal.get();
    }

    @Override
    public void onAssertFailure(IAssert<?> assertCommand, AssertionError ex) {
        super.onAssertFailure(assertCommand, ex);
        throwHard();
    }

    @Override
    public void assertTrue(boolean condition, String message) {
        softAssert.assertTrue(condition, message);
    }

    private void throwHard() {

        SoftAssertThreadLocal.unset();
        softAssert.assertAll();
    }
}
juherr commented 8 years ago

@Mobrockers As it seems to work, can we considere to close the issue?

rouke-broersma commented 8 years ago

Unless you have some suggestions for me to improve it, you may close the issue, thanks.

juherr commented 8 years ago

The only "improvment" I see is to use reflection instead of a static method to find your SoftAssert instance (or the ThreadLocal one).

rouke-broersma commented 8 years ago

@juherr I use a ThreadLocal because I use the SoftAssert in other places and need to guarantee there is only one SoftAssert used in the whole test.

juherr commented 8 years ago

Ok, I supposed you only used it to assert a test method. Forget my suggestion then.

hubertgrzeskowiak commented 8 years ago

I have implemented probably the same functionality as @Mobrockers tried. Here's my implementation:

private class VerificationListener implements IInvokedMethodListener {

        private WebDriverTestBase testinstance;

        public VerificationListener(WebDriverTestBase testinstance)
        {
            this.testinstance = testinstance;
        }

        @Override
        public void beforeInvocation(IInvokedMethod method, ITestResult testResult) {
            this.testinstance.clearVerificationErrors();
        }

        @Override
        public void afterInvocation(IInvokedMethod method, ITestResult result)
        {

            // This listener is registered on whole suite, but should be only triggered
            // once per instance.
            if (! this.testinstance.equals(method.getTestMethod().getInstance()))
            {
                return;
            }

            if (method.isTestMethod() && result.isSuccess())
            {
                try {
                    this.testinstance.assertNoVerificationErrors();
                } catch (AssertionError e) {
                    result.setThrowable(e);
                    result.setStatus(ITestResult.FAILURE);
                }
            }
        }
    }

I register this in my test base class using

@BeforeClass
public void setUp(ITestContext context) throws IOException {
    TestRunner runner = (TestRunner) context;
    runner.getSuite().addListener(new VerificationListener(this));
    //...
}

On the base class I maintain a list (actually a map) of verification errors, and offer methods like

verifyEquals(Comparable expected, Comparable actual, String desc)
verifyTrue(Boolean actual, String desc)

The final assertion thrown plays nicely with Eclipse' TestNG Plug-In, offering a side-by-side view with ALL unequal values - something the original SoftAssert doesn't do.

"Verifications" is my term for soft-assertions, by the way. I wanted to prevent conflicts and confusion with TestNG's SoftAssert.

EDIT: now actually using an interface instead of base class and I'm registering the listener using the Listeners annotation. Let me know if you want to see the newest code :-)

anandbagmar commented 6 years ago

Hi @hubertgrzeskowiak - what does your clearVerificationErrors method do? this.testinstance.clearVerificationErrors();

I would like to see your code if its ok / possible for you to share. Thanks!

hubertgrzeskowiak commented 6 years ago

@anandbagmar apologies for not uploading the whole code. My WebDriverTestBase testinstance is having a private list of verification errors. Whenever a verification (a.k.a. soft assert) fails, I add an entry to that list. clearVerificationErrors() is simply clearing the list. assertNoVerificationErrors() is basically assert this.verification_errors.isEmpty() with some fancy printing of the list's contents.

I can't give you the original code, because I switched companies since.

anandbagmar commented 6 years ago

@hubertgrzeskowiak - that info helps. I solved the problem the following way:

I made my BaseTest implement IHookable interface. And then implemented the methods as below:

    private static final String SOFT_ASSERT = "softAssert";

    @Override
    public void run(IHookCallBack callBack, ITestResult testResult) {
        SoftAssert softAssert = new SoftAssert();
        testResult.setAttribute(SOFT_ASSERT, softAssert);
        callBack.runTestMethod(testResult);
        softAssert = getSoftAssert(testResult);
        if (!testResult.isSuccess()) {
            Throwable throwable = testResult.getThrowable();
            if (null != throwable) {
                if (null != throwable.getCause()) {
                    throwable = throwable.getCause();
                }
                softAssert.assertNull(throwable, ExceptionUtils.getStackTrace(throwable));
            }
        }
        softAssert.assertAll();
    }

    public static SoftAssert getSoftAssert() {
        return getSoftAssert(Reporter.getCurrentTestResult());
    }

    private static SoftAssert getSoftAssert(ITestResult result) {
        Object object = result.getAttribute(SOFT_ASSERT);
        if (object instanceof SoftAssert) {
            return (SoftAssert) object;
        }
        throw new IllegalStateException("Could not find a soft assertion object");
    }

This solution helps to fail the test with all soft asserts + any hard assert as well.

Hanamant1990 commented 5 years ago

Hi,

The below code works fine if I run a single test, however if I run the whole suite from the xml file by specifying the below listener in it, it doesn't run, pls help.

public class SoftAssertListener implements IHookable {
     String testName = null;

        private static final String SOFT_ASSERT = "softAssert";

        @Override
        public void run(IHookCallBack callBack, ITestResult testResult) {
            System.out.println("soft assert listener called");
           LogClass.softAssert=new SoftAssert();
           SoftAssert softAssert=LogClass.softAssert;
            testResult.setAttribute(SOFT_ASSERT, softAssert);
            try {
            callBack.runTestMethod(testResult);
            }catch(Exception ex) {
                LogClass.softAssert.assertTrue(false, "exception occured in test method");
            }
            softAssert.assertAll();
        }
}
krmahadevan commented 5 years ago

@Hanamant1990 - I would request you to please create a new issue (please ensure you are using the latest released version of TestNG viz., 7.0.0.-beta1 to reproduce the problem) and include a full fledged simple example that can be used to reproduce the issue, if this is still. problem.

Hanamant1990 commented 5 years ago

Thanks Mahadevan,

I was getting run time error upon updating to 7.0.0 beta, however the issue got resolved when I updated test ng to 6.11

juherr commented 5 years ago

@Hanamant1990 If I understand well. It is working well with 6.11 but it is failing with 7.0-beta1, right?