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 #442

Merged
merged 1 commit into from
Sep 22, 2021
Merged

Remove powermock #442

merged 1 commit into from
Sep 22, 2021

Conversation

timja
Copy link
Member

@timja timja commented Sep 21, 2021

Due to not respecting module boundaries powermock does not work out of the box in Java 16+. On Java 11+ it had a poor user experience with annotations, guess work and googling required to make it work at all.

Personally I block new powermock tests and if they break I normally remove them / replace them with something else, and I don't seem to be the only one, e.g. @MarkEWaite removed them from the git plugin, jenkinsci/git-plugin#1139 (comment).

@jglick suggested removing powermock from the plugin pom as an encouragement for maintainers when they upgrade to remove their tests or look for different options

Powermock has not had a merged PR in nearly 1 year at this point.

Here's a sample of some concerning issues:

powermock/powermock#992 (comment)
powermock/powermock#1053
powermock/powermock#969

Mailing list is very in-active:
https://groups.google.com/g/powermock

Options for people coming here:

  1. Use a JenkinsRule or a RealJenkinsRule and TestExtension where necessary which will provide more realistic behaviour but slower tests
  2. Use dependency injection with the facade pattern, example: https://github.com/jenkinsci/github-checks-plugin/search?q=facade cc @uhafner
  3. Use mockito or mockito-inline (https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/Mockito.html#39), mockito can now mock static methods and final classes and it is actively maintained. Example PRs: 💣 Remove powermock jenkins#5736, [JENKINS-64803] Fix tags retrieval with folder scoped credentials git-plugin#1139
  4. Just remove the tests, often you have better things to do than fight with mocking frameworks that may or may not be adding a lot of value
  5. Stay with powermock, you'll need to just add an explicit version yourself, in the future if you wish to test against java 16+ you can configure maven-surefire plugin to add the required add-opens, example PowerMockito 2 + Java 11 causes "An illegal reflective access operation has occurred" powermock/powermock#969 (comment)

Depending on what people think we could bump this to 5.x as well, maybe a time to get something like #133 revived (but with less compatibility and hackery)?

@jglick
Copy link
Member

jglick commented Sep 21, 2021

see #441 also

@jglick
Copy link
Member

jglick commented Sep 21, 2021

we could bump this to 5.x as well

If you like. I do not personally find the numbering all that helpful. Sometimes there are changes that will break some child POMs and you need to do a bit of work to upgrade; that is what release notes are for.


@RunWith(PowerMockRunner.class)
@PrepareForTest(Jenkins.class)
@PowerMockIgnore({"com.sun.org.apache.xerces.*", "javax.xml.*", "org.xml.*"})
Copy link
Member

Choose a reason for hiding this comment

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

Ah yes this mess, best left in the rear-view mirror.

Do we care about adding a Mockito IT? To #441 I guess, so prove that the inline static stuff works?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so =/.

Seems a bit weird to add something like that to me. Powermock was more of a special case even so I'm not sure having it here was the best, it wasn't a magic fix to read it from the parent pom and it worked across java versions, you needed the above mess all over the place.

@rsandell
Copy link
Member

rsandell commented Sep 21, 2021

I am slightly skeptical to this. Yes maintaining PowerMock tests is a pita, but it is still less work for me as a maintainer to tweak the existing tests when core or whatever the test is mocking has changed that trying to rewrite them all into JenkinsRule tests. And if I where to rewrite them all then CI would fail because the unit/integration tests would take much more than an hour to run :)

And don't get me started on the enormous pita trying to upgrade the Mockito tests to the newer version of the framework 😓 If I ever get forced to do that upgrade I don't know what to do :/

@timja
Copy link
Member Author

timja commented Sep 21, 2021

And don't get me started on the enormous pita trying to upgrade the Mockito tests to the newer version of the framework 😓 If I ever get forced to do that upgrade I don't know what to do :/

This doesn't force you to upgrade 😄, you'll just need to add an explicit dependency in your plugin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants