-
Notifications
You must be signed in to change notification settings - Fork 122
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
[JENKINS-40109] Make compilation errors serializable #54
Conversation
@@ -1 +1 @@ | |||
buildPlugin(jenkinsVersions: [null, '2.60.3', '2.89']) | |||
buildPlugin(jenkinsVersions: [null, '2.138.1']) |
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.
Current baseline is 2.60.3, so there's no point to test against it twice, and I figured 2.89 was probably added to test against newer Jenkins versions so I went ahead and updated to the latest LTS.
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.
🤷♂️
Test failures are legitimate, probably caused by the dependency updates. I'm looking into them. |
@dwnusbaum It's the core version - needs to be |
@abayer Yeah I saw that, and tested out bumping it, but still had some issues locally (probably because I didn't clean out build artifacts correctly before updating it). I am rerunning tests locally against 2.73.x. EDIT: Seems like my first run against 2.73.x was a fluke, second run passed with no issues. |
Looks like a test not directly to this change (
I'll take a look and see what I can do to shorten it. |
@@ -49,6 +51,7 @@ | |||
|
|||
@Url("https://stackoverflow.com/a/49112612/12916") | |||
@Test public void selfTestLibraries() throws Exception { | |||
Assume.assumeFalse("File names are too long for Windows", Functions.isWindows()); |
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.
It doesn't feel right to try to nickel-and-dime the file names to save a few characters ("mp" -> "m", "branch" -> "b", "master" -> "m"), when the main issue (I think) is due to having 2 hashes included from branch-api in the path (One for ci.jenkins.io, and one for the test itself) in addition to the integer suffix on the JenkinsRule's JENKINS_HOME directory. AIUI jenkinsci/branch-api-plugin#129 might make this better.
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.
jenkinsci/plugin-pom#125 may help somewhat.
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.
3.24 now
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.
Updated and unignored the test in 1be0912. Thanks for the heads up about the parent pom change!
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.
Still failing, will probably just revert and leave the test ignored for now on Windows. It seems strange that there are nested junit
folders in the workspace, but I'm not sure if that is related to any of the recent changes: ...\target\tmp\junit5127255811482076289\junit7031506459549069085
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, please feel free to merge/release
See JENKINS-40109. Subsumes #53 (CC @rudolfwalter).
This PR uses the exception introduced in jenkinsci/workflow-cps-plugin#250 to make compilation errors thrown during shared library loading serializable. The PR also updates some dependencies to satisfy the Maven enforcer and incrementalifies the plugin.
@reviewbybees