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

JUnit 5 migration for greenmail #804

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

CharlesLgn
Copy link

the goal of this PR is to migrate all test-case in JUnit 5.

GreenMailRule in the core module is however not removed, but could now be done

@marcelmay
Copy link
Member

@CharlesLgn , thx for the PR. I'll look into it latest by the weekend.

@CharlesLgn
Copy link
Author

CharlesLgn commented Oct 18, 2024

@CharlesLgn , thx for the PR. I'll look into it latest by the weekend.

@marcelmay , Perfect. don't hesitate to tell me if you want me to also remove GreenMailRule from the core module, which is deprecated and for removal for the 2.0.0

@marcelmay
Copy link
Member

@CharlesLgn , I'm still struggling to take the step and try to think about alternatives for this gordian knot.

  • It will break backwards-compatibility, which - even though noted in e.g. GreenMailRule - would postpone to a release after 2.1 (released!). Therefore, I would target release > 2.1 for this major change (and update the deprecation note on GreenMailRule etc.).
  • Moving the test classes into another module comes at a high cost. Tests should be close to the related source.
    A compromise could be moving 'integration' tests only but keep unit test (no GreenMailRule/Extension). I'm not too happy about this step.
  • Other re-structuring approaches?
    • Make greenmail-core only use Junit5 and merge with junit5 (plus remove GreenMailRule), but keep extra Junit4-Module (depending on greenmail-core)? Would junit5 vintage allow to move to junit5 Extension but keep junit4 Rule (allowing to use greenmail-core + junit4 dep w/o breaking by using exclusions/optional
    • Move completely to Junit5 for releases > 2.1 and get rid of Junit4?
    • Other options?

@CharlesLgn
Copy link
Author

@marcelmay

It will break backwards-compatibility, which - even though noted in e.g. GreenMailRule - would postpone to a release after 2.1 (released!). Therefore, I would target release > 2.1 for this major change (and update the deprecation note on GreenMailRule etc.).

Yes I know, but it seams necessary if you want to remove GreenMailRule. Another solution, to facilitate the GreenMailRule is to put it in greenmail-junit-4 in the exact same package.

with this, the backwards-compatibility will not be break. someone who used the rule will use :

<dependency>
      <groupId>com.icegreen</groupId>
      <artifactId>greenmail-junit4</artifactId>
      <version>2.1.0</version>
      <scope>test</scope>
</dependency>

and if he wants to go back, he will use :

<dependency>
      <groupId>com.icegreen</groupId>
      <artifactId>greenmail-core</artifactId>
      <version>2.0.1</version>
      <scope>test</scope>
</dependency>

the only thing we need is to move the deprecated GreenMailRule is the Junit 4 module

Moving the test classes into another module comes at a high cost. Tests should be close to the related source.
A compromise could be moving 'integration' tests only but keep unit test (no GreenMailRule/Extension). I'm not too happy about this step.

I know that is a tradeoff, but I propose this for two reason :

  • it seems to be the priviledge aproch (I look at how junit5 awnsered the problem, and they created a tests module)
  • having greenmail-core beeing agnostic of the junit version can be very usefull for future migration (another test framework ; a junit 6 version ; ...)

another alternative, is to put every test in the greenmail-junit5 modules

@CharlesLgn
Copy link
Author

@marcelmay I applied what I said. feel free to review

@CharlesLgn
Copy link
Author

hey @marcelmay could you provide me some feedback :)

@CharlesLgn
Copy link
Author

hey @marcelmay
Can I ask for a review ? :)

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

Successfully merging this pull request may close these issues.

2 participants