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
Update to JUnit 5.9 (#652 / 657) #657
Conversation
@@ -1,4 +1,4 @@ | |||
junitVersion=5.8.2 | |||
junitVersion=5.9.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you know, this actually bumps the JUnit version we use under hood. It not just incorporates it in our build (as suggested in #652). From our contributing guidelines:
As documented Pioneer aims to use the lowest JUnit 5 version that supports Pioneer's feature set. At the same time, there is no general reason to hesitate with updating the dependency if a new feature requires a newer version or the old version has a severe bug.
Our code currently only fails with JUnit 5.9 due to test internals, not because we need a certain feature or fix. I'm cool with it, I just wonder if we can and/or want to stay on 5.8.2?
In any case, please also update the build accordingly:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are totally right (e.g. #320 was the issue about upgrading to 5.6.0). But I understood your comment in the issue (#652) as a "we need it" and therefore created this PR. I'll check if I can get the build work without updating. If not I would call a vote on next maintainer meeting about either upgrading the Jupiter version or not adding it to the build matrix to avoid failing builds.
P.S. Will update the two sections of the workflow script you've linked while doing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay in 5.9.0 the interface returns org.junit.jupiter.api.extension.ExecutableInvoker
, but in 5.8.2 the class is placed org.junit.jupiter.engine.execution.ExecutableInvoker
. So even if I "override" the getExecutableInvoker
method, it would return the false interface. Therefore I would not create a second PR with only updating the build.yml
as the 5.9 build will always fail.
Co-authored-by: Daniel Kraus <daniel.kraus@mailbox.org>
Co-authored-by: Daniel Kraus <daniel.kraus@mailbox.org>
|
@beatngu13 Do you have an idea how to implement the set environment thing without reflection so I can get rid of the error? |
These tests are already failing on https://github.com/junit-pioneer/junit-pioneer/runs/7782024824?check_suite_focus=true#step:8:61 So either you ignore it, as you didn't change anything in this regard, or you disable the test on Java 17+ (preferred by me): @EnabledForJreRange(max = JRE.JAVA_16, disabledReason = "See: https://github.com/junit-pioneer/junit-pioneer/issues/509") In #651 / #614, we try to (re)enable them with |
EnvironmentVariable...Tests were already set
Disabled |
Blocked by #651 (Gradle 7.5) due Java 18 is not supported with 7.4: |
@Bukama not blocked, all builds passed (now). Feel free to merge. ✌️ |
Updates the used JUnit Jupiter version closes: junit-pioneer#652 PR: junit-pioneer#657
Updates the used JUnit Jupiter version closes: junit-pioneer#652 PR: junit-pioneer#657
Proposed commit message:
PR checklist
The following checklist shall help the PR's author, the reviewers and maintainers to ensure the quality of this project.
It is based on our contributors guidelines, especially the "writing code" section.
It shall help to check for completion of the listed points.
If a point does not apply to the given PR's changes, the corresponding entry can be simply marked as done.