Enable ParallelSuiteTest #2687
Enable ParallelSuiteTest #2687
Conversation
|
Thanks for the report and the test. Do you want to try a fix? It will help us a lot. |
|
@dsankouski - The tests are currently failing mostly due to a formatter violation. Can you please take a look at this document https://github.com/cbeust/testng/blob/master/.github/CONTRIBUTING.md and fix the formatting issues and then update this PR so that we can see if the tests are all passing in CI as well ? |
dsankouski
force-pushed
the
suite_inherits_threadcount
branch
from
d1b0f43
to
c6b6cf5
Compare
|
@krmahadevan , I fixed formatting. Not sure, how to retrigger CI on draft PRs, though |
|
@juherr, yes, I'm planning to fix that :) |
|
@dsankouski - No worries. The update on the PR triggered the tests and now I see test failures. |
Presumably, test was disabled to get rid of distraction during development and was forgotten. Test was broken and commented out temporary back in 2011 in cc950e9 commit, named `Temp commit`. Commit was introduced in `AllDynamic` branch Test is green both on the base commit of AllDynamic branch (10a202a) as well as on branch merge point (b251fce) Done: - Add test: child suite should obey threadCount parameter - Move private stuff to bottom, add more details in error message
Before this, populateSuiteGraph method was ignoring any suite runner with child suites. This leads to parallel suites not being executed with `randomize suite` option enabled The method now add all nodes, including parents into the graph.
dsankouski
force-pushed
the
suite_inherits_threadcount
branch
from
c6b6cf5
to
af8c3b6
Compare
| @@ -79,10 +59,10 @@ public void suitesShouldRunInParallel4() { | |||
| 5, | |||
There was a problem hiding this comment.
This test implies excluding duplicating suite files from run, which current code DOES NOT. Should we really exclude them? Or just change test case expected results.
My opinion would be to keep this as it is, since no one complained since 2011. @krmahadevan @juherr
More details:
expectedThreadCount and expectedSuiteCount is 5 in this test.
However suite graph is created with 8 nodes, resulting in 8 suites(with duplicates) run, as follows:
suite-parallel-1.xml suite-parallel-2.xml suite-parallel-2-1.xml suite-parallel-2-2.xml
| |
| |
---------- ---------| v
v v suite-parallel-2-2-1.xml
suite-parallel-2-1.xml suite-parallel-2-2.xml
|
|
v
suite-parallel-2-2-1.xml
There was a problem hiding this comment.
@dsankouski - I dont think anyone in the real world would be trying to run the same suite multiple times by passing it multiple times. That perhaps is why no one complained. But assuming if anyone has been doing it and TestNG never complained, pruning for duplicates is going to cause complaints. So as you suggest, I also agree that we should just leave it as is (dont prune the duplicate suites), but just adjust the expectations of the test to satisfy the current behaviour.
@juherr WDYT ?
There was a problem hiding this comment.
I loved that visual representation :) Helps clarify the question a lot @dsankouski
There was a problem hiding this comment.
@juherr , pushing changed expected results solution, while waiting for you reply to check if it works and CI passes.
There was a problem hiding this comment.
I don't understand why the current expectedSuiteCount is 5.
If not 8, it should be 4, right?
There was a problem hiding this comment.
distinct suite name count is 5 in the graph: suite-parallel-1.xml suite-parallel-2.xml suite-parallel-2-1.xml suite-parallel-2-2.xml suite-parallel-2-2-1.xml
There was a problem hiding this comment.
This test implied a requirement to exclude duplicating suite files from run.
For example, for suite graph like:
suite-parallel-1.xml suite-parallel-2.xml suite-parallel-2-1.xml suite-parallel-2-2.xml
| |
| |
---------- ---------| v
v v suite-parallel-2-2-1.xml
suite-parallel-2-1.xml suite-parallel-2-2.xml
|
|
v
suite-parallel-2-2-1.xml
a test expected 5 suites and 5 threads.
However, this test was excluded from TestNG unit tests since 2011, and broken somewhere later
on the timeline. Dropping mentioned requirement (i.e. leaving things as it is now),
since there's no complains on current behaviour by adjusting test expected results, to
8 expected suites and 8 threads.
|
@dsankouski - I think we again have failures because a subsequent commit from you broke the formatting asks. Can you please fix it by running './gradlew autostyleApply' and add those changes as well ? |
testng-core/src/test/resources/parallel-suites/suite-parallel-2.xml
Outdated
Show resolved
Hide resolved
| s.setThreadCount(this.m_threadCount); | ||
| } | ||
| processParallelModeCommandLineArgs(s); | ||
| s.getChildSuites().forEach(this::processParallelModeCommandLineArgs); |
There was a problem hiding this comment.
Child suites of child suites won't be configured. But I can't remember if TestNG supports child-child-suites.
There was a problem hiding this comment.
make processParallelModeCommandLineArgs(s); recursive
- revert suite order - gradle autostyle apply - set parallel mode recursively
Presumably, test was disabled to get rid of distraction during development
and was forgotten.
Test was broken and commented out temporary back in 2011 in
cc950e9 commit, named
Temp commit.Commit was introduced in
AllDynamicbranchTest is green both on the base commit of AllDynamic branch (10a202a) as well as on branch merge point (b251fce)
Add test: child suite should obey threadCount parameter
Tests for #2686
Fixes # .
Did you remember to?
CHANGES.txt./gradlew autostyleApply