-
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
Fix Config invocation order for inheritance #2503
Conversation
* @param xmlTest - The {@link XmlTest} that represents the current <code>test</code> | ||
* @return - <code>true</code> if group based selection was employed and false otherwise. | ||
*/ | ||
public static boolean isGroupBasedExecution(XmlTest xmlTest) { |
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.
why static? why XmlTest as argument?
why not just xmlTest.isGroupBasedExecution()?
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 - Any specific reasons behind adding comments on a PR that is already merged ?
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 just saw it when I tried to figure out the bug that I found in #2664
String[] groupsDependedUpon = m.getGroupsDependedUpon(); | ||
if (groupsDependedUpon.length > 0) { | ||
for (String group : groupsDependedUpon) { | ||
ITestNGMethod[] methodsThatBelongToGroup = | ||
MethodGroupsHelper.findMethodsThatBelongToGroup(m, methods, group); | ||
predecessors.addAll(Arrays.asList(methodsThatBelongToGroup)); | ||
if (XmlTest.isGroupBasedExecution(xmlTest)) { | ||
String[] groupsDependedUpon = m.getGroupsDependedUpon(); | ||
if (groupsDependedUpon.length > 0) { | ||
for (String group : groupsDependedUpon) { | ||
ITestNGMethod[] methodsThatBelongToGroup = | ||
MethodGroupsHelper.findMethodsThatBelongToGroup(m, methods, group); | ||
predecessors.addAll(Arrays.asList(methodsThatBelongToGroup)); | ||
} | ||
} | ||
} | ||
|
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 guess this is the reason why dependsOnGroup and dependsOnMethods is broken in #2664
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.
Feel free to propose a fix ;)
Closes #2489
Fixes #2489
Did you remember to?
CHANGES.txt
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.