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.

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

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

Conversation

virenv
Copy link
Contributor

@virenv virenv commented Nov 25, 2015

No description provided.

@@ -269,28 +269,33 @@ public void setXmlPathInJar(String xmlPathInJar) {
public void initializeSuitesAndJarFile() {
// The Eclipse plug-in (RemoteTestNG) might have invoked this method already
// so don't initialize suites twice.
System.out.println("ENTER");
Copy link
Member

Choose a reason for hiding this comment

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

Oops, I think this line is not expected ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New to Java and git :(, just fixed it up.

Copy link
Member

Choose a reason for hiding this comment

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

:) You can try to squash all your commits into only one with git interactive rebase. It's for advanced git users but it's really useful.

//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());
Copy link
Member

Choose a reason for hiding this comment

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

Move suitePath and rootPath between the 2 for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

s.getFileName() can return null in surefire plugin suites. By putting it here we will avoid the surefire plugin case and avoid a null check.

Copy link
Member

Choose a reason for hiding this comment

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

s.getFileName() can return null in surefire plugin suites

Are we agree it is a surefire issue? Do you have an example ?

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.

@@ -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.

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

@juherr
Copy link
Member

juherr commented Nov 30, 2015

@virenv I'd like to make a release soon. Do you think we will be able to close this pull-request before?

@virenv
Copy link
Contributor Author

virenv commented Nov 30, 2015

@juherr I am little bussy till the end of this week. I think by the end of this week I should be able to send a PR. Will that be fine, when are you looking for a release?
On a separate note, we can safely say that this fix is correct based on current understanding of parse method. We can take care of the refactoring of code of parse in a the next release. I can fix that as a part of separate issue, we can merge this PR for current release. What do you say?

@juherr
Copy link
Member

juherr commented Nov 30, 2015

when are you looking for a release?

No date for the moment. We are waiting for another PR too.

We can take care of the refactoring of code of parse in a the next release.

Why not, but in my opinion there are too many open points for the moment.
Don't worry, at least we will add it in the next release.

@virenv
Copy link
Contributor Author

virenv commented Dec 6, 2015

@juherr I want to inform you that I will take some more time to fix this. I got a crazy schedule this week. Will try to push it in another 4-5 days.

@juherr
Copy link
Member

juherr commented Dec 6, 2015

Ok, don't worry. I will try to work on it too. I feel it won't be an easy fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants