Skip to content
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.

Sign up for GitHub

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

Jump to bottom

This closes #863 Same xmlsuite is added multiple times when tests are run using TestNG.SetXmlSuites method #897

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

This closes #863 Same xmlsuite is added multiple times when tests are run using TestNG.SetXmlSuites method #897

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d627682
This closes #863 Same xmlsuite is added multiple times when tests are…
Nov 25, 2015
18f523b
This closes #863 Same xmlsuite is added multiple times when tests are…
Nov 25, 2015
3dd5f5c
Merge branch 'master' of https://github.com/toolsqa/testng
Nov 25, 2015
5e010aa
Update
Nov 25, 2015
cc5b523
Update TestNG.java
virenv Nov 25, 2015
d3712b1
Build faliure fix
Nov 25, 2015
105f3bf
Build issues fixed
Nov 25, 2015
725e888
Review comment update
Nov 25, 2015
33e5c55
Fixing build faliure
Nov 26, 2015
183b26e
adding review comment
Nov 26, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
45 changes: 25 additions & 20 deletions src/main/java/org/testng/TestNG.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@
/**
* This class is the main entry point for running tests in the TestNG framework.
* Users can create their own TestNG object and invoke it in many different
* ways:
* ways
* <ul>
* <li>On an existing testng.xml
* <li>On a synthetic testng.xml, created entirely from Java
Expand Down Expand Up @@ -275,23 +275,28 @@ public void initializeSuitesAndJarFile() {

m_isInitialized = true;
if (m_suites.size() > 0) {
//to parse the suite files (<suite-file>), if any
for (XmlSuite s: m_suites) {
for (String suiteFile : s.getSuiteFiles()) {
Path rootPath = Paths.get(s.getFileName()).getParent();
try {
Collection<XmlSuite> childSuites = getParser(rootPath.resolve(suiteFile).normalize().toString()).parse();
for (XmlSuite cSuite : childSuites){
cSuite.setParentSuite(s);
s.getChildSuites().add(cSuite);
}
} catch (ParserConfigurationException | IOException | SAXException e) {
e.printStackTrace(System.out);
}
}

}
return;
//to parse the suite files (<suite-file>), if any
for (XmlSuite s: m_suites) {
for (String suiteFile : s.getSuiteFiles()) {
Path suitePath = Paths.get(s.getFileName());
Path rootPath = suitePath.getParent();
try {
Collection<XmlSuite> childSuites = getParser(rootPath.resolve(suiteFile).normalize().toString()).parse();
for (XmlSuite cSuite : childSuites){
Path childSuite = Paths.get(cSuite.getFileName()).normalize();
Path parentSuite = Paths.get(suiteFile).normalize();
if(!childSuite.getFileName().equals(parentSuite.getFileName()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead, what do you think to change parse method to XmlSuite parse(String) (which will add children itself into the parent suite)?
And then add an util method like Collection<XmlSuite> getAllSuite(XmlSuite parent) where we need all suite of the graph.
My proposition is a bit more intrusive but sounds better IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me take a look at it tomorrow, will update you on this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two things here

  1. parse() has a contract which says it will parse the current suite and also any children that it might have. So its perfectly fine for parse method to return this
  2. Its a public method and will be kind of a change that will impact all who have used this way of running tests. Unless we are trying to add an overload of XmlSuite.parse

This was my original thinking to fix the problem. I dont think that we should try to change much if issue can be fixed with a surgical and logical code change.

If you still think that what you have suggested should be done, please explain more to me about your approach may be with some example.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you are right but nobody is supposed to use the Parser class.
And I still think the parse() contract has no sense and should parse the current suite, its children and add them instead.

BTW, at least, I suggere to add a new method that will parse children only. Currently, even the variable name is wrong: childSuites.
I think it is better to have this logic into Parser instead of TestNG.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to make sense now, Parse should parse the given XmlSuite and if it find any child it should add as the child to that suite.
However, this is only one level of nesting, what about multiple level of nesting? for eg one suite points to another suite which in turn point a one more suite file.
What I think is that it should be recursive in nature and complete tree should be built from the parent suite to all the children to any given depth.

Let me know you thoughts and let me rewrite the logic after that. However, I still think that if Parser class is not meant to be used by everyone then is this much change worth? But as its a public API, I ended up using it anyway. Same is true for other people, they might have used it with the current contract of parse(). Will that not be a breaking change for them?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, this is only one level of nesting, what about multiple level of nesting?

For sure, we need some recursive here.

I still think that if Parser class is not meant to be used by everyone then is this much change worth?

I'm agree. But we can add a new method, or an util method that will filter the existing one.
I think the parsing logic should be located in the same class.

{
cSuite.setParentSuite(s);
s.getChildSuites().add(cSuite);
}
}
} catch (ParserConfigurationException | IOException | SAXException e) {
e.printStackTrace(System.out);
}
}
}
return;
}

//
Expand Down Expand Up @@ -1012,13 +1017,13 @@ private void checkSuiteNames(List<XmlSuite> suites) {
private void checkSuiteNamesInternal(List<XmlSuite> suites, Set<String> names) {
for (XmlSuite suite : suites) {
final String name = suite.getName();

int count = 0;

String tmpName = name;
while (names.contains(tmpName)) {
tmpName = name + " (" + count++ + ")";
}

if (count > 0) {
suite.setName(tmpName);
names.add(tmpName);
Expand Down
29 changes: 29 additions & 0 deletions src/test/java/test/sanitycheck/CheckSuiteNamesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,16 @@
import org.testng.xml.XmlSuite;
import org.testng.xml.XmlTest;
import org.xml.sax.SAXException;

import test.SimpleBaseTest;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
import java.util.Set;

import javax.xml.parsers.ParserConfigurationException;

public class CheckSuiteNamesTest extends SimpleBaseTest {
Expand Down Expand Up @@ -81,4 +88,26 @@ public void checkXmlSuiteAddition() throws ParserConfigurationException, SAXExce
tng.setXmlSuites(parser.parseToList());
tng.initializeSuitesAndJarFile();
}

@Test(description = "Verify that same suite is not added multiple times")
public void validateDuplicateSuiteAddition() throws ParserConfigurationException, SAXException, IOException
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test for #829 too? We should check that it is still possible to add the same child suite many times.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same child suite will point to same tests, same tests will fail the check in checkTestNames this method. That is the confusion I have. I am seeing the exception about same test name for two tests in one suite. Lets deal with this in a seperate issue. I will open one for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me pick this up as a part of next issue. I will log one after understanding the problem in details.

{
SuiteListner suiteListner = new SuiteListner();
TestNG tng = create();
String testngXmlPath = getPathToResource("sanitycheck/test-s-b.xml");
Parser parser = new Parser(testngXmlPath);
tng.setXmlSuites(parser.parseToList());
tng.addListener(suiteListner);
tng.run();

Set<String> allSuite = new HashSet<String>();
List<XmlSuite> ranSuites = suiteListner.getAllTestSuite();

Assert.assertEquals(ranSuites.size(), 3, "Correct number of suites ran");
for(XmlSuite suite : ranSuites)
{
Assert.assertTrue(allSuite.add(suite.getFileName()),
String.format("No duplicate of suite %s added", suite.getFileName()));
}
}
}
28 changes: 28 additions & 0 deletions src/test/java/test/sanitycheck/SuiteListner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package test.sanitycheck;

import java.util.ArrayList;
import java.util.List;

import org.testng.ISuite;
import org.testng.ISuiteListener;
import org.testng.xml.XmlSuite;

public class SuiteListner implements ISuiteListener {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: SuiteListener


List<XmlSuite> allSuite = new ArrayList<XmlSuite>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer private & final when possible. But it's clearly my personal taste.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Private should be the right modifier. In Java its public by default. I think final is not necessary because we wont be changing the reference of the array list to any new array list.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we wont be changing the reference of the array list to any new array list.

Agree, but final will warn you if you try ;)
But as I said, it's a personal taste, and I'm ok to not have it in a test class :)


@Override
public void onStart(ISuite suite) {
allSuite.add(suite.getXmlSuite());
}

@Override
public void onFinish(ISuite suite) {

}

public List<XmlSuite> getAllTestSuite(){
return allSuite;
}

}