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

Update to JUnit 5.9 (#652 / 657) #657

Merged
merged 7 commits into from Aug 28, 2022
Merged

Conversation

Bukama
Copy link
Member

@Bukama Bukama commented Aug 13, 2022

Proposed commit message:

Update to JUnit Jupiter 5.9 (#652 / #657)

Updates the used JUnit Jupiter version

closes: #652
PR: #657

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.

@Bukama Bukama changed the title Update to JUnit 5.9 (#652 / ${pull-request}) Update to JUnit 5.9 (#652 / 657) Aug 13, 2022
@Bukama Bukama requested a review from beatngu13 August 13, 2022 13:17
@@ -1,4 +1,4 @@
junitVersion=5.8.2
junitVersion=5.9.0
Copy link
Member

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:

Copy link
Member Author

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.

Copy link
Member Author

@Bukama Bukama Aug 15, 2022

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.

@Bukama Bukama requested a review from beatngu13 August 16, 2022 17:14
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
Bukama and others added 2 commits August 17, 2022 16:54
Co-authored-by: Daniel Kraus <daniel.kraus@mailbox.org>
Co-authored-by: Daniel Kraus <daniel.kraus@mailbox.org>
@beatngu13 beatngu13 added the full-build Triggers full build suite on PR label Aug 17, 2022
@Bukama
Copy link
Member Author

Bukama commented Aug 17, 2022

Abstract entry-based extension > should not mix backups of different extensions on clear environment variable and set system property FAILED
org.junit.jupiter.api.extension.ExtensionConfigurationException: Cannot access Java runtime internals to modify environment variables. Have a look at the documentation for possible solutions: https://junit-pioneer.org/docs/environment-variables/#warnings-for-reflective-access
at app//org.junitpioneer.jupiter.EnvironmentVariableUtils.getFieldValue(EnvironmentVariableUtils.java:104)
at app//org.junitpioneer.jupiter.EnvironmentVariableUtils.setInProcessEnvironmentClass(EnvironmentVariableUtils.java:78)
at app//org.junitpioneer.jupiter.EnvironmentVariableUtils.modifyEnvironmentVariables(EnvironmentVariableUtils.java:50)
at app//org.junitpioneer.jupiter.EnvironmentVariableUtils.clear(EnvironmentVariableUtils.java:45)
at app//org.junitpioneer.jupiter.AbstractEntryBasedExtensionTests.setUp(AbstractEntryBasedExtensionTests.java:36)

    Caused by:
    java.lang.reflect.InaccessibleObjectException: Unable to make field private static final java.util.HashMap java.lang.ProcessEnvironment.theEnvironment accessible: module java.base does not "opens java.lang" to unnamed module @59d016c9
        at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:354)
        at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
        at java.base/java.lang.reflect.Field.checkCanSetAccessible(Field.java:180)
        at java.base/java.lang.reflect.Field.setAccessible(Field.java:174)
        at org.junitpioneer.jupiter.EnvironmentVariableUtils.getFieldValue(EnvironmentVariableU

@Bukama
Copy link
Member Author

Bukama commented Aug 19, 2022

@beatngu13 Do you have an idea how to implement the set environment thing without reflection so I can get rid of the error?

@beatngu13
Copy link
Member

These tests are already failing on main:

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 --adds-open, but we are still struggling with the Gradle setup.

EnvironmentVariable...Tests were already set
@Bukama
Copy link
Member Author

Bukama commented Aug 21, 2022

Disabled AbstractEntryBasedExtensionTests. The two EnvironmentVariable...Tests were already disabled (what's strange as the AbstractEntryBasedExtensionTests also belongs to these env thing. Let's see.

@Bukama
Copy link
Member Author

Bukama commented Aug 21, 2022

Blocked by #651 (Gradle 7.5) due Java 18 is not supported with 7.4:
Gradle-issue gradle/gradle#20344 is fixed with 7.5

@beatngu13
Copy link
Member

@Bukama not blocked, all builds passed (now). Feel free to merge. ✌️

@Bukama Bukama merged commit bb212c5 into junit-pioneer:main Aug 28, 2022
@Bukama Bukama deleted the bishue652_junit59 branch August 28, 2022 07:21
Bukama added a commit to Bukama/junit-pioneer that referenced this pull request Aug 28, 2022
Bukama added a commit to Bukama/junit-pioneer that referenced this pull request Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full-build Triggers full build suite on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants