webcompere / system-stubs

System Stubs - Test Doubles for Java System resources
MIT License
92 stars 12 forks source link

Upgrade and rationalize dependencies #59

Closed chadlwilson closed 1 year ago

chadlwilson commented 1 year ago
codecov-commenter commented 1 year ago

Codecov Report

Patch and project coverage have no change.

Comparison is base (7df9b91) 81.20% compared to head (23fb2a5) 81.20%.

:exclamation: Current head 23fb2a5 differs from pull request most recent head 45eeab9. Consider uploading reports for the commit 45eeab9 to get more accurate results

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the GitHub App Integration for your organization. Read more.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #59 +/- ## ========================================= Coverage 81.20% 81.20% Complexity 326 326 ========================================= Files 47 47 Lines 761 761 Branches 40 40 ========================================= Hits 618 618 Misses 129 129 Partials 14 14 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ashleyfrieze commented 1 year ago

@chadlwilson it looks like this change has come at the same time that I'm doing something contradicting it on #46

I'm ditching Mockito and using ByteBuddy instead. This fixes an issue with stubbing environment variables across threads. I've also upgraded to minimum JDK 11.

However, there may still be merit in some of the other changes you've proposed here.

My change will also fix Appveyor settings, which I notice is a blocker on this PR. It will be built in Windows JDK 11 & 17 and in Linux JDK 11.

Would you mind coming back to this after I've got #46 in, and release v2.1.0?

ashleyfrieze commented 1 year ago

@chadlwilson - reviewing the detail of this PR, it looks like I've made a bunch of similar changes in #46, but there may still be room for further upgrades.

chadlwilson commented 1 year ago

Oh, OK, fair enough.

For the dependency upgrade stuff, it can probably be split and done via a separate PR based on whether you still intend to change target to Java 11 (or not).

ashleyfrieze commented 1 year ago

@chadlwilson Version 2.1.0 is now merged. Would you like to cast your eye over main and see what you think could be changed. Some of the plugin versions might like to be bumped, and you had some idea of managing JUnit dependencies better.

Build is now Java 11

ashleyfrieze commented 1 year ago

@chadlwilson - the changes in this PR look to be valuable. I'm slightly wary of requiring consumers to upgrade to the highest possible JUnit 5. What's the lowest version that we can require them to use without compatibility issues?

chadlwilson commented 1 year ago

Hi @ashleyfrieze - seems you have done the hard work here in 2.1.0 already, so I have rebased and converted this to a general dependency cleanup and updated the description accordingly.

Note that the mockito-core runtime dependency seemed to be left behind in 2.1.0 despite it not being required. I've removed this to clean up the runtime dependencies for users.

I suppose independent of this you can close #49 now since it is no longer relevant in 2.1.0 onwards since there is no longer a mockito dependency :-)

ashleyfrieze commented 1 year ago

@chadlwilson - thanks for your input and tidying up. I'm looking forward to merging your PR when it makes the build go green. Do you think we'll need a release 2.1.1 imminently? or should these changes get mopped up in a later release?

chadlwilson commented 1 year ago

I'm slightly wary of requiring consumers to upgrade to the highest possible JUnit 5. What's the lowest version that we can require them to use without compatibility issues?

Are you able to clarify your concern here? I don't think I have changed anything that will affect consumers needing to upgrade. believe all I have done is change the "suggested" version, and version tested against to 5.10.0 but since this is a provided scope dependency on system-stubs-jupiter, I don't think this will affect anyone - users still need to bring/define their own Junit 5 version.

Do you think we'll need a release 2.1.1 imminently? or should these changes get mopped up in a later release?

Given I think your intent was to completely remove mockito as a required runtime dependency for users, I think it might be sensible to release a 2.1.1 since mockito-core is still a compile/runtime dependency of system-stubs-jupiter and system-stubs-junit4 in 2.1.0 which will still involve possible version conflicts for folks and doesn't seem to be your intention with the changes in #46?

chadlwilson commented 1 year ago

I've removed these compile+runtime dependencies via

https://github.com/webcompere/system-stubs/pull/59/files#diff-97ff3f992f6581d98977e5bb7b7414ec61ba3cabd3c3f846902b7aae12e67885R38-R42

and

https://github.com/webcompere/system-stubs/pull/59/files#diff-f1b1fede6661d7a6be30b1a2bf3a0328f2e76162c69f30a822be3ea4e9ef1797L39-L42

ashleyfrieze commented 1 year ago

@chadlwilson - looks robust to me. I've merged it. I'll drop a release soon.

ashleyfrieze commented 1 year ago

@chadlwilson - thanks for your hard work and involvement!

chadlwilson commented 1 year ago

Cool! No problem - appreciate your effort on this library 🙏

The only bit I couldn't really test was the maven-gpg-plugin for the Maven Central release, but from a previous upgrade I did on another project I don't think there is any breaking change. Obviously feel free to revert if it causes any headache.

The current dependency tree like this (from my consuming project)

|    |    +--- uk.org.webcompere:system-stubs-jupiter:2.1.0
|    |    |    +--- uk.org.webcompere:system-stubs-core:2.1.0
|    |    |    |    +--- net.bytebuddy:byte-buddy:1.14.7
|    |    |    |    \--- net.bytebuddy:byte-buddy-agent:1.14.7
|    |    |    \--- org.mockito:mockito-core:5.5.0
|    |    |         +--- net.bytebuddy:byte-buddy:1.14.6 -> 1.14.7
|    |    |         +--- net.bytebuddy:byte-buddy-agent:1.14.6 -> 1.14.7
|    |    |         \--- org.objenesis:objenesis:3.3

should hopefully become

|    |    +--- uk.org.webcompere:system-stubs-jupiter:2.1.1
|    |    |    +--- uk.org.webcompere:system-stubs-core:2.1.1
|    |    |         +--- net.bytebuddy:byte-buddy:1.14.7
|    |    |         \--- net.bytebuddy:byte-buddy-agent:1.14.7
ashleyfrieze commented 1 year ago

Hey @chadlwilson - release 2.1.1 is out - it will hit maven central when it propagates from oss.sonatype.org

I'm not presently an active user of System Stubs as I'm working on some TypeScript projects right now. So, please feel free to road test this version and feedback if something could use improvement.

chadlwilson commented 1 year ago

Thanks! All working fine with 2.1.1 for https://github.com/search?q=repo%3Agocd%2Fgocd+systemstubs&type=code (we target Java 11, but run tests with Java 17, latest mockito, latest JUnit FWIW)