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

[FIXED JENKINS-37663] Check execution result in post as well #220

Merged
merged 3 commits into from Nov 21, 2017

Conversation

Projects
None yet
3 participants
@abayer
Copy link
Member

commented Nov 20, 2017

  • JENKINS issue(s):
  • Description:
    • The new junit step doesn't set the Run to UNSTABLE directly, it sets the context (and therefore the execution) result to UNSTABLE. We should be checking the execution result as well as the Run result.
  • Documentation changes:
    • n/a
  • Users/aliases to notify:
[FIXED JENKINS-37663] Check execution result in post as well
The new junit step doesn't set the Run to UNSTABLE directly, it sets
the context (and therefore the execution) result to UNSTABLE. We
should be checking the execution result as well as the Run result.

@abayer abayer added this to the 1.2.5 milestone Nov 20, 2017

@abayer abayer requested review from rsandell and svanoort Nov 20, 2017

@reviewbybees

This comment has been minimized.

Copy link

commented Nov 20, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@@ -38,7 +38,9 @@ import org.jenkinsci.plugins.workflow.job.WorkflowRun
public class Aborted extends BuildCondition {
@Override
public boolean meetsCondition(WorkflowRun r) {
return r.getResult() != null && r.getResult().equals(Result.ABORTED)
Result execResult = getExecutionResult(r)
return (execResult != null && execResult.equals(Result.ABORTED)) ||

This comment has been minimized.

Copy link
@rsandell

rsandell Nov 21, 2017

Member

You could clean this up a bit to make it more readable. Groovy performs a .equals bu default with the == operator. But that doesn't really matter since they should all be the same instance anyway, so an == operator should catch it in standard java as well.

return execResult == Result.ABORTED || r.getResult() == Result.ABORTED

This comment has been minimized.

Copy link
@abayer

abayer Nov 21, 2017

Author Member

Oh right, null safety for free. =)

@@ -46,11 +47,13 @@ public class Changed extends BuildCondition {
}
// If the current build's result isn't null (i.e., it's got a specified status), and it's different than the
// previous build's result, we're changed.
else if (r.getResult() != null && !prev.getResult().equals(r.getResult())) {
else if ((execResult != null && !prev.getResult().equals(execResult)) ||

This comment has been minimized.

Copy link
@rsandell

rsandell Nov 21, 2017

Member

similar here: (execResult != null && prev.result != execResult) || r.result != prev.result //r.result should always be != null

@rsandell
Copy link
Member

left a comment

🐝 but 🐜

@abayer abayer merged commit e396a2a into jenkinsci:master Nov 21, 2017

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.