testng-team / testng

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

Tests are not running in correct order if parallel="classes" is used with tests having dependsOnGroup and Priority #2621

Open DarpanSinghal opened 3 years ago

DarpanSinghal commented 3 years ago

TestNG Version

7.4.0

Expected behavior

image

Actual behavior

image

Is the issue reproducible on runner?

Test case sample

package com.org.testng7;

import org.testng.annotations.Test;

public class Testng7{

    public Testng7() {
        // TODO Auto-generated constructor stub
    }

    @Test(groups = { "g1" }, priority = 1)
    public void t1() {
        System.out.println("Testng7:test 1" + " Thread ID: " + Thread.currentThread().getId());
    }

    @Test(groups = { "g2" }, dependsOnGroups = "g1", priority = 3)
    public void t2() {
        System.out.println("Testng7:test 2" + " Thread ID: " + Thread.currentThread().getId());
    }

    @Test(groups = { "g2" }, dependsOnGroups = "g1", priority = 2)
    public void t3() {
        System.out.println("Testng7:test 3" + " Thread ID: " + Thread.currentThread().getId());
    }

    @Test(groups = { "g2" }, dependsOnGroups = "g1", priority = 5)
    public void t4() {
        System.out.println("Testng7:test 4" + " Thread ID: " + Thread.currentThread().getId());
    }

    @Test(groups = { "g2" }, priority = 4)
    public void t5() {
        System.out.println("Testng7:test 5" + " Thread ID: " + Thread.currentThread().getId());
    }
}

// Another File

package com.org.testng7;

import org.testng.annotations.Test;

import com.kronos.testng.BaseUITest;

public class Testng7_copy{

    public Testng7_copy() {
        // TODO Auto-generated constructor stub
    }

    @Test(groups = { "g1" }, priority = 1)
    public void Testng7_copyt1() {
        System.out.println("Testng7_copy:test 1" + " Thread ID: " + Thread.currentThread().getId());
    }

    @Test(groups = { "g2" }, dependsOnGroups = "g1", priority = 3)
    public void Testng7_copyt2() {
        System.out.println("Testng7_copy:test 2" + " Thread ID: " + Thread.currentThread().getId());
    }

    @Test(groups = { "g2" }, dependsOnGroups = "g1", priority = 2)
    public void Testng7_copyt3() {
        System.out.println("Testng7_copy:test 3" + " Thread ID: " + Thread.currentThread().getId());
    }

    @Test(groups = { "g2" }, dependsOnGroups = "g1", priority = 5)
    public void Testng7_copyt4() {
        System.out.println("Testng7_copy:test 4" + " Thread ID: " + Thread.currentThread().getId());
    }

    @Test(groups = { "g2" }, priority = 4)
    public void Testng7_copyt5() {
        System.out.println("Testng7_copy:test 5" + " Thread ID: " + Thread.currentThread().getId());
    }
}

//Testng.xml file

<!DOCTYPE suite SYSTEM "http://testng.org/testng-1.0.dtd" >
<suite name="Suites" parallel="classes" thread-count="2"
    configfailurepolicy="continue">

    <test name="Tests" >
        <groups>
            <run>
                <include name="g1" />
                <include name="g2" />
            </run>
        </groups>
        <classes>
            <class name="com.org.testng7.Testng7" />
            <class name="com.org.testng7.Testng7_copy" />
        </classes>

    </test>
</suite>
DarpanSinghal commented 3 years ago

Kindly have a look. Above scenario is working fine:

juherr commented 3 years ago

Is it a duplicate of #2372?

DarpanSinghal commented 3 years ago

It is not duplicate as in #2372 we are talking about dependency only but in this we have dependency & priority both.

krmahadevan commented 3 years ago

@DarpanSinghal - Is this a problem only when running tests in eclipse or is this a problem when you run your tests using a build tool (Maven or Gradle).

DarpanSinghal commented 3 years ago

I checked with eclipse only taking testng 7.4 jar as an external jar. Along with it, tested using our Automation f/w code which is based on Gradle.

krmahadevan commented 3 years ago

@DarpanSinghal - Please verify this from a command line as well (using maven and/or gradle) because I would like to confirm if this problem is associated with the eclipse plugin (From your screenshots I gather that you are basing the conclusion based on the eclipse plugin output) and want to rule out a problem with core testng itself.

So please try with maven/gradle and post back results.

DarpanSinghal commented 3 years ago

Hi @krmahadevan , Eclipse plugin was not used in this execution. I updated my previous comment. "I checked with eclipse only taking testng 7.4 jar as an external jar. Along with it, tested using our Automation f/w code which is based on Gradle."

DarpanSinghal commented 3 years ago

Kindly let me know when I can expect the fix.

krmahadevan commented 3 years ago

Below sample can be used to reproduce the problem

Test classes

import org.testng.annotations.Test;

public class TestClassSampleOne {

  @Test(groups = {"g1"}, priority = 1)
  public void t1() {
  }

  @Test(groups = {"g2"}, dependsOnGroups = "g1", priority = 3)
  public void t2() {
  }

  @Test(groups = {"g2"}, dependsOnGroups = "g1", priority = 2)
  public void t3() {
  }

  @Test(groups = {"g2"}, dependsOnGroups = "g1", priority = 5)
  public void t4() {
  }

  @Test(groups = {"g2"}, priority = 4)
  public void t5() {
  }
}
import org.testng.annotations.Test;

public class TestClassSampleTwo {

  @Test(groups = {"g1"}, priority = 1)
  public void t1() {
  }

  @Test(groups = {"g2"}, dependsOnGroups = "g1", priority = 3)
  public void t2() {
  }

  @Test(groups = {"g2"}, dependsOnGroups = "g1", priority = 2)
  public void t3() {
  }

  @Test(groups = {"g2"}, dependsOnGroups = "g1", priority = 5)
  public void t4() {
  }

  @Test(groups = {"g2"}, priority = 4)
  public void t5() {
  }
}

Listener that is used here

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import org.testng.IExecutionListener;
import org.testng.IInvokedMethod;
import org.testng.IInvokedMethodListener;
import org.testng.ITestResult;
import org.testng.internal.Version;

public class TestNarrator implements IInvokedMethodListener, IExecutionListener {

  private final List<String> logs = Collections.synchronizedList(new ArrayList<>());

  @Override
  public void onExecutionStart() {
    System.err.println("TestNG version : " + Version.VERSION);
  }

  @Override
  public void onExecutionFinish() {

  }

  @Override
  public void beforeInvocation(IInvokedMethod method, ITestResult testResult) {
    int priority = method.getTestMethod().getPriority();
    String[] groups = method.getTestMethod().getGroups();
    String msg = String.format("{method=[%s],groups=%s,priority=[%d]}",
        method.getTestMethod().getMethodName(),
        Arrays.toString(groups),
        priority
    );
    logs.add(msg);
  }

  @Override
  public void afterInvocation(IInvokedMethod iInvokedMethod, ITestResult iTestResult) {

  }

  public List<String> getLogs() {
    return logs;
  }
}

Test Runner

import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import static org.assertj.core.api.Assertions.assertThat;
import org.testng.ITestNGListener;
import org.testng.TestNG;
import org.testng.annotations.Test;
import org.testng.xml.XmlClass;
import org.testng.xml.XmlGroups;
import org.testng.xml.XmlRun;
import org.testng.xml.XmlSuite;
import org.testng.xml.XmlSuite.ParallelMode;
import org.testng.xml.XmlTest;

public class TestCase {

  @Test
  public void runTestCase() {
    XmlSuite xmlSuite = new XmlSuite();
    xmlSuite.setParallel(ParallelMode.CLASSES);
    xmlSuite.setThreadCount(2);
    xmlSuite.setName("2621_suite");
    XmlTest xmlTest = new XmlTest(xmlSuite);
    xmlTest.setName("2621_test");
    List<XmlClass> xmlClasses = Arrays.asList(
        new XmlClass(TestClassSampleOne.class),
        new XmlClass(TestClassSampleTwo.class)
    );
    xmlTest.setXmlClasses(xmlClasses);
    XmlGroups xmlGroups = new XmlGroups();
    XmlRun xmlRun = new XmlRun();
    xmlRun.onInclude("g1");
    xmlRun.onInclude("g2");
    xmlGroups.setRun(xmlRun);
    xmlTest.setGroups(xmlGroups);
    TestNG testng = new TestNG();
    testng.setXmlSuites(Collections.singletonList(xmlSuite));
    TestNarrator listener = new TestNarrator();
    testng.addListener((ITestNGListener) listener);
    testng.run();
    String[] expectedLogs = new String[]{
        "{method=[t1],groups=[g1],priority=[1]}",
        "{method=[t1],groups=[g1],priority=[1]}",
        "{method=[t3],groups=[g2],priority=[2]}",
        "{method=[t3],groups=[g2],priority=[2]}",
        "{method=[t2],groups=[g2],priority=[3]}",
        "{method=[t2],groups=[g2],priority=[3]}",
        "{method=[t5],groups=[g2],priority=[4]}",
        "{method=[t5],groups=[g2],priority=[4]}",
        "{method=[t4],groups=[g2],priority=[5]}",
        "{method=[t4],groups=[g2],priority=[5]}"
    };
    assertThat(listener.getLogs()).containsExactly(expectedLogs);
  }
}
krmahadevan commented 3 years ago

@DarpanSinghal - I spent some more time on this, and to me this looks like TestNG 7.4.0 is actually working fine and we may have ended up fixing a bug that was always there.

Here's my analysis.

When TestNG forms the Directed acyclic graph, it identifies first the below set of methods as eligible for execution. TestClassSampleOne.t1 (belongs to g1 and has p1 priority) TestClassSampleTwo.t1 (belongs to g1 and has p2 priority) TestClassSampleOne.t5 (belongs to g2 and has p4 priority) TestClassSampleTwo.t5 (belongs to g2 and has p4 priority)

After the above 4 get executed, the next batch that becomes eligible for execution are

TestClassSampleOne.t3 (belongs to g2 and has p2 priority) TestClassSampleTwo.t3 (belongs to g2 and has p2 priority)

TestClassSampleOne.t2 (belongs to g2 and has p3 priority) TestClassSampleTwo.t2 (belongs to g2 and has p3 priority)

TestClassSampleOne.t4 (belongs to g2 and has p5 priority) TestClassSampleTwo.t4 (belongs to g2 and has p5 priority)

So here's what the output would be

{method=[t1],groups=[g1],priority=[1]}
{method=[t5],groups=[g2],priority=[4]}
{method=[t1],groups=[g1],priority=[1]}
{method=[t5],groups=[g2],priority=[4]}

{method=[t3],groups=[g2],priority=[2]}
{method=[t2],groups=[g2],priority=[3]}
{method=[t4],groups=[g2],priority=[5]}

{method=[t3],groups=[g2],priority=[2]}
{method=[t2],groups=[g2],priority=[3]}
{method=[t4],groups=[g2],priority=[5]}

The reason why I say a bug may have gotten fixed is because t5() which belongs to g2 group should get executed in the first iteration itself, because its a independent set of methods since it doesnt have any hard dependency (dependsOnMethods|dependsOnGroups)

The below gif should re-iterate what i am saying (I have intentionally added a delay of 10 seconds between animation so that its easy to follow and find out what is going on)

output

@juherr - WDYT ?

juherr commented 3 years ago

@krmahadevan I'm not sure to understand your explanation.

The priority weight is supposed to define which edge must be followed first:

https://github.com/cbeust/testng/blob/b703fdc81a3a6f1ed3d0dee92c035ad3903178b6/testng-core/src/main/java/org/testng/TestRunner.java#L155-L161

Then, due to priority, I think the expected order should be the one you showed previously:

{method=[t1],groups=[g1],priority=[1]}
{method=[t1],groups=[g1],priority=[1]}
{method=[t3],groups=[g2],priority=[2]}
{method=[t3],groups=[g2],priority=[2]}
{method=[t2],groups=[g2],priority=[3]}
{method=[t2],groups=[g2],priority=[3]}
{method=[t5],groups=[g2],priority=[4]}
{method=[t5],groups=[g2],priority=[4]}
{method=[t4],groups=[g2],priority=[5]}
{method=[t4],groups=[g2],priority=[5]}

I don't catch the reason why t5 could be run before t3

krmahadevan commented 3 years ago

@juherr

The below table should add more context

Pass Class Method Priority BelongsTo DependsOn
1 OptimusPrime t1 1 g1 Nothing
1 BumbleBee t1 1 g1 Nothing
2 OptimusPrime t5 4 g2 Nothing
2 BumbleBee t5 4 g2 Nothing
3 OptimusPrime t3 2 g2 g1
3 BumbleBee t3 2 g2 g1
4 OptimusPrime t2 3 g2 g1
4 BumbleBee t2 3 g2 g1
5 OptimusPrime t4 5 g2 g1
5 BumbleBee t4 5 g2 g1

I don't catch the reason why t5 could be run before t3

That is because t5 does not have dependsOn, and t3 has dependsOn.

So when we build the graph and ask for freenodes, t3 will not be given, because it has upstream nodes that need to run. On the other hand, t5 does not have any upstream dependencies and so it can be executed immediately.

I believe that the current functionality is correct. The core concept is that independent methods will run first and only then dependent methods would run. So t3 can never run before t1 and t5 have run to completion, because they are the upstream dependencies for t3

juherr commented 3 years ago

But priority is supposed to be a dependency too like a virtual group.

If I remember well, we only removed the priority edges in the past because it generated issues.

WDYT?

krmahadevan commented 3 years ago

Priority is like a soft dependency (An upstream failure will not cause a downstream test to be skipped)

But dependsOn is like a hard dependency (An upstream failure will cause downstream tests to be skipped)

That's why I was saying that the behaviour is correct. Priority has a say when there are no dependencies. Basically one should not mix hard and soft dependencies (which is what is happening here)

DarpanSinghal commented 3 years ago

Thanks for sharing your thoughts but TestNG 6.14.3 is working fine and execution order is as expected. If the principal of hard & soft dependencies is same then why result is different in TestNG 6.14.3 and TestNG 7.4.0

krmahadevan commented 3 years ago

@DarpanSinghal - Like I said before, I think a bug that was always there in the system seems to have eventually gotten fixed.

juherr commented 3 years ago

@krmahadevan I agree with your definition of soft/hard dependencies but it doesn't explain why a soft dependency could not be respected.

krmahadevan commented 3 years ago

@juherr - Can you please take another look at the table i shared earlier and pls let me know if that answers your question ?

I think soft dependency can be honoured only after a hard dependency is honoured.

juherr commented 3 years ago

I don't get it.

In pass 2, why do you think t5 is a better candidate than t3?

krmahadevan commented 3 years ago

Because parallelism is set to classes. Thread count is 2. So the first 4 free nodes would be picked up for execution. And at that point in time group2 methods that have a dependency cannot still be executed since G1 is still not done with execution.

juherr commented 3 years ago

According to the doc, parallelism is not supposed to have an effect on the order:

parallel="classes": TestNG will run all the methods in the same class in the same thread, but each class will be run in a separate thread.

dependsOn is a hard dependency then t2, t3 and t4 won't be run if t1 is failing. priority is a soft dependency then t5 will be run if t1 (or any other) is failing.

Then, for me, the dependencies are:

t2(OptimusPrime)->t1(OptimusPrime): (dependsOnGroup g1)
t3(OptimusPrime)->t1(OptimusPrime): (dependsOnGroup g1)
t4(OptimusPrime)->t1(OptimusPrime): (dependsOnGroup g1)
t2(OptimusPrime)->t1(BumbleBee): (dependsOnGroup g1)
t3(OptimusPrime)->t1(BumbleBee): (dependsOnGroup g1)
t4(OptimusPrime)->t1(BumbleBee): (dependsOnGroup g1)
t3(OptimusPrime)->t1(OptimusPrime): (priority)
t2(OptimusPrime)->t3(OptimusPrime): (priority)
t5(OptimusPrime)->t2(OptimusPrime): (priority)
t4(OptimusPrime)->t5(OptimusPrime): (priority)
t3(OptimusPrime)->t1(BumbleBee): (priority)
t2(OptimusPrime)->t3(BumbleBee): (priority)
t5(OptimusPrime)->t2(BumbleBee): (priority)
t4(OptimusPrime)->t5(BumbleBee): (priority)
t2(BumbleBee)->t1(BumbleBee): (dependsOnGroup g1)
t3(BumbleBee)->t1(BumbleBee): (dependsOnGroup g1)
t4(BumbleBee)->t1(BumbleBee): (dependsOnGroup g1)
t2(BumbleBee)->t1(OptimusPrime): (dependsOnGroup g1)
t3(BumbleBee)->t1(OptimusPrime): (dependsOnGroup g1)
t4(BumbleBee)->t1(OptimusPrime): (dependsOnGroup g1)
t3(BumbleBee)->t1(BumbleBee): (priority)
t2(BumbleBee)->t3(BumbleBee): (priority)
t5(BumbleBee)->t2(BumbleBee): (priority)
t4(BumbleBee)->t5(BumbleBee): (priority)
t3(BumbleBee)->t1(OptimusPrime): (priority)
t2(BumbleBee)->t3(OptimusPrime): (priority)
t5(BumbleBee)->t2(OptimusPrime): (priority)
t4(BumbleBee)->t5(OptimusPrime): (priority)

The first method to run is t1 because it is the only one without dependency. It can be run at the same time in its dedicated thread.

Once both t1 is run, the dependencies are:

t2(OptimusPrime)->t3(OptimusPrime): (priority)
t5(OptimusPrime)->t2(OptimusPrime): (priority)
t4(OptimusPrime)->t5(OptimusPrime): (priority)
t2(OptimusPrime)->t3(BumbleBee): (priority)
t5(OptimusPrime)->t2(BumbleBee): (priority)
t4(OptimusPrime)->t5(BumbleBee): (priority)
t2(BumbleBee)->t3(BumbleBee): (priority)
t5(BumbleBee)->t2(BumbleBee): (priority)
t4(BumbleBee)->t5(BumbleBee): (priority)
t2(BumbleBee)->t3(OptimusPrime): (priority)
t5(BumbleBee)->t2(OptimusPrime): (priority)
t4(BumbleBee)->t5(OptimusPrime): (priority)

The only possible candidate is t3 because it is the only one without dependency. It can be run at the same time in its dedicated thread.

Once both t3 is run, the dependencies are:

t5(OptimusPrime)->t2(OptimusPrime): (priority)
t4(OptimusPrime)->t5(OptimusPrime): (priority)
t5(OptimusPrime)->t2(BumbleBee): (priority)
t4(OptimusPrime)->t5(BumbleBee): (priority)
t5(BumbleBee)->t2(BumbleBee): (priority)
t4(BumbleBee)->t5(BumbleBee): (priority)
t5(BumbleBee)->t2(OptimusPrime): (priority)
t4(BumbleBee)->t5(OptimusPrime): (priority)

The only possible candidate is t2 because it is the only one without dependency. It can be run at the same time in its dedicated thread.

Once both t2 is run, the dependencies are:

t4(OptimusPrime)->t5(OptimusPrime): (priority)
t4(OptimusPrime)->t5(BumbleBee): (priority)
t4(BumbleBee)->t5(BumbleBee): (priority)
t4(BumbleBee)->t5(OptimusPrime): (priority)

The only possible candidate is t5 because it is the only one without dependency. It can be run at the same time in its dedicated thread.

Once both t5 is run, the dependencies are:

The last candidate is t4 because it is the only one without dependency. And it can be run at the same time in its dedicated thread.


I know the current implementation is not that process but I think it is the one we expect.

krmahadevan commented 3 years ago

@juherr - I still fail to understand as to why do you do think that t5 cannot be considered as part of independent methods because it does not have an upward dependency. Since it doesnt have any dependsOnXX it should be considered eligible for execution.

From what I see in the code, we use priority to sort the free nodes. We apply dependsOnXX to determine free nodes to run.

If you apply this rationale, then t5 will be eligible to be executed since its part of free nodes.

juherr commented 3 years ago

From what I see in the code, we use priority to sort the free nodes.

Sure but it is not expected. See the disabled tests: https://github.com/cbeust/testng/blob/master/testng-core/src/test/java/test/priority/PriorityTest.java

HiM-BludShoT commented 3 years ago

Hi, Any idea when it is going to be fixed? Or is there any workaround for now? Thanks in advance