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

💣 Remove powermock #5736

Merged
merged 5 commits into from
Sep 24, 2021
Merged

💣 Remove powermock #5736

merged 5 commits into from
Sep 24, 2021

Conversation

timja
Copy link
Member

@timja timja commented Sep 18, 2021

It doesn't work on Java 17 as far as I can tell, at least with some basic effort, and it appears to be unmaintained.

Mockito can do what it was doing generally.

some tests I couldn't get working or were too painful =/

cc @jglick

@@ -478,7 +478,7 @@ private void checkTarUntarRoundTrip(String filePrefix, long fileSize) throws Exc
}

@Test public void copyToWithPermissionSpecialPermissions() throws IOException, InterruptedException {
assumeFalse("Test uses POSIX-specific features", Functions.isWindows());
assumeFalse("Test uses POSIX-specific features", Functions.isWindows() || Platform.isDarwin());
Copy link
Member Author

Choose a reason for hiding this comment

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

fails on my mac, assume fine to skip it there

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Seems like an improvement. I would not mind deleting all the Mockito tests too, but that is another matter.

Recommend also removing the dependency management entry from the plugin POM.

@MarkEWaite
Copy link
Contributor

Recommend also removing the dependency management entry from the plugin POM.

Won't that break plugins using powermock when they upgrade to that parent POM? Or is that an intentional outcome of what you're proposing so that plugin maintainers are notified that core no longer uses powermock?

@timja
Copy link
Member Author

timja commented Sep 19, 2021

Recommend also removing the dependency management entry from the plugin POM.

Won't that break plugins using powermock when they upgrade to that parent POM? Or is that an intentional outcome of what you're proposing so that plugin maintainers are notified that core no longer uses powermock?

IIRC powermock was originally added to the plugin parent pom to try get people on at least java 11 compatible versions, although it was never really compatible with java 11 you had to add a whole bunch of hacks / workarounds to make it work.

This would be a you should really remove your powermock tests, either migrate to mockito, integration tests, facade pattern or just remove the tests entirely

@timja
Copy link
Member Author

timja commented Sep 19, 2021

I can reproduce the last failure with mvn clean install but not mvn test -Dtest=hudson.model.JobTest or in my IDE =/

[INFO]
[ERROR] Failures:
[ERROR]   JobTest.use_agent_platform_path_separator_when_contribute_path:73 The contributed PATH was not joined using the path separator defined in agent node
Expected: a string containing "/test;"
     but: was "/test:/usr/local/sbin:/Users/timja/.asdf/shims:/usr/local/opt/asdf/libexec/bin:/Users/timja/.nvm/versions/node/v14.15.4/bin:/Users/timja/.rbenv/shims:/usr/local/opt/mysql-client/bin:/usr/local/opt/ruby/bin:/Library/Java/JavaVirtualMachines/zulu-11.jdk/Contents/Home/bin:/Users/timja/bin:/usr/local/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/go/bin:/Users/timja/code/jenkins/backend-commit-history-parser/bin:/Users/timja/bin:/usr/local/opt/fzf/bin"
[INFO]

@timja timja marked this pull request as ready for review September 19, 2021 16:46
@timja timja added the skip-changelog Should not be shown in the changelog label Sep 19, 2021
@timja timja requested a review from a team September 19, 2021 16:46
@timja
Copy link
Member Author

timja commented Sep 21, 2021

Removal from the plugin pom proposed in jenkinsci/plugin-pom#442, but shouldn't affect this PR

For the record this PR changes the failing test count on Java 17:

Before:

Tests run: 20077, Failures: 8, Errors: 42, Skipped: 17

Now:

Tests run: 20101, Failures: 3, Errors: 3, Skipped: 20

Remaining failures are xstream related which will likely require surefire args changes or changing the default converter with xstream

@timja timja requested a review from a team September 21, 2021 13:18
core/pom.xml Show resolved Hide resolved
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Generally I prefer PowerMock to Mockito. I hope it is a matter of time until PowerMock is adapted though it is likely to be a breaking change too. No objections from me w.r.t this PR, +1

@oleg-nenashev oleg-nenashev added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Sep 23, 2021
@oleg-nenashev
Copy link
Member

We may merge it in 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process

@timja timja merged commit ddfacc2 into jenkinsci:master Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback skip-changelog Should not be shown in the changelog
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants