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

Jump to bottom

make additional tests compatible with Jenkins 2.307 #169

Merged
merged 2 commits into from Sep 21, 2021
Merged

make additional tests compatible with Jenkins 2.307 #169

merged 2 commits into from Sep 21, 2021

Conversation

car-roll
Copy link
Collaborator

@car-roll car-roll commented Sep 12, 2021 •
edited

additional test cleanup to support the terminology changes in 2.307
downstream of #168

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

car-roll
car-roll commented Sep 12, 2021
View reviewed changes
src/main/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepExecution.java Outdated
@@ -809,7 +809,7 @@ private static void finish(@CheckForNull final String cookie) {
env.put(COOKIE_VAR, cookie);
// Cf. CoreEnvironmentContributor:
if (exec.getOwner() instanceof MasterComputer) {
env.put("NODE_NAME", "master");
env.put("NODE_NAME", node.getSelfLabel().getName()); // mirror https://github.com/jenkinsci/jenkins/blob/89d334145d2755f74f82aad07b5df4119d7fa6ce/core/src/main/java/jenkins/model/CoreEnvironmentContributor.java#L63
Copy link
Collaborator Author

@car-roll car-roll Sep 12, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied from #168

@car-roll car-roll marked this pull request as draft September 12, 2021 06:15
@car-roll car-roll mentioned this pull request Sep 20, 2021
Merged
@car-roll car-roll marked this pull request as ready for review September 21, 2021 15:39
timja
timja approved these changes Sep 21, 2021
View reviewed changes
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢

dwnusbaum
dwnusbaum approved these changes Sep 21, 2021
View reviewed changes
Copy link
Member

@dwnusbaum dwnusbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me. I would test it with mvn clean verify -Djenkins.version=2.307 if you haven't already just to make sure there aren't any issues.

@timja
Copy link
Member

timja commented Sep 21, 2021

Looks fine to me. I would test it with mvn clean verify -Djenkins.version=2.307 if you haven't already just to make sure there aren't any issues.

I ran it locally on:

(Mac M1 ARM64)

openjdk version "11.0.12" 2021-07-20 LTS

On both current core and new core result was:

 Tests run: 79, Failures: 3, Errors: 0, Skipped: 4

[ERROR] Failures:
[ERROR]   ShellStepTest.abort:213->ensureForWhile:781 org.jenkinsci.plugins.workflow.steps.durable_task.ShellStepTest$$Lambda$448/0x0000000800d91040@7e656bc7
[ERROR]   ShellStepTest.interruptingAbortsBuild:723 unexpected build status
[ERROR] org.jenkinsci.plugins.workflow.steps.durable_task.ShellStepTest.interruptingAbortsBuildEvenWithReturnStatus

Tests failed for:

Expected: is <ABORTED>
     but: was <SUCCESS>

@car-roll
Copy link
Collaborator Author

@timja @dwnusbaum I get the same failures locally, but on CI those tests always pass. The failing tests for 2.307+ are: ShellStepTest.missingNewline, ShellStepTest.remoteConsoleNotes, ShellStepTest.remoteLogger.
I tested those under 2.307 and confirmed they failed before this PR and succeeded after.

@car-roll
Copy link
Collaborator Author

Though this issue locally feels like it's related to https://issues.jenkins.io/browse/JENKINS-65195 ?

@timja
Copy link
Member

timja commented Sep 21, 2021

Though this issue locally feels like it's related to issues.jenkins.io/browse/JENKINS-65195 ?

Could be yeah there's other process issues that aren't fixed, e.g. if I go to /restart in the browser and click restart I'll get a 'killed' message in my terminal and it doesn't restart just dies.

@car-roll car-roll merged commit 8590c45 into jenkinsci:master Sep 21, 2021
@car-roll car-roll deleted the terminology-cleanup branch September 22, 2021 03:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dwnusbaum dwnusbaum dwnusbaum approved these changes

@timja timja timja approved these changes

@rsandell rsandell Awaiting requested review from rsandell

Assignees
No one assigned
Labels
maintenance
Projects
None yet
Milestone
No milestone
3 participants
@car-roll @timja @dwnusbaum