testng-team / testng-eclipse

Eclipse plug-in for TestNG
https://testng.org
194 stars 164 forks source link

TestNG eclipse plugin doesn't show proper exception if @BeforeMethod (using ITestResult as parameter) fails #307

Closed jimpatel90 closed 7 years ago

jimpatel90 commented 7 years ago

Plugin Version

6.10.0.201612030230

Steps to reproduce:

Try to run below class using eclipse (TestNG eclipse plugin) and observe report generated in eclipse.

import org.testng.Assert;
import org.testng.ITestResult;
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;

public class TestPlugin {

    @BeforeMethod
    public void setUp(ITestResult result) {
        Assert.fail();
    }

    @Test
    public void testPlugin() {
        // To some tests here
    }
}

Expected behaviour

In result, setUp should show as failed with correct exception(In this case, java.lang.AssertionError)

Actual behaviour

Test is shown as fail(instead of setUp) without giving actual exception. Also, it doesn't generate other reports (HTML and JUnit XML) It gives following exception:

java.lang.RuntimeException
    at org.testng.internal.TestResult.toString(TestResult.java:251)
    at org.testng.internal.TestResult.toString(TestResult.java:236)
    at org.testng.reporters.TestHTMLReporter.generateTable(TestHTMLReporter.java:118)
    at org.testng.reporters.TestHTMLReporter.generateLog(TestHTMLReporter.java:303)
    at org.testng.reporters.TestHTMLReporter.onFinish(TestHTMLReporter.java:44)
    at org.testng.TestRunner.fireEvent(TestRunner.java:1200)
    at org.testng.TestRunner.afterRun(TestRunner.java:991)
    at org.testng.TestRunner.run(TestRunner.java:604)
    at org.testng.SuiteRunner.runTest(SuiteRunner.java:366)
    at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:361)
    at org.testng.SuiteRunner.privateRun(SuiteRunner.java:319)
    at org.testng.SuiteRunner.run(SuiteRunner.java:268)
    at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
    at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:86)
    at org.testng.TestNG.runSuitesSequentially(TestNG.java:1264)
    at org.testng.TestNG.runSuitesLocally(TestNG.java:1189)
    at org.testng.TestNG.runSuites(TestNG.java:1104)
    at org.testng.TestNG.run(TestNG.java:1076)
    at org.testng.remote.AbstractRemoteTestNG.run(AbstractRemoteTestNG.java:132)
    at org.testng.remote.RemoteTestNG.initAndRun(RemoteTestNG.java:236)
    at org.testng.remote.RemoteTestNG.main(RemoteTestNG.java:81)

Note: If I remove ITestResult result from setUp, it works fine(gives proper exception).

Operating System

Windows

missedone commented 7 years ago

hi @jimpatel90, what do you want to achieve by passing ITestResult to beforemethod:

    @BeforeMethod
    public void setUp(ITestResult result) {
        Assert.fail();
    }
jimpatel90 commented 7 years ago

Hi @missedone,

In my case, setUp creates a selenium webdriver instance which is used in running test. Tests run in parallel mode at methods level. Earlier, I used ThreadLocal for driver instance to manage seperate drivers for each tests. It was working fine. But when I applied timeOut at @Test level, problem started. I realized that setUp and testMethods run in different thread if timeOut is applied. Hence ThreadLocal doesn't work in this case. Then, I thought of using a Map that manages test-local-driver. For that I need unique key for each test. I used ITestResult for that because it would be unique for each test execution and I can easily use Reporter.getCurrentTestResult() to retrieve it for running test. During one of my execution, setUp failed and result in eclipse was odd. Hence logged this.

Something like below:

    @BeforeMethod
    public void setUp(ITestResult result) {
        DriverManager.createTestLocalDriver(result);
    }

DriverManager.java

    public static void createTestLocalDriver(ITestResult result){
        map.put(result, Newly created driver instance);
    }

    public static WebDriver getTestLocalDriver(){
        return map.get(Reporter.getCurrentTestResult());
    }

Thanks

missedone commented 7 years ago

@jimpatel90 , could you use some other ParallelMode than TEST, for example, INSTANCES, try to make sure your threadLocal approach work? (this is according to TestNG source code below:)

  protected static void invokeWithTimeout(ITestNGMethod tm, Object instance,
      Object[] parameterValues, ITestResult testResult, IHookable hookable)
      throws InterruptedException, ThreadExecutionException {
    if (ThreadUtil.isTestNGThread() && testResult.getTestContext().getCurrentXmlTest().getParallel() != XmlSuite.ParallelMode.TESTS) {
      // We are already running in our own executor, don't create another one (or we will
      // lose the time out of the enclosing executor).
      invokeWithTimeoutWithNoExecutor(tm, instance, parameterValues, testResult, hookable);
    } else {
      invokeWithTimeoutWithNewExecutor(tm, instance, parameterValues, testResult, hookable);
    }
  }

@juherr, do you have any suggestion for @jimpatel90 's case?

jimpatel90 commented 7 years ago

Previously, I used class level parallel mode. There are certain classes that contain more test methods and hence these methods were running in single thread taking more execution time even if other threads were available. Hence I changed parallel mode to methods level to maximize use of parallel threads.

Will InheritableThreadLocal work instead of ThreadLocal? It worked for one of my test(with timeOut set). I am not sure how would it behave with other tests (having DataProvider or parameters or invocationCount>1). I will try that tomorrow and update this thread.

juherr commented 7 years ago

The main issue is already known: https://github.com/cbeust/testng/issues/914 Timeout and parallel features are not good friends for the moment.

About:

@BeforeMethod
    public void setUp(ITestResult result) {

It is supposed to work (or fail with an appropriate message). https://github.com/cbeust/testng/blob/master/src/main/java/org/testng/internal/TestResult.java#L233

missedone commented 7 years ago

@juherr , if there's no recommended approach for @jimpatel90 's use case, I think we might need to workaround this at TestNG:

  @Override
  public String toString() {
    List<String> output = Reporter.getOutput(this);
    return Objects.toStringHelper(getClass())
        .omitNulls()
        .omitEmptyStrings()
        .add("name", getName())
// Nick: here threw the RuntimeException because current status is -1, the initial state.
        .add("status", toString(m_status))
        .add("method", m_method)
        .add("output", output != null && output.size() > 0 ? output.get(0) : null)
        .toString();

  }

  private static String toString(int status) {
    switch(status) {
      case SUCCESS: return "SUCCESS";
      case FAILURE: return "FAILURE";
      case SKIP: return "SKIP";
      case SUCCESS_PERCENTAGE_FAILURE: return "SUCCESS WITHIN PERCENTAGE";
      case STARTED: return "STARTED";
// Nick: we can return a message "UNKNOWN STATUS" instead of throw exception in a toString() method 
      default: throw new RuntimeException();
    }
  }
juherr commented 7 years ago

@missedone I am not feeling it good because this state is not supposed to happen in an afterRun event. We should dig deeper and understand the root cause first. Maybe you can open an issue on testng project.

@jimpatel90 Is public void setUp(ITestResult result) { working when you disable parallel run?

jimpatel90 commented 7 years ago

@juherr , Its not working without parallel too (i.e. not showing proper exception in case setUp(ITestResult result) fails).

BTW, using InheritableThreadLocal instead of ThreadLocal worked for me. I am able to set driver instance in setUp method and retrieve same instance from test(having timeOut). Hence my problem is solved and I do't need to use ITestResult in setUp method.

Thanks