-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Restore testnames when using suites in suite. #2712
Conversation
7ae2eae
to
3726a06
Compare
Hi Martin, I think you also need to add some info in |
3726a06
to
d9a0ec1
Compare
thanks, done |
@krmahadevan please review when you have time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't quite understood the scenario wherein this doesn't work. Can we please help add a unit test for this which explicitly tests out the fix (am guessing that there's no test right now for this which is why the build was passing even though this functionality was broken)
d9a0ec1
to
21757d6
Compare
The problem occurs when we defined suites in suites as below, then it requires the test names to exist in all subsuites, after the bug that was introduced in TestNg 7.5. but we define several testnames that can exist in any of the subsuites and only the matching tests should be exuecuted. DOCTYPE suite SYSTEM "http://testng.org/testng-1.0.dtd" >
<suite name="testsuite">
<suite-files>
<suite-file path="./subsuite1.xml" />
<suite-file path="./subsuite2.xml" />
<suite-file path="./../startrestart/subsuite3.xml" />
</suite-files>
</suite> Regarding the unit test, I will try to add one, but I did not find any unit test for the class TestNg |
21757d6
to
d107137
Compare
I added a unit test in JarFileUtilsTest.java, since I was able to reuse allot of code for a similar test. |
d107137
to
103e723
Compare
testNg.setTestNames(Arrays.asList(expectedTestNames)); | ||
testNg.setXmlPathInJar(jar.getAbsolutePath()); | ||
testNg.setTestJar(jar.getAbsolutePath()); | ||
testNg.initializeSuitesAndJarFile(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martinaldrin - I dont see any assertions. Was that intentional ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, The purpose of the test is to ensure that a exception is not thrown. I can not add any assert methods.
We just ensure that the method do not check suites in jarfiles here, because that is handled later in JarFileUtiles.
And JarFileUtiles already have unit tests to ensure that this is working as expected, the problem started to aggregate suites in the jarfile here without consider sub suites.
A very similar method exist in class: JarFileUtil method: testngXmlExistsInJar that take care of that part.
1eb708d
to
43f9b2e
Compare
43f9b2e
to
5273e25
Compare
Thank for the change! 👍 |
Fixes #2709.
Did you remember to?
CHANGES.txt
./gradlew autostyleApply
We 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.
Note: For more information on contribution guidelines please make sure you refer our Contributing section for detailed set of steps.