-
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
Extracting out unrelated logic from TestNG class. #1596
Extracting out unrelated logic from TestNG class. #1596
Conversation
* Moved some of the parsing logic to the Parser class. * Moved some of the zip file parsing logic to its own class. * Added a test jar to support tests for Zip file extraction logic.
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.
Awesome change 👍
suites.add(XmlSuiteUtils.newXmlSuiteUsing(classes)); | ||
} | ||
} catch (IOException ex) { | ||
ex.printStackTrace(); |
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.
Maybe throw an TestNGException
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.
Fixed.
} | ||
suites.add(suiteToAdd); | ||
} | ||
foundTestngXml = true; |
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.
you can return directly
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.
Fixed.
import static org.assertj.core.api.Assertions.assertThat; | ||
|
||
public class JarFileUtilsTest { | ||
private static final File jar = new File("src/test/resources/testng-tests.jar"); |
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.
Note for later: the jar should/could be generated by the test (for example with https://github.com/shrinkwrap/shrinkwrap)
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.
Added a TODO
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.
LGTM.
Yup, go right ahead @krmahadevan. I noticed @juherr hadn't looked yet so I didn't want to merge it just yet but it looked good to me. |
own class.
extraction logic.
Fixes # .
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.