[TestResult] confusion (set / get) methods to obtain the method name

[shaburov]

testng-7.0.0-beta1

interface ITestResult contains methods:

  /** @return The name of this TestResult, 
   * typically identical to the name of the method. 
   */
  String getName();

   /**
   * If this result's related instance implements ITest or use @Test(testName=...), 
   * returns its test name, otherwise returns null.
   */
  String getTestName();

  /** @param name - The new name to be used as a test name */
  void setTestName(String name);

However, the implementation class TestResult return: IInvokedMethod.getTestResult().getName() -> TestName IInvokedMethod.getTestResult().getTestName() -> null

Not only is the result confusing. The implementation of TestResult is contrary to the contract ITestResult.

Playback Code:

import org.testng.IInvokedMethod;
import org.testng.IInvokedMethodListener;
import org.testng.ITestResult;
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Listeners;
import org.testng.annotations.Test;

import java.util.UUID;

@Listeners(Bug.InvokedMethodListener.class)
public class Bug {
    @BeforeMethod
    public void BeforeMethod() { }
    @Test()
    public void test() { }

    public static class InvokedMethodListener implements IInvokedMethodListener {
        public void beforeInvocation(IInvokedMethod method, ITestResult testResult) {
            String methodName = method.getTestMethod().getMethodName();
            String shortUUID = UUID.randomUUID().toString().substring(0, 8);
            method.getTestResult().setTestName(methodName + "@" + shortUUID);
            System.out.println("getTestResult().setTestName() - " + methodName + "@" + shortUUID);
            System.out.println("getTestResult().getName()     - " + method.getTestResult().getName());
            System.out.println("getTestResult().getTestName() - " + method.getTestResult().getTestName());
            System.out.println("\n");
        }
    }
}

Actual console output:

getTestResult().setTestName() - BeforeMethod@9982ef7c
getTestResult().getName()     - BeforeMethod@9982ef7c
getTestResult().getTestName() - null

getTestResult().setTestName() - test@46f0a694
getTestResult().getName()     - test@46f0a694
getTestResult().getTestName() - null

Expected console output:

getTestResult().setTestName() - BeforeMethod@9982ef7c
getTestResult().getName()     - BeforeMethod
getTestResult().getTestName() - BeforeMethod@9982ef7c

getTestResult().setTestName() - test@46f0a694
getTestResult().getName()     - test
getTestResult().getTestName() - test@46f0a694

In my personal conviction, the implementations of the getName () and getTestName () methods are confused.

[shaburov]

@juherr, consent to refactoring.

1. Set @Deprecate for old methods and

   • setTestName()
   • getName()
   • getTestName()

2. Add new methods to the ITestResult and TestResult:

   • setResultTestName(String string) - string != null && string not empty
   • String getResultTestName() - String (default return ITestNGMethod name)
   • setTestMethodName(String string) -> return UnsupportedOperationException
   • String getTestMethodName - return real test method name (ITestNGMethod)

3. Make changes to reports (IReporter and etc.)

4. Make changes to existing test + add new test

[krmahadevan]

@shaburov -

• getName() by default is supposed to be returning back the calculation from one of the following // Calculate the name: either the method name, ITest#getTestName or toString() if it's been overridden.

• getTestName() is supposed to support the customized test names provided via the ITest implementation that a test class is supposed to provide. Only if that is provided by a test class, would getTestName() give you a valid value and in other cases it is supposed to be returning a null (as the java docs say)

So wouldn't the following refactoring be less invasive ?

• deprecate setTestName() and add a new method called setName()

I believe the confusion is only because setTestName() method name is wrongly named. It should have been just named as setName() (so that its in accordance with getName())

[juherr]

Agree to rename setTestName() to setName().

@shaburov Does it fix the issue too?

[shaburov]

@juherr I do not think it's a good idea. Renaming the API of a method without @Deprecate can break the user's implementation. Although if you look at it from the other side, this is version 7.+ and similar changes are implied. As you say, I'll do.

[juherr]

@shaburov IMO both options are valid.

But I agree that a deprecation will be more safe and fair.

[danberindei]

@juherr @shaburov setTestName was only introduced in 7.0.0-beta1, so it would have been in fact very easy to rename it to setName back then.

[danberindei]

getName() by default is supposed to be returning back the calculation from one of the following // Calculate the name: either the method name, ITest#getTestName or toString() if it's been overridden.

@krmahadevan that's what the implementation comment in TestResult.init() says, but the API documentation in ITestResult.getName() says

@return The name of this TestResult, typically identical to the name of the method.

So the implementation is wrong, and it should have never used the testName from the @Test annotation, or ITest.getTestName().

Other tools, like maven-surefire, expect ITestResult.getName() to return a method name, and they report just testResult.getName(), not testResult.getName() + "." + testResult.getMethod().getName():

https://github.com/apache/maven-surefire/blob/220652a04acf12f02245ef51b740b82c64d16c15/surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/TestNGReporter.java#L70

IntelliJ has a workaround for this bug, but it's only applied when testResult.getTestName().equals(testResult.getTestClass().getTestName()), so they hope it will be fixed at some point:

https://github.com/JetBrains/intellij-community/blob/f00e91891fd274b14cf88b71725acd4375a3fecd/plugins/testng_rt/src/org/testng/IDEATestNGRemoteListener.java#L363

[krmahadevan]

@shaburov - Would you be willing to help raise a PR for this ? That way it would be easier to reason out and then track this to closure ?

[juherr]

The expected behavior about name is supposed to be covered by tests: https://github.com/cbeust/testng/blob/master/src/test/java/test/name/NameTest.java

@danberindei Do you think one or more of the tests should be changed? I agree that the documentation should be fixed accordingly.