-
Notifications
You must be signed in to change notification settings - Fork 111
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
[WiP] Added and reviewed tests from @kazesberger #63
Conversation
@@ -91,4 +98,62 @@ public void testGlobalPropertiesSetWORKSPACE() throws Exception { | |||
|
|||
assertEquals(result_testGlobalVariableName, result_testJobVariableName); | |||
} | |||
|
|||
@Test | |||
public void testChangeOfGlobalPropertyGetsRecognizedWhenWithoutJobPropertyAndRunOnSlaves() throws Exception { |
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.
wow. It should be splitted to small test methods. Think now it checks EVERITHING.
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.
@lanwen Indeed. The PR is [WiP]. I thought to review this kind of things. Anyway, thanks so much.
Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests |
I think it makes sense to extract the test in any case. @recena , could you please retain the authorship of the original commit? @kazesberger or his company may have strong opinions about it. |
@oleg-nenashev I don't know what you mean. |
@recena |
@oleg-nenashev I know it. The PR title describes perfectly I'm doing. I did not use his commit because I'm inluding reviews. If you don't agree (once again), please close the PR. |
I just ask to resubmit changes with the proper author |
This PR aims to rescue interesting tests from #14 (by @kazesberger). These tests are being reviewing and documented.