@Factory with dataProvider changes order of iterations

[nebehr]

When a test class is instantiated by a constructor with @factory annotation.
Example:

public class TestTest {
    int num;

    @Factory(dataProvider = "data")
    public TestTest(int n) {
        num = n;
    }

    @DataProvider
    public static Object [][] data() {
        return new Object[][] {{1}, {2}, {3}, {4}};
    }

    @Test
    public void test() {
        System.out.println(num);
    }
}

Expected output would be

1
2
3
4

but real order is different upon every execution. It appears that it essentially depends on test class instance hashcodes, where the list of nodes is sorted by default toString() implementation in org.testng.internal.Graph.java:129.

If dataprovider is specified in the @test instead of @factory, the order remains correct.

[juherr]

The documentation doesn't specify anything about order and test classes are not supposed to be dependent.

IMO, it is not a bug

[cbeust]

Maybe not a bug but inconsistent since the behavior is different between
@factory and @test, so worth a look in my opinion.

Cédric

On Fri, Sep 11, 2015 at 8:26 AM, Julien Herr notifications@github.com
wrote:

The documentation doesn't specify anything about order and test classes
are not supposed to be dependent.

IMO, it is not a bug

—
Reply to this email directly or view it on GitHub
#799 (comment).

[nebehr]

It is more about the expected order of data-driven iterations rather than dependency between them. If the data comes from an external source, it is only natural for a user to expect to see the iterations in the test report be in this same order.
In any case, sorting the nodes by their string representation (of which method name is only a part) does not feel right. There probably has to be a custom Comparator somewhere.

[juherr]

Workarround: add toString() to TestTest.

The Comparator is on Graph.Node and used during Grapg#topologicalSort.

A comment says:

// Sort the nodes alphabetically to make sure that methods of the same class
// get run close to each other as much as possible

[satyasrikanth]

Hi @juherr , Can you please elaborate the workaround you have mentioned. I am also facing the same problem as mentioned above, order of instances.
When using the dataprovider for single testcase, order is good. But when using factory for set of test cases, the order is getting changed every time.

[juherr]

@satyasrikanth In fact, I've already observed some different behaviors when we implement toString on a test class. The comment from code explains it well: "Sort the nodes alphabetically".

Then, adding:

public String toString() {
  return "TestTest[" + num + "]";
}

may work (or not because I didn't test it).

[BrandonDudek]

Still an issues in v6.11.

Also, I tried adding a toString() method, and it did not give it alphabetical ordering.

[krmahadevan]

@BrandonDude - I tried the following using 6.11 and it works fine. Can you please help share a sample that is not working for you ?

package com.rationaleemotions.github.issue799;

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

public class TestTest {
    private int num;

    @Factory(dataProvider = "data")
    public TestTest(int n) {
        num = n;
    }

    @DataProvider
    public static Object[][] data() {
        return new Object[][]{{1}, {2}, {3}, {4}};
    }

    @Test
    public void test() {
        System.out.println(num);
    }

    @Override
    public String toString() {
        return Integer.toString(num);
    }
}

Output

...
... TestNG 6.11 by Cédric Beust (cedric@beust.com)
...
1
2
3
4

===============================================
Suite
Total tests run: 4, Failures: 0, Skips: 0
===============================================

[juherr]

@krmahadevan Could you try with a randomized tostring? And maybe test with a randomized itest too?

[krmahadevan]

@juherr - What are you referring to when you say "randomized toString()" ? Can you please help elaborate so that I can try accordingly.

[juherr]

@krmahadevan Sure! In fact, the order may be the expected one because of toString order.

what if:

public String toString() {
  return Integer.toString(Math.abs(num - 4)); // pseudo random
}

and same with ITest :

public String getName() {
  return "Test " + Integer.toString(Math.abs(num - 4)); // pseudo random
}

[krmahadevan]

That would obviously change the order randomly no ? Since toString() is returning a random string every time. Just curious: what are we trying to ascertain ?

[juherr]

The order is not supposed to be dependent to the test name.

[krmahadevan]

@juherr - I am still a bit confused with your expectation.

I noticed the following as implementation of org.testng.internal.Graph.Node#compareTo

@Override
public int compareTo(Node<T> o) {
  return getObject().toString().compareTo(o.getObject().toString());
}

The order is not supposed to be dependent to the test name.

Looking at the code, I think only toString() is involved here.
It looks like we are dependent on the toString() implementation of the object that's embedded within the Node object. So I don't think test name is involved here.

I was thinking that maybe we could change the compareTo() implementation to something like below ?

@Override
public int compareTo(Graph.Node<T> o) {
    if ((getObject() instanceof Comparable) && (o.getObject() instanceof Comparable) ) {
        return ((Comparable) getObject()).compareTo(o.getObject());
    }
    return getObject().toString().compareTo(o.getObject().toString());
}

Please let me know your thoughts.

[juherr]

@krmahadevan Yes, it is the cause of the issue. And I think it should be removed because I don't see the reason of it: order is managed by ordering features, and name is definitely not one of them :)

But I may miss something too ;)

[krmahadevan]

@juherr - So shall i go ahead and raise a PR which includes a check to see if Comparable can be used for this as a fix ?

[juherr]

@krmahadevan First, try to remove the comparable first, if possible.

// Sort the nodes alphabetically to make sure that methods of the same class
// get run close to each other as much as possible

IMO, it should be managed outside, like an ordering feature (which should already exist with group-by-instance).

At least, you can replace toString comparable by something more logical (maybe with something like Node<T extends Comparable<?>>)

[mmurphy58]

This seems to be a good workaround. I am running 1 suite, 1 test and using @factory and @dataProvider to spawn 90 different classes with different City names. before, I could not force the listed order. Overriding the toString() method is working. I don't understand it, but it works in 6.11.1

[JoaoGFarias]

If this warning and method still needed?

[juherr]

@JoaoGFarias I supposed we can remove it. Can you make a pull request with the toString removal?