Update MethodInvocationHelper.java

[sainjalpolys]

Fixes # .

Did you remember to?

• Add test case(s)
• Update CHANGES.txt

We encourage pull requests that:

• Add new features to TestNG (or)
• Fix bugs in TestNG

If your pull request involves fixing SonarQube issues then we would suggest that you please discuss this with the
TestNG-dev before you spend time working on it.

[krmahadevan]

@sainjalpolys - Thanks for taking the time to submit the PR. But can you please help elaborate as to what is the issue that we are trying to fix in this PR ? Also please help add a test that vets out the change.

[sainjalpolys]

 private List<String> errorCodes = Arrays.asList("ERR100909", "ERR100");

    @Override
    public void run(IHookCallBack callBack, ITestResult testResult) {
        callBack.runTestMethod(testResult);
        Throwable t = testResult.getThrowable();
        if (t != null) {
            t = t.getCause();
        }

        boolean shouldFail = (t instanceof LocationBDLException&& errorCodes.contains(((LocationBDLException) t).getErrorCode()));
        if (shouldFail) {
            testResult.setThrowable(null);
            testResult.setStatus(ITestResult.SUCCESS);
        } else {
            testResult.setThrowable(t);
            testResult.setStatus(ITestResult.FAILURE);
        }
      
    }

    @Test()
    public void test1() throws LocationBDLException {
        throw new LocationBDLException("ERR100", "s", new Throwable());
    }

But while calling runTestMethod, It is catching the exception and setting the error variable in the catch block..

  @Override
      public void runTestMethod(ITestResult tr) {
        try {
          invokeMethod(thisMethod, testInstance, parameters);
        } catch (Throwable t) {
          error[0] = t;
          tr.setThrowable(t); // make Throwable available to IHookable
        }
      }

So even if we are setting the test result as success,

testResult.setThrowable(null);
testResult.setStatus(ITestResult.SUCCESS);

It will throw exception and test is getting failure

protected static void invokeHookable(final Object testInstance, final Object[] parameters,
      Object hookableInstance, final Method thisMethod, TestResult testResult) throws Throwable {
    Method runMethod = hookableInstance.getClass().getMethod("run",
        new Class[] { IHookCallBack.class, ITestResult.class });
    final Throwable[] error = new Throwable[1];

    IHookCallBack callback = new IHookCallBack() {
      @Override
      public void runTestMethod(ITestResult tr) {
        try {
          invokeMethod(thisMethod, testInstance, parameters);
        } catch (Throwable t) {
          error[0] = t;
          tr.setThrowable(t); // make Throwable available to IHookable
        }
      }

      @Override
      public Object[] getParameters() {
        return parameters;
      }
    };
    runMethod.invoke(hookableInstance, new Object[] { callback, testResult });
    if (error[0] != null) {
      throw error[0];
    }
  }

So we can avoid setting error[0] = t; and throwing exception from there class . .

[juherr]

That's strange it doesn't break tests. So I'm not opposed to the change. But it will break the backward compatibility.

private List<String> errorCodes = Arrays.asList("ERR100909", "ERR100");

    @Override
    public void run(IHookCallBack callBack, ITestResult testResult) {
        try { 
            callBack.runTestMethod(testResult);
        } catch(Exception e) {
            t = e.getCause();
        }

        boolean shouldFail = (t instanceof LocationBDLException&& errorCodes.contains(((LocationBDLException) t).getErrorCode()));
        if (shouldFail) {
            testResult.setThrowable(null);
            testResult.setStatus(ITestResult.SUCCESS);
        } else {
            testResult.setThrowable(t);
            testResult.setStatus(ITestResult.FAILURE);
        }
      
    }

    @Test()
    public void test1() throws LocationBDLException {
        throw new LocationBDLException("ERR100", "s", new Throwable());
    }

should have the behavior you expect too.

@krmahadevan I let you take the merge decision.

[krmahadevan]

@juherr -

That's strange it doesn't break tests.

Perhaps there is no tests that is currently checking this ? That perhaps explains why there are no build breakages.

But it will break the backward compatibility.

Perhaps yes. But I guess we should be fine, because this is a new major version.

@sainjalpolys - Thanks for elaborating the issue. Can you please help add a test in test.hook.HookableTest that vets out your change ?
We also would like to ensure that the contributors get credited for their contributions. The usual practice is that, one creates a new issue (or) refers to an existing issue, in the CHANGES.txt file with a reference to the github issue and also the contributors name. I would leave it to you if you would like to go through with that or not.

But as soon as you can please help add a test, we should be able to get this changeset merged.

[juherr]

@sainjalpolys Is my solution working for you?

I found one of my old answers too: https://stackoverflow.com/q/38784823/4234729

@krmahadevan In fact, my surprise is because of the thrown exception is not useful for the internal flow. It means that with or without it TestNG is still working as expected.

I didn't find anything in the documentation or javadoc talking about the expected behavior. So we can decide to apply the change if wanted. I have no opinion about the best choice :)

[juherr]

Tests are missing

[sainjalpolys]

@julien

…

On Sun, Mar 24, 2019 at 6:38 PM Julien Herr ***@***.***> wrote: ***@***.**** requested changes on this pull request. Tests are missing — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2040 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AZHBDAQtEwStC1xceQn-5EJUjI_S2Te8ks5vZ3i8gaJpZM4cEveW> .

[sainjalpolys]

@julien Herr - Thanks for the follow up.. And Sorry.. your solution doesn't work for me..

…

On Sun, Mar 24, 2019 at 6:38 PM Julien Herr ***@***.***> wrote: ***@***.**** requested changes on this pull request. Tests are missing — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2040 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AZHBDAQtEwStC1xceQn-5EJUjI_S2Te8ks5vZ3i8gaJpZM4cEveW> .

[juherr]

@sainjalpolys

And Sorry.. your solution doesn't work for me..

Why? What is the behavior? The exception you removed is supposed to be caught now.