Order for DependsOnGroups has changed after TestNg 7.4.0

[martinaldrin]

TestNG Version

7.5 not working
7.4 works

Expected behavior

s3
s1
s2

Actual behavior

s1
s2
s3

Is the issue reproducible on runner?

• Maven
• Eclipse
• IntelliJ

Test case sample

Here is an example for BeforeSuite, but the same problem exists for all configuration methods. The same issue also for dependsOnMethod

    @BeforeSuite(groups = { "g1" })
    public void s3() {
        System.out.println("s3");
    }

    @BeforeSuite(groups = { "g2" }, dependsOnGroups = "g1")
    public void s2() {
        System.out.println("s2");
    }

    @BeforeSuite(groups = { "g2" }, dependsOnGroups = "g1")
    public void s1() {
        System.out.println("s1");
    }

    @Test()
    public void test3() {
        System.out.println("test3");
    }

I have tried to find the commit where the fault was introduced, but my IDE did not like to switch gradle version and I'm new at gradle.

[martinaldrin]

After some more investigation I found the commit

#2503

[krmahadevan]

@martinaldrin - I didnt quite understand the issue here from the sample that you shared. How is the test being run? What does the suite xml file look like ?

Also can you please add the expected and the actual output as well ?

[martinaldrin]

The problems is that the sorting on depends on doesn't not work on master.
Regardless if I add depends on or not the methods are executed in alphabetical order. DependsOn only works for test annotation, not for any of the configuration methods

[krmahadevan]

@martinaldrin - The configurations are defined as being part of groups. So dependsOnGroups would work only when the group selection is enabled from the suite xml file.

So if you are running from within the IDE, then you are just running the test class without any groups. And hence the dependsOnGroups attribute is going to be ignored.

[martinaldrin]

But that is a non backward compatible change. similar to what we raised for beforeGroups.
Why should the ordering be ignored just because I don't add a filter?

[Elisedlund-ericsson]

@krmahadevan the issue is not just dependsOnGroups. its also fails for dependsOnMethods

like the following should fail:

@BeforeSuite
public void s3() {
    System.out.println("s3");
}

@BeforeSuite(dependsOnMethods= "s3")
public void s2() {
    System.out.println("s2");
}

@BeforeSuite(dependsOnMethods= "s2")
public void s1() {
    System.out.println("s1");
}

Should result in execution order: s3 s2 s1 but does not on since the commit #2503

basically dependsOnMethods and dependsOnGroups does nothing for all configuration methods right now, It has worked in all released TestNG versions

replacing "BeforeSuite"(or any other config method) with "Test" seems to work so it only an issue for configuration methods.

[krmahadevan]

But that is a non backward compatible change. similar to what we raised for beforeGroups.

The below test runs fine when executed against 6.14.3 and which uses the test class sample that you shared in this defect. So not sure I quite understand the backward compatibility being broken here.

import java.io.File;
import static org.assertj.core.api.Assertions.assertThat;
import org.testng.TestNG;
import org.testng.annotations.Test;

public class IssueTest {

  @Test
  public void testMethod() {
    TestNG testng = new TestNG();
    testng.setTestClasses(new Class[]{SampleTestClass.class});
    testng.run();
    File file = new File(
        TestNG.class.getProtectionDomain().getCodeSource().getLocation().getFile());
    assertThat(file.getName()).isEqualTo("testng-6.14.3.jar");
    String[] expected = new String[]{"s3", "s1", "s2", "test3"};
    assertThat(SampleTestClass.logs).containsExactly(expected);
  }
}

[martinaldrin]

on master it will run in following order "s1, s2, s32 instead of "s3, s1, s2"
6.14 and up 7.4 works, the fault is introduced in #2503

[martinaldrin]

Sorry if we confused you. the problem seems to only affect DependsOnGroups, not DependsOnMethod.
and the problem seems to be related to the added if statement in MethodHelper

 if (XmlTest.isGroupBasedExecution(xmlTest)) {
......
}

[krmahadevan]

@martinaldrin -

on master it will run in following order "s1, s2, s32 instead of "s3, s1, s2"

Yes, because when you run from within the IDE, you basically are not using groups.

The javadocs for dependsOnGroups states the below

  /**
   * The list of groups this method depends on. Every method member of one of these groups is
   * guaranteed to have been invoked before this method. Furthermore, if any of these methods was
   * not a SUCCESS, this test method will not be run and will be flagged as a SKIP.
   *
   * @return the value
   */

So when we are running from within the IDE, group filtering is not getting applied.

@cbeust - Am back with the same question again. I have always been under the assumption that anything that involves or is associated with groups is meant to be applicable only when running with group filtering enabled. From within IDE, thats not the case.

What should be the behaviour for dependsOnGroups attribute, when no group filtering is enabled. TestNG seems to still honour this attribute even when no groups are chosen until now (7.4.0) which IMO is a bug that has always been there. But I would like to understand what is the expected behaviour here so that we can move forward with this.

@juherr - WDYT ?

[martinaldrin]

Hi @krmahadevan
In my view, suite xml group filtering is just for include/exclude groups, in similar way as method include/exclude works.

The javadoc does not say anything about filtering in suite xml is required.

And in this case, you did not skip them, you just ignored the sorting of the configuration methods that have dependsOnGroups.

[krmahadevan]

@martinaldrin -

In my view, suite xml group filtering is just for include/exclude groups, in similar way as method include/exclude works.
The javadoc does not say anything about filtering in suite xml is required.

Just to ensure that we sort of the difference of opinions is why I am asking for inputs from @cbeust who has built TestNG so that the confusions are sorted out.

The javadoc does not say anything about filtering in suite xml is required.

The way i see this is that it is applicable only when groups are enabled. including or excluding groups is exactly that.

Lets say there are 3 methods as below :

@Test public void foo() {}

@Test (groups="a") public void bar() {}

@Test(groups="b") public void fooBar() {}

The way that I am seeing this is that,

• if a user does not mention the <groups> tag at all and just runs this test class, then all the 3 methods should be executed
• if a user uses includes or excludes tags, in the suite file, then he/she should be able to select only bar and foobar but foo() should not be chosen, because its NOT part of any groups and to me there's a distinction between being part of a group and not being part of any group. Not being part of any group IMO is not equivalent to I am part of all groups.

This is why I need this to be clarified once and for all and we add tests so that this discrepancy in terms of understanding the behaviour goes away.

There were no tests which iterates the behaviour that you are suggesting, else my earlier PR would have broken some changes. Which is what is making me wonder, are we looking at undocumented behaviour ?

[martinaldrin]

Hi @krmahadevan
I agree unit tests are lacking in this area.

But in my opinion it is not unclear, because of.

1. The sorting was working in the same way for groups and methods in earlier versions.
2. The include/exclude working in the same way for groups and methods in earlier versions. up 7.4 broken in 7.5
3. Include/Exclude of methods and groups are specified in a similar way in the suite.
4. Similarity between the annotation makes it easier to use, if you can expect similar behavior.
5. Documentation does not say anything about that groups must be defined in a suite xml.
6. A if statement was added in the generic sorting mechanism that skipped the sorting for some methods.
7. A Behavior was change, will introduce a non backward compatible change that will affect many users.

[krmahadevan]

Hi @krmahadevan I agree unit tests are lacking in this area.

But in my opinion it is not unclear, because of.

1. The sorting was working in the same way for groups and methods in earlier versions.
2. The include/exclude working in the same way for groups and methods in earlier versions. up 7.4 broken in 7.5
3. Include/Exclude of methods and groups are specified in a similar way in the suite.
4. Similarity between the annotation makes it easier to use, if you can expect similar behavior.
5. Documentation does not say anything about that groups must be defined in a suite xml.
6. A if statement was added in the generic sorting mechanism that skipped the sorting for some methods.

Like I said before, I would like to wait to hear back from @cedricrefresh before deciding on what should be the way forward. We both are basically stating two different perspectives at how the feature should work like and I dont think its going to go anywhere till we arrive at a consensus which is where I need @cedricrefresh inputs.

[martinaldrin]

Hi again,

I understand that you will sort this out first, I just wanted to give my view on it and input to the issue which I hope make it easier to to take a good decision.

After more digging, I understand that #2503 was implemented to solve issues that was introduced in #2432
#2432 did changes related to groups. In the example suite provide in #2432 no groups are used in the suite xml. So I think #2503 will also break #2403

[juherr]

If confirmed, I think the issue should be fixed when dependsOnMethods is used.

---

About groups:

@krmahadevan

I have always been under the assumption that anything that involves or is associated with groups is meant to be applicable only when running with group filtering enabled.

As a user, it took me a lot of time to understand it because it is not what I expected.
But true, that is how groups are supposed to work according to the implementation.

@Elisedlund-ericsson @martinaldrin

Guys, the feature you are asking for doesn't exist in TestNG yet and the documentation is not precise enough.
The behaviors you observe about groups are mostly side effects and can break when some other issues are fixed.
The main reason is that at runtime TestNG doesn't know what are groups of test methods.
To be honest, I feel the feature you want won't be easy to implement with the current design.

[martinaldrin]

Guys, the feature you are asking for doesn't exist in TestNG yet and the documentation is not precise enough. The behaviors you observe about groups are mostly side effects and can break when some other issues are fixed. The main reason is that at runtime TestNG doesn't know what are groups of test methods. To be honest, I feel the feature you want won't be easy to implement with the current design.

We have had this behavior us long I have used TestNg and is working up to TestNg 7.4, but the sorting was disabled in #2503. which has not yet been released. My understanding both from code and documentation is that sorting did not care about the group filtering until recently. I'm pretty sure that this change will affect plenty of users, and also #2432.

I hope the old behavior could be restored, and more unit tests could make protect this to happen again.

[martinaldrin]

If confirmed, I think the issue should be fixed when dependsOnMethods is used.

DependsOnMethod can never replace DependsOnGroup, maybe in my simple example, but not in the reality.

[juherr]

DependsOnMethod can never replace DependsOnGroup, maybe in my simple example, but not in the reality

Maybe not but they are 2 different issues and there is no debate or design problem with DependsOnMethod -> easier to fix

[martinaldrin]

The problem is all about #2432 Which change the whole behavior, saying that you can add methods with dependsOn methods that does not exists.

#2503 does not solve the issue, just ignore sorting if group in suite xml is not enabled.
I suggest another solution, instead of skipping sorting, I think the problem exists in topologicalSort in Graph.java, Where we throw an exception if we can't find a mapping group, instead we should remove the method that have an incorrect group dependency to support #2432. But In my opinion, that is very dangerous, because then we have no idea if we adding groups correctly.

So allowing #2432 is very dangerous in my opinion and should not be the default behavior, if we want that as a feature I would prefer that we add an property to support that. Because that will allow us to define group dependencies that does not exists.

[martinaldrin]

Sorry, I misunderstood #2432, that should of course work as well.
Any how I don't think #2503 solved the problem by disabling sorting.
Therefor I created a PR #2666 to restore the old behavior, but of course it will break #2432 which is as important as this issue. But I don't fully understand the code yet, so I'm not able to solve #2432 at this point.

[martinaldrin]

As I understand it When we add back the sorting based on groups we get cyclic dependencies. If the methods are spread out to different classes.

And as I understand it the problem is this method, where a a method dependency is added to solve issues that methods in base classes are executed before the methods in the test classes. And If we add group dependsOn we get duplicated dependencies which give the cyclic dependencies.
MethodInheritance.fixMethodInheritance()
m2.addMethodDependedUpon(MethodHelper.calculateMethodCanonicalName(m1));

I think the solution could be that we only add one dependency, if we have a group dependency that needs to get higher priority than the dependencies between classes.
I don't think mixing of DependsOnGroups and DependsOnMethods are well supported, but since DependsOnMethods are added internally it is hard to avoid that.

[juherr]

@martinaldrin I think you got some part of the design issue.
You can add test cases in a PR if you have some in mind ;)

[martinaldrin]

Hi @juherr, Yes I understand part of the problem now. but I have no clue about how to solve it and I guess I will need your expert help to solve it. Yes I will add more unit tests to my PR.

[martinaldrin]

But after more digging. I actually think the behavior that we try to protect in test.configuration.issue2432.IssueTest is wrong.
Test expect following behaviour:

"prepareConfig",
"uploadConfigToDatabase",
"verifyConfigurationAfterInstall",
"prepareConfigForTest1",
"test1"

I belive that following behaviour is correct:

"prepareConfig",
"prepareConfigForTest1",
"uploadConfigToDatabase",
"verifyConfigurationAfterInstall",
"test1"

both prepareConfig and prepareConfigForTest1 have the group "prepareConfig" and therefor I they should be executed first regardless of which class they are in. If we don't want that that behavior I think the groups between the classes should be different. otherwise it will not be possible to define groups into different classes.

[martinaldrin]

I created a separate PR where I use my priority implementation to handle the class hierarchy sorting.
But unit test that I showed above needs to be changed, since that will not be possible to solve, without breaking other things.

#2667

I know that this is not a perfect solution either, the best would be to have a separate priority handling for class hierarchy to not interfere with user configurations. But at least I show that this could work in a different way without affecting dependsOnGroups.

I believe that configuration like, dependsOnMethod, dependsOnGroup, Priority should have higher priority than the sorting on naming and class hierarchy.

[martinaldrin]

One more PR, showing an alternative solution where I use a different priority to not interfere with the user configurations.

#2668

So please let me know which solution you prefer and I will try to finalize that one. and divide my changes into smaller commits.

[juherr]

@martinaldrin Thanks for all your contributions.

I guess I will need your expert help to solve it.

To be honest, I've no idea how to deal with it because, except for selection, the groups feature is not yet clearly designed and may be difficult to integrate into the current base code.

[martinaldrin]

@juherr I think it works pretty well for everything except for one that was recently implemented after 7.4 release.
With my changes I think groups starts to work as it did before

I also believe that current implementation is quite dangerous, adding dependsOnMethod internally could definitely give some strange behavior since it can interfere with users configuration. Adding an internal priority would be much nicer here, since that can never interfere with the user configuration and just make a more soft sorting that users can override easily.

                  if (!equalsEffectiveClass(m1, m2) && !dependencyExists(m1, m2, methods)) {
                    Utils.log("MethodInheritance", 4, m2 + " DEPENDS ON " + m1);
                    m2.addMethodDependedUpon(MethodHelper.calculateMethodCanonicalName(m1));
                  }