-
Notifications
You must be signed in to change notification settings - Fork 204
[JENKINS-49601] Fix NPE in ReplayCause when replaying Runs. fixes #1401 #1729
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
Open
rbrindl
wants to merge
4
commits into
jenkinsci:master
Choose a base branch
from
rbrindl:fix_NPE_in_ReplayCause
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
3b4f342
[JENKINS-49601] Fix NPE in ReplayCause when replaying Runs.
rbrindl e7e9f8a
Merge branch 'master' into fix_NPE_in_ReplayCause
rbrindl f1685d6
[JENKINS-49601] Remove null-check of run.getParents() because it caus…
rbrindl 92de9aa
fix spotless
rbrindl File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
71 changes: 71 additions & 0 deletions
71
plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/replay/ReplayCauseTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| package org.jenkinsci.plugins.workflow.cps.replay; | ||
|
|
||
| import static org.junit.Assert.assertNotNull; | ||
| import static org.junit.Assert.assertNull; | ||
|
|
||
| import hudson.model.CauseAction; | ||
| import hudson.model.TaskListener; | ||
| import hudson.util.StreamTaskListener; | ||
| import hudson.util.XStream2; | ||
| import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; | ||
| import org.jenkinsci.plugins.workflow.job.WorkflowJob; | ||
| import org.jenkinsci.plugins.workflow.job.WorkflowRun; | ||
| import org.junit.Rule; | ||
| import org.junit.Test; | ||
| import org.jvnet.hudson.test.JenkinsRule; | ||
|
|
||
| public class ReplayCauseTest { | ||
|
|
||
| @Rule | ||
| public JenkinsRule j = new JenkinsRule(); | ||
|
|
||
| @Test | ||
| public void replayCausePrintIsSafeBeforeOnAddedTo() throws Exception { | ||
| WorkflowJob p = j.createProject(WorkflowJob.class, "p"); | ||
| p.setDefinition(new CpsFlowDefinition("echo 'hello'", false)); | ||
| WorkflowRun original = j.assertBuildStatusSuccess(p.scheduleBuild2(0)); | ||
|
|
||
| // construct cause, don't call onAddedTo yet. | ||
| ReplayCause cause = new ReplayCause(original); | ||
| assertNull(cause.getRun()); | ||
|
|
||
| // verify print() does not throw NPE. | ||
| TaskListener listener = StreamTaskListener.fromStdout(); | ||
| cause.print(listener); | ||
|
|
||
| // verify getOriginal() returns run after onAddedTo. | ||
| original.addAction(new CauseAction(cause)); | ||
| assertNotNull(cause.getOriginal()); | ||
| } | ||
|
|
||
| @Test | ||
| public void replayCausePrintIsSafeAfterDeserializeBeforeOnLoad() throws Exception { | ||
| WorkflowJob p = j.createProject(WorkflowJob.class, "p2"); | ||
| p.setDefinition(new CpsFlowDefinition("echo 'hello'", false)); | ||
| WorkflowRun original = j.assertBuildStatusSuccess(p.scheduleBuild2(0)); | ||
|
|
||
| // Create a new run and attach the cause so run is initially set. | ||
| WorkflowRun newRun = j.assertBuildStatusSuccess(p.scheduleBuild2(0)); | ||
| ReplayCause cause = new ReplayCause(original); | ||
| newRun.addAction(new CauseAction(cause)); | ||
| assertNotNull(cause.getRun()); | ||
|
|
||
| // serialize and deserialize the cause, which should clear the transient run field. | ||
| XStream2 xs = new XStream2(); | ||
| String xml = xs.toXML(cause); | ||
|
|
||
| Object obj = xs.fromXML(xml); | ||
| if (obj instanceof ReplayCause) { | ||
| ReplayCause deserialized = (ReplayCause) obj; | ||
| assertNull(deserialized.getRun()); | ||
|
Comment on lines
+53
to
+60
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same for this. |
||
|
|
||
| // getOriginal() is safe to call and returns null until onLoad is called | ||
| TaskListener listener = StreamTaskListener.fromStdout(); | ||
| deserialized.print(listener); | ||
|
|
||
| // simulate Jenkins calling onLoad to reattach the run (Run.onload() is protected) | ||
| deserialized.onLoad(newRun); | ||
| assertNotNull(deserialized.getOriginal()); | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This is meaningless; it does not correspond to a real scenario. A cause should not be used until it is attached to something.
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 would not say its meaningless, because this is exactly what happens on our Jenkins instances about every two weeks when someone replays a job. So it IS a real scenario: there is a ReplayCause with a run that is null, and that's exactly what these tests check. I don't know (or can only speculate) why this is.
But since the constructor of ReplayCause does not set run, but only onLoad() and onAddedTo() do, there definitely is a window of opportunity in which run is null.
The same goes for the de-serialization case.
I didn't want to make this more complicated than necessary: There is a member that may be null when dereferenced, so a null-check seems to be in order. I can drop the tests, if you like.