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

[SHIRO-776] refactor: JUnit5 Best Practices #1338

Merged
merged 1 commit into from
Mar 2, 2024

Conversation

timtebeek
Copy link
Contributor

@timtebeek timtebeek commented Mar 1, 2024

Following this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a GitHub issue filed
    for the change (usually before you start working on it). Trivial changes like typos do not
    require a GitHub issue. Your pull request should address just this issue, without pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [#XXX] - Fixes bug in SessionManager,
    where you replace #XXX with the appropriate GitHub issue. Best practice
    is to use the GitHub issue title in the pull request title and in the first line of the commit message.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • add fixes #XXX if merging the PR should close a related issue.
  • Run mvn verify to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
  • If you have a group of commits related to the same change, please squash your commits into one and force push your branch using git rebase -i.
  • Committers: Make sure a milestone is set on the PR

Trivial changes like typos do not require a GitHub issue (javadoc, comments...).
In this case, just format the pull request title like [DOC] - Add javadoc in SessionManager.

If this is your first contribution, you have to read the Contribution Guidelines

If your pull request is about ~20 lines of code you don't need to sign an Individual Contributor License Agreement
if you are unsure please ask on the developers list.

To make clear that you license your contribution under the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.


Hi! Noticed that the JUnit 5 migration had missed a few small steps to optimize the methods called, and sometimes swap the argument order. The compiler won't complain about such issues, but it's best to conform to JUnit 5's best practices. Hope this is helpful! It's a mostly automated change, prompted by the summons here: https://twitter.com/lobaorn/status/1763676683012317411

Tagging @bmarwell for review, since he opened

Use this link to re-run the recipe: https://app.moderne.io/recipes/builder/hBA8IygoJ?
organizationId=YzQ4NzM2ZmItNmRjOS00ZmU2LWJhOTUtNDQ4NTViYTRhNWFk

@lprimak lprimak requested a review from bmarwell March 1, 2024 23:08
@lprimak lprimak added this to the 2.0.1 milestone Mar 1, 2024
@lprimak
Copy link
Contributor

lprimak commented Mar 1, 2024

Thank you for your contribution!

@timtebeek
Copy link
Contributor Author

Thank you for your contribution!

You're welcome! There's potentially some more automated changes we can do here, but don't want to flood you with PRs. Here's another commit that can be cherry picked: main...timtebeek:shiro:refactor/migrate-to-java-11

@lprimak
Copy link
Contributor

lprimak commented Mar 1, 2024

Go ahead :). Will work through them

@bdemers
Copy link
Member

bdemers commented Mar 2, 2024

Thanks @timtebeek, this is awesome!

Do you have a public Moderne dashboard you can point us to?

@timtebeek
Copy link
Contributor Author

Thanks @timtebeek, this is awesome!

Do you have a public Moderne dashboard you can point us to?

Glad to hear! Yes you can run any of our recipes through https://app.moderne.io/marketplace, which is free for OSS.
You'll likely want to create a user organization for Shiro via de selector in the top left corner, and then select Shiro
image

Or you can select the existing Open Source organization we already have for Apache, Maven, Gradle and others.

From there you can go to our marketplace to select and run recipes; I like looking over our best practice recipes. Here's the typical list of recipes to run when first starting out. All of those should complete in about a minute for Shiro, and from there you can then create PRs as I did here.

Some recipes also come with dedicated visualizations, which we're wrapping up into a dashboard of insights and fixes to announce coming week. I'll share a link when the blog is out! :)

@timtebeek
Copy link
Contributor Author

As an example of what you could do next, I see you're right now using assertj for one single test:
https://github.com/search?q=repo%3Aapache%2Fshiro%20assertj&type=code

If you were to want to switch over to AssertJ we have a recipe that should do most of the work for you:
https://app.moderne.io/recipes/org.openrewrite.java.testing.assertj.Assertj

If not, then it's maybe best to phase out that single use to prevent a long period of dual use or slow adoption.

@bmarwell
Copy link
Contributor

bmarwell commented Mar 2, 2024

Ha, yes. We wanted to switch to assertj at some point, and that test is so far the only one which was added.

@lprimak lprimak merged commit d8014ef into apache:main Mar 2, 2024
22 checks passed
@timtebeek timtebeek deleted the refactor/j-unit-5-best-practices branch March 2, 2024 22:07
@timtebeek
Copy link
Contributor Author

Thanks @timtebeek, this is awesome!

Do you have a public Moderne dashboard you can point us to?

Yes finally I can announce our DevCenter, as posted on our blog:
https://www.moderne.io/blog/moderne-devcenter-dashboard-used-to-migrate-secure-visualize-large-codebases

Here's a quick preview of what's explained on the above link
image

We have this available for all of Apache, Gradle, Maven and a few others via https://app.moderne.io/devcenter
You'll want to select the corresponding organization in the selector in the top left corner, to then see the following for Apache Maven.

image

Needless to say some work to do with regards to the recent Java 17 migration effort for Apache Maven, but exited to help out! :)

@bdemers
Copy link
Member

bdemers commented Mar 5, 2024

Looks great @timtebeek ! Thanks for the link!

@timtebeek
Copy link
Contributor Author

Ha, yes. We wanted to switch to assertj at some point, and that test is so far the only one which was added.

@bdemers / @bmarwell based on this comment I've gone ahead and started a branch with a mostly automated migration to AssertJ. Feel free to turn this into a pull request! :)

main...timtebeek:shiro:refactor/assert-j-best-practices

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