jhannes / logevents

An easy-to-extend implementation of SLF4J with batteries included and sensible defaults
Other
42 stars 8 forks source link

Junit5 support #72

Closed norrs closed 2 years ago

norrs commented 2 years ago

Pull in junit5.x using the vintage engine to support the junit 4 engine and associated tests.

Provide Extensions for Junit5 for which was available as Rules for Junit4.

norrs commented 2 years ago

@jhannes I could move the Extension's to the junit5 module instead of upgrading junit and including the vintage engine which is able to run junit4 tests?

But would love to have a release with the extensions

norrs commented 2 years ago

This requires some more work, as all imports need to make sure they come from Junit5 and not Junit4. ie Assert vs Assertions etc. I just fixed a version internally at work which passes (copied and ported class to Kotlin), I'll come back and fix this PR after Ive solved using test containers with junit5.

Let me know if you want it into the junit5 module or not.

jhannes commented 2 years ago

This looks like a good addition and I've been looking to do this. Is there any negative interaction between junit 4 and junit 5?

I don't understand what you mean by "module". Did you have in mind a separate logevents-junit5 maven module? This would definately an easy pull request to accept, but I would prefer if the main module would work with both junit 4 and junit 5.

norrs commented 2 years ago

@jhannes For the best migration path, I would say the best would be to translate everything to jupiter APIs under junit5, then make a release with bundling junit vintage engine included.

Then do a major version bump, change to junit5 engine and remove vintage.

See the introduction here: https://junit.org/junit5/docs/current/user-guide/#overview-what-is-junit-5

In our project we used jupiter api with vintage engine and junit:junit on classpath, until we refactored away from Rule's to extensions and then removed vintage and junit:junit and added jpiter engine for pure junit5 tests.

Tho for test containers to work, we had to use the "hack" described here: https://github.com/testcontainers/testcontainers-java/issues/5404#issuecomment-1168669875 until they move to junit5. But we at least got rid of the junit4 dependency and everything is now junit5.

When I was mentioning modules, I were thinking about logevents-junit5-demo you added .. but I belive maybe the approach I wrote above is good? Because if people need to stay on junit4, they still have the option to exclude the juniper engine implementation, and pull in the vintage engine explicit together with junit5 juniper api. Vintage engine deals with briding junit4 impl with junit5 juniper api.

norrs commented 2 years ago

In other words, I can continue fixing rest of the imports etc to use the junit5 juniper engine and pull in junit5 vintage engine which is compatible with junit4, then rest of the clean up can happen in a major version bump if you follow semver-ish.

jhannes commented 2 years ago

Hope you had a good summer. I finally got around to reviewing the changes. I made some changes - for now I propose moving the junit5 support to a separate package. If you're happy with the changes, I can merge this now.

As for the major move to JUnit 5, I don't want logevents to be a library that forces its users to upgrade junit, even with the use of the vintage engine. But I'm happy with the idea of upgrading logevent's own tests to Jupiter. Does that seem like a good first step to you?

norrs commented 2 years ago

@jhannes I'm not very familiar with optional defined on a dependency inside pom.xml. But it seems like this means that users don't get junit dependency as a transitive dependency when pulling in logevents, right? Which means users who want to use the classes living within the optional package namespace needs to take care of importing their expected version of junit. That sounds like a better approach.

The important bits is that users should be able to avoid importing junit4 when using junit5 and visa versa.

Be free to squash/rebase commits before merging.

The "summer" in Trondheim consisted of :cloud_with_rain: and an angry latina who lost her first summer. Hopefully she will forgive me when we're taking holidays in September ;-)

jhannes commented 2 years ago

@norrs You're correct about optional dependencies.

I've merged the pull request now.

Hope September will prove sunny. This summer was not the best time to be in Norway.

jhannes commented 2 years ago

This pull request is released in version 0.4.1