@Factory with dataProvider changes order of iterations #1467

krmahadevan · 2017-07-11T18:24:43Z

Closes #799

Fixes #799 .

Did you remember to?

Add test case(s)
Update CHANGES.txt

We encourage pull requests that:

Add new features to TestNG (or)
Fix bugs in TestNG

If your pull request involves fixing SonarQube issues then we would suggest that you please discuss this with the
TestNG-dev before you spend time working on it.

Closes testng-team#799

krmahadevan · 2017-07-11T18:26:41Z

ping @juherr

juherr

It really looks promising! 👍

juherr · 2017-07-12T06:44:58Z

src/main/java/org/testng/internal/Systematiser.java

+
+import java.util.Comparator;
+
+public class Systematiser {


I'm not a huge fan of the name but I've nothing better to propose :)

juherr · 2017-07-12T06:45:17Z

src/main/java/org/testng/internal/Systematiser.java

+public class Systematiser {
+
+    private Systematiser() {
+        //Utility class. Defeat instantiation.


The class should be final too.

juherr · 2017-07-12T06:49:49Z

src/main/java/org/testng/internal/Graph.java

@@ -121,7 +122,10 @@ public void topologicalSort() {
    // Sort the nodes alphabetically to make sure that methods of the same class
    // get run close to each other as much as possible
    //
-    Collections.sort(nodes2);
+    Comparator<Node> comparator = Systematiser.getComparator();


Could you add the Comparator<Node<T>> as constructor param?

I didnt quite understand this observation @juherr . Can you please elaborate.

The comparator should be an attribute of the graph and the comparator instantiation logic should be outside.
The attribute description will be: private final Comparator<Node<T>> comparator and its usage will be Collections.sort(nodes2, comparator).

Ok, I understood. But why would the comparator instantiation be plugged into a Graph instance from outside ? The comparator instantiation is completely dependent on just a JVM argument and as such I thought we can have it just query the comparator on an On-Demand basis from Systematiser

JVM argument is just a quick workaround which should not be used in the core.
I don't like it because it is a global variable which is bad by definition.
That's why I think we should manage the feature from a global point of view but offer JVM arg to the users in order to keep API in the same state.
At the same time, if the integration of the feature modifies all the structure, we may use some temporary tricks and think about the core redesign later.

From what I have looked at, if one were to be resorting to make this available via a configuration instead of a JVM argument, this is going to require quite a few changes. Also if you look at it, a configuration would still be applicable at the entire TestNG instance level, which is kind of what the JVM argument is doing as well. Does that add a bit of clarity to your question ?

I will try to have a deeper look asap.

juherr · 2017-07-12T06:51:22Z

src/main/java/org/testng/internal/Systematiser.java

+                break;
+
+            case NONE:
+            default:


Instead of null, maybe we can use a default comparator (all params are equals)?

juherr · 2017-07-12T06:54:04Z

src/main/java/org/testng/internal/Systematiser.java

+        //Utility class. Defeat instantiation.
+    }
+
+    public static Comparator<Graph.Node> getComparator() {


Comparator<Graph.Node<ITestNGMethod>>

We cannot do this because the INSTANCES one doesn't work with a ITestNGMethod but merely works with just an Object. The ITestNGMethod based comparator also if you notice resorts to using the ITestNGMethod object iff the Node contains an ITestNGMethod object.

I don't catch it because o1.getObject().toString() will work with Node<ITestNGMethod> too.

In fact, this point is important IMO.

@juherr - Ok, but I would like to understand why does Node object have to be tied to ITestNGMethod ? There are tests for the Graph class viz., GraphTest which attempts at testing Graph behavior using Strings. If we basically tie down the Comparator to ITestNGMethod then this would cause changes to be required in the test as well and by behavior one shouldn't have to need an ITestNGMethod object to test the behavior of the Graph. That is why I didn't change it.

krmahadevan · 2017-07-12T09:22:14Z

@juherr - I have addressed some of the review comments and responded to the ones that I have not fixed. Please help take a look and let me know if this can be approved.

juherr

LGTM. Maybe we just can remove the system property from the test.

Ping @cbeust

juherr · 2017-07-15T07:56:27Z

src/test/resources/testng.xml

  <!--This test depends on System properties. So its intentionally being run at the end
  because the toggling of the system property will affect other tests -->
  <test name = "RunAtLast">
    <classes>
      <class name="test.dataprovider.issue128.DataProviderParametersMismatchTest"/>
    </classes>
  </test>
+
+  <test name="RunOrderingTest">


Now, we may add a programmatically way to configure the test without system properties.

cbeust · 2017-07-15T11:23:05Z

src/main/java/org/testng/internal/Systematiser.java

+
+import java.util.Comparator;
+
+public final class Systematiser {


We need a Javadoc for this class, especially since its name is so bizarre.

@cbeust - I have added the required javadocs as well. Can you please see if thats fine ?

If you have some other name in mind please let me know @cbeust I can rename this. I named this so, keeping in mind it helps organize methods in some fashion. But if the name is odd, I can have it named to something like ComparatorFactory or something on those lines

@factory with dataProvider changes order of iterations

13275dd

Closes testng-team#799

juherr requested changes Jul 12, 2017

View reviewed changes

Fixing review comments.

4bd8a35

Move Systematiser usage to TestNG class

66d78dc

juherr approved these changes Jul 15, 2017

View reviewed changes

cbeust reviewed Jul 15, 2017

View reviewed changes

Fixing javadocs.

9c0524d

cbeust merged commit 4c446b4 into testng-team:master Jul 15, 2017

krmahadevan deleted the fix-799 branch July 16, 2017 04:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

@Factory with dataProvider changes order of iterations #1467

@Factory with dataProvider changes order of iterations #1467

krmahadevan commented Jul 11, 2017

krmahadevan commented Jul 11, 2017

juherr left a comment

juherr Jul 12, 2017

juherr Jul 12, 2017

krmahadevan Jul 12, 2017

juherr Jul 12, 2017

krmahadevan Jul 12, 2017

juherr Jul 12, 2017

krmahadevan Jul 12, 2017 •

edited

Loading

juherr Jul 12, 2017

krmahadevan Jul 12, 2017

juherr Jul 12, 2017

juherr Jul 12, 2017

krmahadevan Jul 12, 2017

juherr Jul 12, 2017

krmahadevan Jul 12, 2017

juherr Jul 12, 2017

juherr Jul 12, 2017

krmahadevan Jul 12, 2017

krmahadevan commented Jul 12, 2017

juherr left a comment

juherr Jul 15, 2017

cbeust Jul 15, 2017

krmahadevan Jul 15, 2017

krmahadevan Jul 15, 2017


		import java.util.Comparator;

		public final class Systematiser {

@Factory with dataProvider changes order of iterations #1467

@Factory with dataProvider changes order of iterations #1467

Conversation

krmahadevan commented Jul 11, 2017

Did you remember to?

krmahadevan commented Jul 11, 2017

juherr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krmahadevan Jul 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krmahadevan commented Jul 12, 2017

juherr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krmahadevan Jul 12, 2017 •

edited

Loading