testng-team / testng

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

Replacement API for `IClass.getInstanceHashCodes()` and `IClass.getInstances(boolean)` #3111

Open marcphilipp opened 5 months ago

marcphilipp commented 5 months ago

In 7.10.1, the IClass.getInstanceHashCodes() method was deprecated. As discussed in https://github.com/junit-team/testng-engine/issues/116#issuecomment-2044320087, it is used by the JUnit Platform adapter (testng-engine) to append the instance index to the display name which is important to differentiate between different runs. Prior to removing the method, a replacement for this use case should be added.

juherr commented 5 months ago

As I understand, something like Integer ITestResult.getFactoryMethod().getInvocationIndex() should provided.

marcphilipp commented 5 months ago

Yes, that sounds sensible. 👍

krmahadevan commented 5 months ago

@marcphilipp - Can you please check if this would work for you ( This exists already within TestNG)

        ITestResult itr = Reporter.getCurrentTestResult();
        int index = itr.getMethod().getFactoryMethodParamsInfo().getIndex();
marcphilipp commented 5 months ago

Yes, that works on 7.5 and later (IParameterInfo.getIndex() was added in 5cffe02b91cacfa9253aac44dae4b6645164efad). 👍

Why is IParameterInfo in an internal package? Is it ok to use it anyway?

krmahadevan commented 5 months ago

🤦 - I didnt see the package information. Let me see what we can do to ensure that information is available. Worst case, we can expose that interface to the public package since it does have information that can be of relevance to the user.

@juherr WDYT ?

marcphilipp commented 5 months ago

I checked for other deprecations and found IClass.getInstances(boolean) which is used to compute the index of the test instance: https://github.com/junit-team/testng-engine/blob/dcd461e556f618685084ec6a6719c91a227a1298/src/main/java/org/junit/support/testng/engine/MethodDescriptor.java#L53-L64

Is there an existing replacement for that as well?

marcphilipp commented 5 months ago

WIP PR for replacing the call site of deprecated APIs: https://github.com/junit-team/testng-engine/pull/121

juherr commented 5 months ago

@krmahadevan I think IParameterInfo can be moved to the public package.

@marcphilipp You don't need result.getTestClass().getInstances(true) if result.getMethod().getFactoryMethodParamsInfo().getIndex() provides the good index value, right?

marcphilipp commented 5 months ago

You're right. I found one case where that doesn't work, though:

import static org.testng.Assert.assertNotEquals;

import org.testng.annotations.Factory;
import org.testng.annotations.Test;

public class FactoryMethodTestCase {

    private final String a;
    private final String b;

    @Factory
    public static Object[] factoryData() {
        return new Object[] { new FactoryMethodTestCase("a", "b"), new FactoryMethodTestCase("c", "d") };
    }

    public FactoryMethodTestCase(String a, String b) {
        this.a = a;
        this.b = b;
    }

    @Test
    public void test() {
        assertNotEquals(a, b);
    }
}

For this test class, IParameterInfo.getIndex() always returns 0 even though it should return 1 for the second instance. Is that a known issue?

juherr commented 5 months ago

I didn't find any test that checked the feature.

Samples from https://github.com/testng-team/testng/tree/master/testng-core/src/test/java/test/factory/github1083 could be used.

krmahadevan commented 5 months ago

@marcphilipp

For this test class, IParameterInfo.getIndex() always returns 0 even though it should return 1 for the second instance. Is that a known issue?

There are two scenarios in which someone would use the @Factory annotation. We are seeing a value of 0 for the second scenario. I will debug and find out why is this the case.

Are you still looking for a replacement for getInstanceHashCode() ?

Scenario 1

A constructor that is annotated using the @Factory annotation and the @Factory is tied to a data provider.

Scenario-1-Sample ```java import org.testng.annotations.DataProvider; import org.testng.annotations.Factory; import org.testng.annotations.Listeners; import org.testng.annotations.Test; @Listeners(LoggingListener.class) public class TestCaseSample { private final int ignored; @Factory(dataProvider = "getData") public TestCaseSample(int i) { this.ignored = i; } @Test public void testMethod() { } @DataProvider public static Object[][] getData() { return new Object[][]{{1}, {2}}; } } ```
Execution output ```shell LF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder". SLF4J: Defaulting to no-operation (NOP) logger implementation SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details. Index = 0 Index = 1 =============================================== Default Suite Total tests run: 2, Passes: 2, Failures: 0, Skips: 0 =============================================== ```

Scenario 2

A static method that is annotated using @Factory annotation.

Scenario-2-Sample ```java import org.testng.annotations.DataProvider; import org.testng.annotations.Factory; import org.testng.annotations.Listeners; import org.testng.annotations.Test; @Listeners(LoggingListener.class) public class AnotherTestCaseSample { private final int ignored; public AnotherTestCaseSample(int i) { this.ignored = i; } @Test public void testMethod() { } @Factory public static Object[] getInstance() { return new AnotherTestCaseSample[]{ new AnotherTestCaseSample(1), new AnotherTestCaseSample(2), }; } @DataProvider public static Object[][] getData() { return new Object[][]{{1}, {2}}; } } ```
Execution output ```shell SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder". SLF4J: Defaulting to no-operation (NOP) logger implementation SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details. Index = 0 Index = 0 =============================================== Default Suite Total tests run: 2, Passes: 2, Failures: 0, Skips: 0 =============================================== ```
marcphilipp commented 5 months ago

Are you still looking for a replacement for getInstanceHashCode() ?

Only if result.getMethod().getFactoryMethodParamsInfo().getIndex() doesn't work here.

krmahadevan commented 5 months ago

@marcphilipp - Can you please tell me how can I programmatically use the JUnit TestNG platform library and run things from within an IDE (without using the console launcher or the maven integration)

I am basically looking for something similar to

TestNG testng = new TestNG();
testng.run();

I am doing something like this

        TestNGTestEngine engine = new TestNGTestEngine();
        engine.discover()
        ExecutionRequest request = new ExecutionRequest();
        engine.execute(request);

but am not sure as to how do I pass in a proper implementation to the discover and the execute method.

marcphilipp commented 5 months ago

There are multiple options but I'd recommend using EngineTestKit for this:

import org.junit.platform.engine.discovery.DiscoverySelectors;
import org.junit.platform.testkit.engine.EngineExecutionResults;
import org.junit.platform.testkit.engine.EngineTestKit;
import org.testng.annotations.Test;

public class Demo {

    public static void main(String[] args) {
        EngineExecutionResults results = EngineTestKit.engine(new TestNGTestEngine())
                .selectors(DiscoverySelectors.selectClass(TestCase.class))
                .execute();

        results.allEvents().debug();
    }

    public static class TestCase {
        @Test
        public void test() {
        }
    }
}

Please refer to https://junit.org/junit5/docs/current/user-guide/#testkit for more information.

krmahadevan commented 5 months ago

@marcphilipp - Thank you for sharing that information. Can you please also let me know as to how do I check for the place wherein the hashCode is being used in the reporting? That will help me check the place wherein this is being used and also debug more.

krmahadevan commented 5 months ago

I have made some progress and found as to where is the getInstanceHashCodes() being used and its relevance.

But I am trying to understand as to why does JUnit create test class instances, especially here.

Also can you please let me know as to when would the output of toMethodId be used so that I can check that as well?

marcphilipp commented 4 months ago

The potentially created test class instances are only used to have a reliable way to compute an "invocation index". The index is used as part of the unique ID since in JUnit platform every test class, method, invocation, etc. must have a unique ID which is used for reporting etc. Therefore, if result.getMethod().getFactoryMethodParamsInfo().getIndex() could be "fixed" to also work in the https://github.com/testng-team/testng/issues/3111#issuecomment-2051160016 scenario, we could remove the call to getInstanceHashCodes().

@krmahadevan Have you had a chance whether it always returning 0 in the linked scenario is on purpose or a bug that can be fixed?

krmahadevan commented 4 months ago

@marcphilipp

Have you had a chance whether it always returning 0 in the linked scenario is on purpose or a bug that can be fixed?

The zero returning by the method is due to the fact that it tries to align itself with the parameters that can be passed to a factory method along with the indices. So using this method may not be a solution in this case.

I have a PR raised that can be used to for JUnit purposes and you wouldn't need to depend on the getIndex() method.

marcphilipp commented 4 months ago

Sounds good! Please let me know when that's merged and available in a snapshot version.