Skip to content

Migrate log4j-jdbc-dbcp2 to JUnit 5 #3007

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

Merged
merged 1 commit into from
Sep 26, 2024

Conversation

ninetteadhikari
Copy link
Contributor

Hello! 👋

We are from Neighbourhoodie, the implementation partner of the STF Bug Resilience Program. This work is part of our agreed Milestone 1. Upgrade from JUnit 4 to JUnit 5. This PR migrates the tests located in log4j-jdbc-dbcp2 to JUnit5.

  • Our idea is to deliver small size PRs with the changes. If you'd like us to do it in some other way, please tell us so
  • Part of the deliverable is to transport these changes from the 2.x branch to the main branch. Should we create the PR against main once this PR is merged?

Thank you!

Checklist

  • Base your changes on 2.x branch if you are targeting Log4j 2; use main otherwise
  • ./mvnw verify succeeds (if it fails due to code formatting issues reported by Spotless, simply run ./mvnw spotless:apply and retry)
  • Non-trivial changes contain an entry file in the src/changelog/.2.x.x directory
  • Tests for the changes are provided
  • Commits are signed (optional, but highly recommended)

Copy link

Job Requested goals Build Tool Version Build Outcome Build Scan®
build-macos-latest clean install 3.9.8 Build Scan NOT_PUBLISHED
build-ubuntu-latest clean install 3.9.8 Build Scan NOT_PUBLISHED
build-windows-latest clean install 3.9.8 Build Scan NOT_PUBLISHED
Generated by gradle/develocity-actions

@ninetteadhikari
Copy link
Contributor Author

hi @grobmeier we started working on migrating modules from JUnit 4 to 5. As we continue working on other modules we wanted to get your feedback. Let us know if we are going in the right direction. Thanks!:)

@vy
Copy link
Member

vy commented Sep 26, 2024

Hey @ninetteadhikari! Thanks so much for the contribution! 😍

You're going in the right direction. Some guidelines you might benefit from:

  • Submit PRs per module
  • Make sure you read the development guide
  • Make sure ./mvnw verify -pl :<module-name> passes before submitting the PR
  • Make sure you follow our coding conventions (final modifiers for variables, etc.)
  • Make sure the visibility of test classes are sufficiently decreased as you migrate to JUnit 5 (i.e., classes and methods can be package-local instead of public)
  • If you need to rewrite assertions, prefer AssertJ [fluent API] over JUnit

[I will review the PR right away...]

@vy vy self-assigned this Sep 26, 2024
@vy vy added tests Pull requests or issues related to tests appenders Affects one or more Appender plugins appenders:JDBC Issue concerning the JDBC appender labels Sep 26, 2024
@vy vy added this to the 2.25.0 milestone Sep 26, 2024
@vy vy merged commit a7383e1 into apache:2.x Sep 26, 2024
6 of 9 checks passed
@vy vy changed the title Migrate log4j-jdbc-dbcp2 to JUnit 5 Migrate log4j-jdbc-dbcp2 to JUnit 5 Sep 26, 2024
@ninetteadhikari
Copy link
Contributor Author

ninetteadhikari commented Sep 27, 2024

Do you want this PR to be made to the main branch as well?

That will be awesome! 🤩

vy pushed a commit that referenced this pull request Sep 27, 2024
Co-authored-by: Alba Herrerias <[email protected]>
Co-authored-by: James Coglan <[email protected]>
@vy
Copy link
Member

vy commented Sep 27, 2024

Do you want this PR to be made to the main branch as well?

@ninetteadhikari, this is not needed anymore. I've ported it myself. 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
appenders:JDBC Issue concerning the JDBC appender appenders Affects one or more Appender plugins tests Pull requests or issues related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants