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

Refactor parts of log4j-core-test to migrate to JUnit5 #3061

Draft
wants to merge 5 commits into
base: 2.x
Choose a base branch
from

Conversation

ninetteadhikari
Copy link
Contributor

Introduction

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-core-test to JUnit5.c

Please note the log4j-core-test module contains over 150 files and this PR updates the tests for 40 of them. Further PRs will be made to update the rest of the tests.

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)

@ninetteadhikari ninetteadhikari changed the title Refactor log4j-core-test to migrate to JUnit5 Refactor parts of log4j-core-test to migrate to JUnit5 Oct 7, 2024
Copy link

github-actions bot commented Oct 7, 2024

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

@vy vy added the tests Pull requests or issues related to tests label Oct 7, 2024
@vy
Copy link
Member

vy commented Oct 7, 2024

Please note the log4j-core-test module contains over 150 files and this PR updates the tests for 40 of them. Further PRs will be made to update the rest of the tests.

@ninetteadhikari, @hulkoba, thanks so much! 😍 Would it be possible to keep all JUnit 4-to-5 migration changes in this PR for log4j-core-test instead of creating more follow-up PRs?

@ninetteadhikari
Copy link
Contributor Author

Please note the log4j-core-test module contains over 150 files and this PR updates the tests for 40 of them. Further PRs will be made to update the rest of the tests.

@ninetteadhikari, @hulkoba, thanks so much! 😍 Would it be possible to keep all JUnit 4-to-5 migration changes in this PR for log4j-core-test instead of creating more follow-up PRs?

sure thing:) let me then convert this PR to draft and open it for review later once we have migrated all the files.

@ninetteadhikari ninetteadhikari marked this pull request as draft October 10, 2024 08:22
ninetteadhikari and others added 4 commits October 10, 2024 11:03
Signed-off-by: Ninette Adhikari <ninette.adhikari@gmail.com>
Signed-off-by: Ninette Adhikari <ninette.adhikari@gmail.com>
Signed-off-by: Ninette Adhikari <ninette.adhikari@gmail.com>
@ninetteadhikari
Copy link
Contributor Author

hi @vy we had a quick question, we are trying to write new extensions to replace some of the old ones and getting some errors. For example we created JndiExtension to replace the JndiRule however the build fails with this error:

Failed to execute goal biz.aQute.bnd:bnd-baseline-maven-plugin:7.0.0:baseline (check-api-compat) on project log4j-core-test: An error occurred while calculating the baseline: Baseline problems detected. See the report in /home/runner/work/logging-log4j2/logging-log4j2/log4j-core-test/target/baseline/log4j-core-test-2.25.0-SNAPSHOT.txt.

org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal biz.aQute.bnd:bnd-baseline-maven-plugin:7.0.0:baseline (check-api-compat) on project log4j-core-test: An error occurred while calculating the baseline

do we have to add anything additional to make extensions work or make changes to the build? let us know thanks!

@vy
Copy link
Member

vy commented Nov 14, 2024

@ninetteadhikari, that is an API compatibility check failure – see Fixing API compatibility check failures
.

@vy
Copy link
Member

vy commented Nov 14, 2024

we created JndiExtension to replace the JndiRule however the build fails with this error:

@ninetteadhikari, this is a breaking change to log4j-core-test. Though we are not that strict about backward compatibility when it comes to utility classes in *-test modules. You'll probably need a @BaselineIgnore("2.24.1") in package-info.java of the associated package.

@ninetteadhikari
Copy link
Contributor Author

thanks so much for the response! we'll see if we can make it work:)

@ninetteadhikari ninetteadhikari marked this pull request as ready for review November 15, 2024 12:53
@ninetteadhikari ninetteadhikari marked this pull request as draft November 15, 2024 12:57
@ninetteadhikari
Copy link
Contributor Author

Hi @vy sorry to bug you again, we tried ignoring different versions but had no luck: @BaselineIgnore("2.24.0"), @BaselineIgnore("2.24.1"), @BaselineIgnore("2.25.0")

We are still getting the same build failure. To give you a bit of a context I pushed the extension change commit here and these are the build failures we are seeing. Please let us know if you have insights into this thanks!

@vy
Copy link
Member

vy commented Nov 15, 2024

@ninetteadhikari, I think your changes need to be sync'ed with 2.x. Could you try the following, please?

  1. get fetch -p upstream (Assuming upstream points to the official apache/logging-log4j2 repository)
  2. git rebase upstream/2.x
  3. Edit log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/junit/package-info.java, set the version to 2.25.0, commit your changes
  4. Validate the fix: ./mvnw verify -DskipTests -pl :log4j-core-test
  5. git push -f

(I was able to fix the issue locally, but since you did not give us rights to edit your PRs, I could not push them.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Pull requests or issues related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants