testng-team / testng

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

Parametrized set of test cases with @Factory annotation behaves incorrectly #2867

Closed OlegKuzovkov closed 1 year ago

OlegKuzovkov commented 1 year ago

I am trying to migrate from Juni4 to testNG and noticed that when trying to create a test class with different parameters set using @Factory and @DataProvider + @BeforeClass method the execution behavior is not as expected. The setup is:

public class TestClass1 {

    private static String str1;
    private static String str2;

    @Factory(dataProvider = "parameters")
    public TestClass1(String sss, String ggg) {
        str1 = sss;
        str2 = ggg;
        log.info("Print in constructor");
    }

    @DataProvider
    public static Object[][] parameters() throws Exception {
        return new Object[][]{
                { "Data 1", "Data 1 1" },
                { "Data 2", "Data 2 2" }
        };
    }

    @BeforeClass
    public static void beforeClass() {
       log.info("Print in before " + str1);
    }

    @Test
    public void asdasdTest() {
        log.info("Print in test 1 " + str2);
    }

    @Test
    public void asdasdTest2() {
        log.info("Print in test 2 " + str2);
    }
}

After the execution I see the following weird execution order:

2023-01-10 13:43:18,673 [main] INFO  com.ctera.auto.tests.unittest.TestClass1 - Print in constructor
2023-01-10 13:43:18,684 [main] INFO  com.ctera.auto.tests.unittest.TestClass1 - Print in constructor
13:43:18.721 WARN  o.t.i.Configuration - main - Detected a static method [com.ctera.auto.tests.unittest.TestClass1.beforeClass()]. Static configuration methods can cause  unexpected behavior.
13:43:18.730 WARN  o.t.i.Configuration - main - Detected a static method [com.ctera.auto.tests.unittest.TestClass1.beforeClass()]. Static configuration methods can cause  unexpected behavior.
13:43:18.749 INFO  o.t.i.Utils - main - [TestNG] Running:
  C:\Users\olegk\AppData\Local\JetBrains\IdeaIC2022.3\temp-testng-customsuite.xml

2023-01-10 13:43:18,804 [main] INFO  com.ctera.auto.tests.unittest.TestClass1 - Print in before Data 2
2023-01-10 13:43:18,853 [main] INFO  com.ctera.auto.tests.unittest.TestClass1 - Print in test 1 Data 2 2
2023-01-10 13:43:18,927 [main] INFO  com.ctera.auto.tests.unittest.TestClass1 - Print in before Data 2
2023-01-10 13:43:18,930 [main] INFO  com.ctera.auto.tests.unittest.TestClass1 - Print in test 1 Data 2 2
2023-01-10 13:43:18,934 [main] INFO  com.ctera.auto.tests.unittest.TestClass1 - Print in test 2 Data 2 2
2023-01-10 13:43:18,938 [main] INFO  com.ctera.auto.tests.unittest.TestClass1 - Print in test 2 Data 2 2

Comparing this behavior with Junit4 parametrization it seems there is an issue with that in testNG.

Junit4 same setup:

@RunWith(Parameterized.class)
public class TestClass2 {

    private static String str1;
    private static String str2;

    @Parameterized.Parameters
    public static Collection<Object[]> parameters() throws Exception {
        return Arrays.asList(new Object[][]{
                { "Data 1", "Data 1 1" },
                { "Data 2", "Data 2 2" }
        });
    }

    public TestClass2(String sss, String ggg) {
        str1 = sss;
        str2 = ggg;
        log.info("Print in constructor");
    }

    @BeforeClass
    public static void beforeClass() {
        log.info("Print in before " + str1);
    }

    @Test
    public void asdasdTest() {
        log.info("Print in test " + str2);
    }

    @Test
    public void asdasdTest2() {
        log.info("Print in test 2 " + str2);
    }
}

JUnit4 output:

2023-01-10 13:40:47,336 [main] INFO  com.ctera.auto.tests.unittest.TestClass2 - Print in constructor
2023-01-10 13:40:47,339 [main] INFO  com.ctera.auto.tests.unittest.TestClass2 - Print in test Data 1 1
2023-01-10 13:40:47,341 [main] INFO  com.ctera.auto.tests.unittest.TestClass2 - Print in constructor
2023-01-10 13:40:47,341 [main] INFO  com.ctera.auto.tests.unittest.TestClass2 - Print in test 2 Data 1 1
2023-01-10 13:40:47,342 [main] INFO  com.ctera.auto.tests.unittest.TestClass2 - Print in constructor
2023-01-10 13:40:47,342 [main] INFO  com.ctera.auto.tests.unittest.TestClass2 - Print in test Data 2 2
2023-01-10 13:40:47,343 [main] INFO  com.ctera.auto.tests.unittest.TestClass2 - Print in constructor
2023-01-10 13:40:47,343 [main] INFO  com.ctera.auto.tests.unittest.TestClass2 - Print in test 2 Data 2 2
2023-01-10 13:40:47,031 [main] INFO  com.ctera.auto.tests.unittest.TestClass2 - Print in before null

TestNG Version

7.7.0

Expected behavior

We would love this to behave similar to how JUnit4 interprets that:

  1. @BeforeClass method should be executed only once and not for each test/test class instance separately, because this is a static method
  2. Constructor code should be executed once per test data entry
  3. Test 1 and Test 2 are executed per test data entry

Actual behavior

Execution order that I see:

  1. Constructor code is executed for each test data entry first
  2. @BeforeClass method is executed
  3. Only first test is executed
  4. @BeforeClass method is executed again
  5. Test 1 is executed
  6. Test 2 is executed
  7. Test 2 us executed AGAIN

Is the issue reproducible on runner?

Contribution guidelines

Incase you plan to raise a pull request to fix this issue, please make sure you refer our Contributing section for detailed set of steps.

krmahadevan commented 1 year ago

@OlegKuzovkov - I dont think that there is any issue here. TestNG is working as designed. TestNG will invoke the @BeforeClass annotated method for each instance of your test class. For a normal test class that is not powered by a factory, the behaviour would be similar to how JUnit does, but you will see this behavior stand out when you couple the @BeforeClass with a factory powered test, wherein you are creating multiple instances of the same class.

So currently what you ask for has never existed in TestNG (Atleast not to the best of my knowledge). I am not sure if building parity with JUnit is the direction that we would want to take for TestNG.

@juherr @cbeust - What are your thoughts on building this parity?

OlegKuzovkov commented 1 year ago

@krmahadevan If you were right, why would only the first test be executed after the first execution of the @BeforeClass method, and after the second execution of the @BeforeClass method, the first test was executed once and the second twice? Check the console output:

2023-01-10 13:43:18,804 [main] INFO  com.ctera.auto.tests.unittest.TestClass1 - Print in before Data 2
2023-01-10 13:43:18,853 [main] INFO  com.ctera.auto.tests.unittest.TestClass1 - Print in test 1 Data 2 2
2023-01-10 13:43:18,927 [main] INFO  com.ctera.auto.tests.unittest.TestClass1 - Print in before Data 2
2023-01-10 13:43:18,930 [main] INFO  com.ctera.auto.tests.unittest.TestClass1 - Print in test 1 Data 2 2
2023-01-10 13:43:18,934 [main] INFO  com.ctera.auto.tests.unittest.TestClass1 - Print in test 2 Data 2 2
2023-01-10 13:43:18,938 [main] INFO  com.ctera.auto.tests.unittest.TestClass1 - Print in test 2 Data 2 2

Also, this is quite a cool feature if Junit that testNG is missing. Why won't you have it in some way also?

At the moment what we need is the capability to execute parameterized test class with different data sets having a single @BeforeClass for all of that and I am trying to find a way to implement it with testNG and no luck so far. Could you pls advise?

krmahadevan commented 1 year ago

@krmahadevan If you were right, why would only the first test be executed after the first execution of the https://github.com/BeforeClass method, and after the second execution of the https://github.com/BeforeClass method, the first test was executed once and the second twice? Check the console output:

@OlegKuzovkov - Please remove the static classifier for your data members and your @BeforeClass and try again. You should see the expected result. Your static data members are basically causing the data to be overwritten.

Also, this is quite a cool feature if Junit that testNG is missing. Why won't you have it in some way also?

For historical reasons TestNG has always worked this way. Which is why am trying to get inputs from @cbeust and @juherr as to what should TestNG's stance be.

At the moment what we need is the capability to execute parameterized test class with different data sets having a single @BeforeClass for all of that and I am trying to find a way to implement it with testNG and no luck so far. Could you pls advise?

You could build a base class that leverages the IConfigurable interface and within that use the AtomicBoolean constructs to build a runOnce mechanism and then have your child classes extend this base class. That should do.

Here's a sample

import java.util.concurrent.atomic.AtomicBoolean;
import org.testng.IConfigurable;
import org.testng.IConfigureCallBack;
import org.testng.ITestResult;

public class BaseClass implements IConfigurable {

  private static final AtomicBoolean runOnce = new AtomicBoolean(false);

  @Override
  public void run(IConfigureCallBack callBack, ITestResult testResult) {
    if (runOnce.compareAndSet(false, true)) {
      callBack.runConfigurationMethod(testResult);
    } else {
      //Informing TestNG that even though we wilfully skipped the execution
      // of the other configuration methods, they are still to be considered as
      // successfully run tests.
      testResult.setStatus(ITestResult.SUCCESS);
    }
  }
}
OlegKuzovkov commented 1 year ago

@OlegKuzovkov - Please remove the static classifier for your data members and your @BeforeClass and try again. You should see the expected result. Your static data members are basically causing the data to be overwritten.

@krmahadevan Done what you said, the issue is still happens. Just for clarity regarding the execution order I modified the output in each step:

@Log4j2
public class TestClass1 {

    private String str1;
    private String str2;

    @Factory(dataProvider = "parameters")
    public TestClass1(String sss, String ggg) {
        str1 = sss;
        str2 = ggg;
        log.info("Running constructor of the test class");
    }

    @DataProvider
    public static Object[][] parameters() throws Exception {
        return new Object[][]{
                { "Data 1", "Data 1 1" },
                { "Data 2", "Data 2 2" }
        };
    }

    @BeforeClass
    public void beforeClass() {
       log.info("Running BeforeClass method");
    }

    @Test
    public void test1() {
        log.info("Running test 1");
    }

    @Test
    public void test2() {
        log.info("Running test 2");
    }
}

Output is still wrong:

2023-01-11 09:28:44,949 [main] INFO  com.ctera.auto.tests.unittest.TestClass1 - Running constructor of the test class
2023-01-11 09:28:44,964 [main] INFO  com.ctera.auto.tests.unittest.TestClass1 - Running constructor of the test class
09:28:45.025 INFO  o.t.i.Utils - main - [TestNG] Running:
  C:\Users\olegk\AppData\Local\JetBrains\IdeaIC2022.3\temp-testng-customsuite.xml

2023-01-11 09:28:45,084 [main] INFO  com.ctera.auto.tests.unittest.TestClass1 - Running BeforeClass method
2023-01-11 09:28:45,130 [main] INFO  com.ctera.auto.tests.unittest.TestClass1 - Running test 1
2023-01-11 09:28:45,209 [main] INFO  com.ctera.auto.tests.unittest.TestClass1 - Running BeforeClass method
2023-01-11 09:28:45,211 [main] INFO  com.ctera.auto.tests.unittest.TestClass1 - Running test 1
2023-01-11 09:28:45,215 [main] INFO  com.ctera.auto.tests.unittest.TestClass1 - Running test 2
2023-01-11 09:28:45,219 [main] INFO  com.ctera.auto.tests.unittest.TestClass1 - Running test 2

As you can see, the second test got executed twice after the second execution of the BeforeClass method, and not executed after the first execution at all. The behavior is inconsistent and not what it should be. Test 1 and Test 2 should be executed once after each BeforeClass execution.

krmahadevan commented 1 year ago

@OlegKuzovkov

As you can see, the second test got executed twice after the second execution of the BeforeClass method, and not executed after the first execution at all.

You would need to run the tests via a suite file with the attribute group-by-instances set to true This attribute basically tells TestNG this Dear TestNG, since there are going to be more than 1 instance of the same test class, when you execute them, please group the methods such that they are run together for each instance

Here's what I am talking about.

Modified class to include the toString() so that the output is a bit more clear.

import lombok.extern.slf4j.Slf4j;
import org.testng.annotations.BeforeClass;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Factory;
import org.testng.annotations.Test;

@Slf4j
public class TestClass1 {

  private final String str1;
  private final String str2;

  @Factory(dataProvider = "parameters")
  public TestClass1(String sss, String ggg) {
    str1 = sss;
    str2 = ggg;
    log.info("Running constructor of the test class for the instance " + this);
  }

  @DataProvider
  public static Object[][] parameters() {
    return new Object[][]{
        {"Data 1", "Data 1 1"},
        {"Data 2", "Data 2 2"}
    };
  }

  @BeforeClass
  public void beforeClass() {
    log.info("Running BeforeClass method for the instance " + this);
  }

  @Test
  public void test1() {
    log.info("Running test 1 for the instance " + this);
  }

  @Test
  public void test2() {
    log.info("Running test 2 for the instance " + this);
  }

  @Override
  public String toString() {
    return "{" +
        "str1='" + str1 + '\'' +
        ", str2='" + str2 + '\'' +
        '}';
  }
}

Now create a suite xml file that looks like below:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE suite SYSTEM "https://testng.org/testng-1.0.dtd">
<suite name="my_suite">
  <test thread-count="5" name="my_test" verbose="2" group-by-instances="true">
    <classes>
      <class name="com.rationaleemotions.issue2867.TestClass1"/>
    </classes>
  </test> <!-- my_test -->
</suite> <!-- my_suite -->

Now when you run this suite xml file, you should see an output that looks like below:

INFO  com.rationaleemotions.issue2867.TestClass1  - Running BeforeClass method for the instance {str1='Data 1', str2='Data 1 1'}
INFO  com.rationaleemotions.issue2867.TestClass1  - Running test 1 for the instance {str1='Data 1', str2='Data 1 1'}
INFO  com.rationaleemotions.issue2867.TestClass1  - Running test 2 for the instance {str1='Data 1', str2='Data 1 1'}
INFO  com.rationaleemotions.issue2867.TestClass1  - Running BeforeClass method for the instance {str1='Data 2', str2='Data 2 2'}
INFO  com.rationaleemotions.issue2867.TestClass1  - Running test 1 for the instance {str1='Data 2', str2='Data 2 2'}
INFO  com.rationaleemotions.issue2867.TestClass1  - Running test 2 for the instance {str1='Data 2', str2='Data 2 2'}
OlegKuzovkov commented 1 year ago

@krmahadevan That works. Thank you!

Any reason that setting is not true by default? I can't think of a single case where this should be false to be honest Is there a way to specify this parameter in the code and not in xml suite?

krmahadevan commented 1 year ago

@OlegKuzovkov

Any reason that setting is not true by default?

I am not sure why that is so :)

Is there a way to specify this parameter in the code and not in xml suite?

Sure you can. Here's how you do it.

  1. Create an implementation of org.testng.IAlterSuiteListener that looks like below
import java.util.List;
import org.testng.IAlterSuiteListener;
import org.testng.xml.XmlSuite;

public class TestGroupingListener implements IAlterSuiteListener {

  @Override
  public void alter(List<XmlSuite> suites) {
    suites.forEach(each -> each.setGroupByInstances(true));
  }
}
  1. Create a folder named META-INF\services and inside it create a file named org.testng.ITestNGListener
  2. Add the fully qualified class name of TestGroupingListener (the class we created in (1)) into this file.

After that, TestNG should automatically start invoking your listener.

Here TestNG is using the ServiceLoader approach to automatically wire in mandatory listeners.

More information available in the documentation here

krmahadevan commented 1 year ago

@OlegKuzovkov - Let me know if we can go ahead and close this issue off.

OlegKuzovkov commented 1 year ago

@krmahadevan Yes, you can close it. Thank you