@Factory with dataProvider changes order of iterations #1454
Conversation
krmahadevan
force-pushed
the
krmahadevan-fix-799
branch
from
2a9e3f8
to
0ec3541
Compare
| @@ -242,7 +244,7 @@ protected static void addMethods(XmlClass cls, String... methods) { | |||
| } | |||
|
|
|||
| public static String getPathToResource(String fileName) { | |||
| String result = System.getProperty(TEST_RESOURCES_DIR); | |||
| String result = System.getProperty(TEST_RESOURCES_DIR,"src/test/resources"); | |||
| doAssert(actual, Arrays.asList(expectedValues)); | ||
| } | ||
|
|
||
| protected static void doAssert(List<String> actual, List<String> expected) { |
There was a problem hiding this comment.
I think AssertJ has already the expected assertion.
There was a problem hiding this comment.
I am not sure if AssertJ had it or not because am not very familiar with AssertJ. I will try and revisit it and see if I can find it. If yes, then I will address this.
| @@ -36,7 +38,8 @@ public void includedGroups() { | |||
|
|
|||
| tng.run(); | |||
|
|
|||
| assertThat(listener.getSucceedMethodNames()).containsExactly("method1", "method3"); | |||
| List<String> expected = Arrays.asList("method1", "method3"); | |||
| doAssert(listener.getSucceedMethodNames(), expected); | |||
There was a problem hiding this comment.
I'm not sure about the modification of the current tests (except if the order is wrong).
Instead, we should try to keep the previous behavior.
An option could be to introduce a new edge between tests of the same class.
There was a problem hiding this comment.
All the tests that have been modified are only making themselves agnostic to the order of the test methods. The current tests have all had this hard wired into them. The moment I removed off the ordering, the tests started breaking because now the order is all governed by how Java's reflection yields those methods.
An option could be to introduce a new edge between tests of the same class.
Where would that new option reside ? It would be good if you can please help add more context.
There was a problem hiding this comment.
the order is all governed by how Java's reflection yields those methods
Does it mean it could change with different java version? Or it is the order in the file from top to bottom?
The problem is the modification of the current behavior.
Where would that new option reside?
Just an idea: It could be something like "alphaOrder" where methods of a class will be sorted by their name and linked together.
The goal is to have a way to keep the current behavior (or have a way to restore it).
There was a problem hiding this comment.
@juherr - As per this StackOverFlow post, it looks like the order is not defined. Since the order is not defined, there's no predicting on how it could be.
Just an idea: It could be something like "alphaOrder" where methods of a class will be sorted by their name and linked together.
Sounds good. But I would be glad if you could please help call out as to where would this option/configuration reside. Is it from within the TestNG DTD or via a JVM argument or something on that lines ? The JVM argument would be the easiest to do.
@juherr - The changes to the tests have been done only to make them agnostic to the order of the method execution. The current implementation has them adhere to ordering, but since I removed the sorting and eventually the ordering, the tests started breaking because All said and done, I still have 2 tests from the parallel test execution which are breaking. I haven't been able to crack the reason of the failure. I raised this PR because that was the only way in which I was able to get travis to give me feedback. Running the build locally on my Mac was taking ~30+ minutes for the tests to run to completion (Remember I had sent you and @cbeust an email about elongated build times due to long running tests ? I still have that problem and haven't found a solution. ). So until this PR turns all the tests green, this PR is not good to be merged. |
|
I'm fine with the goal of the PR if we keep some tests that enforce the order :) About your time issue, I don't have a Mac for the moment, so I can't reproduce the issue. Maybe it is caused by gradle/gradle#736 |
@juherr - Bingo! That was the answer... I updated my |
|
I believe that was my first suggestion when you mentioned your problem, that it was probably some DNS resolution issue. Glad you figured it out. |
|
@cbeust - Yes, it was. But I didn't understand the relevance of the DNS to this issue and how to go about fixing it(my bad). That was why I was going around in circles. |
|
@juherr - I am for now closing off this PR since as we discussed this needs more work to be done. Will submit a new one once I have something concrete on this. |
|
@krmahadevan 👍 |
Closes #799
Fixes #799 .
Did you remember to?
CHANGES.txtWe encourage pull requests that:
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.