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

8274274: Update JUnit to version 5.8.1 #633

Closed
wants to merge 12 commits into from
Closed

Conversation

@hjohn
Copy link
Collaborator

@hjohn hjohn commented Sep 24, 2021

I've added JUnit 5 as a test dependency and made sure that the JUnit 4 tests still work. Also added a single JUnit 5 tests, and confirmed it works.

I've updated the Eclipse project file for the base module only.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/633/head:pull/633
$ git checkout pull/633

Update a local copy of the PR:
$ git checkout pull/633
$ git pull https://git.openjdk.java.net/jfx pull/633/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 633

View PR using the GUI difftool:
$ git pr show -t 633

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/633.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 24, 2021

👋 Welcome back jhendrikx! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Sep 24, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 24, 2021

@hjohn hjohn force-pushed the feature/junit5 branch from 072494e to 233fdb8 Sep 24, 2021
@hjohn
Copy link
Collaborator Author

@hjohn hjohn commented Sep 24, 2021

Sorry for the force push, forgot I already submitted it as a PR. I only removed a few extraneous dependencies that got added in the verification-metadata.xml (a few extra got added from different versions of JUnit 5 that weren't needed for the final version I'm using).

Copy link
Member

@kevinrushforth kevinrushforth left a comment

As mentioned in JBS, any new third-party libraries require prior third-party license approval. And we will need to work with you on sponsoring this (as you can't contribute any third-party code under the OCA).

Speaking of which, there are more libraries added to gradle-verification.xml than I would expect. Each one will need third-party approval, if they are required. Since this is for internal use (build / test only) and not something we redistribute, that makes it easier, although it still depends on the license for each piece.

Finally, we have some closed tests that we need to ensure aren't impacted by this, so that's one more area we need to coordinate.

We can take a look, but it won't be right away. Can you also start a thread on openjfx-dev? I'd like to gauge the level of interest in enabling JUnit 5 for new tests.

gradle/verification-metadata.xml Show resolved Hide resolved
gradle/verification-metadata.xml Show resolved Hide resolved
gradle/verification-metadata.xml Outdated Show resolved Hide resolved
gradle/verification-metadata.xml Show resolved Hide resolved
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Sep 24, 2021

/reviewers 3

@openjdk
Copy link

@openjdk openjdk bot commented Sep 24, 2021

@kevinrushforth
The number of required reviews for this PR is now set to 3 (with at least 1 of role reviewers).

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Sep 24, 2021

Sorry for the force push, forgot I already submitted it as a PR.

That's OK. I deleted the couple of comments I had added relating to blocks that disappeared.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Sep 24, 2021

Btw, I think this should be moved to a Draft PR until the third-party issues are resolved (including any needed approvals).

@hjohn hjohn changed the title 8274274: Update JavaFX test framework to JUnit 5 Draft: 8274274: Update JavaFX test framework to JUnit 5 Sep 24, 2021
@openjdk openjdk bot removed the rfr label Sep 24, 2021
@kevinrushforth kevinrushforth marked this pull request as draft Sep 25, 2021
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Sep 25, 2021

The gradle verification task failed (see the test results from GitHub Actiona). It looks like it still needs the hamcrest jars. Go ahead and add them back in (and remove the exclusion for them).

@hjohn
Copy link
Collaborator Author

@hjohn hjohn commented Sep 25, 2021

The gradle verification task failed (see the test results from GitHub Actiona). It looks like it still needs the hamcrest jars. Go ahead and add them back in (and remove the exclusion for them).

Yeah, I noticed, I couldn't get rid of them. Apparently JUnit 4 itself is depending on them now (starting from 4.10 orso) and the Vintage engine requires JUnit 4.12+, so it is actually overriding JUnit 4.8.2 to 4.13.2 and pulls in the Hamcrest jars again.

I've tried forcing JUnit to 4.8.2 and even though that works, it leads to a spam stacktrace in the logs for each test executed because it can't find something via reflection. I then tried to hide this log message, but have been unable to find any way of getting that message blocked by configuring java.util.logging that works in combination with Gradle and its daemon.

So... I've sort of think this is not a fight we can win easily and have reincluded the Hamcrest jars.

EDIT: Still something breaks, base is working but graphics isn't, will fix this locally and update in a moment.

@hjohn
Copy link
Collaborator Author

@hjohn hjohn commented Oct 12, 2021

The thread on the mailinglist to gauge interest seems to have been concluded. We got 4 responses, all in favor.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Oct 14, 2021

I'll look at getting the necessary approvals starting next week. It should be relatively straight-forward.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Oct 20, 2021

As indicated in the JBS issue, we can now proceed with this. You will need to change the title of this PR to match the updated bug title.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Oct 22, 2021

It looks like I spoke too soon. I am informed there is still another needed step.

@hjohn hjohn changed the title Draft: 8274274: Update JavaFX test framework to JUnit 5 Draft: 8274274: Update JUnit to version 5.8.1 Oct 28, 2021
@hjohn
Copy link
Collaborator Author

@hjohn hjohn commented Oct 28, 2021

I was on holidays, I've updated the title.

@hjohn hjohn changed the title Draft: 8274274: Update JUnit to version 5.8.1 8274274: Update JUnit to version 5.8.1 Oct 28, 2021
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Nov 10, 2021

All tasks needed to get third-party approval are now completed. You can move this PR back to ready and the review can proceed.

@hjohn hjohn marked this pull request as ready for review Nov 11, 2021
@openjdk openjdk bot added the rfr label Nov 11, 2021
Copy link
Member

@kevinrushforth kevinrushforth left a comment

I was able to do a build / test on my local build machine using locally cached jar files. In order for this to work, I needed to add an explicit list of dependencies in build.gradle. See comments inline.

Once you make the requested updates, I'll do a CI test build.

build.gradle Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Nov 20, 2021

I also was able to make the (minor) changes needed to our closed build to use the 4.13.2 jar files.

As a note to other committers: Given that this is a third-party update, and that I need to integrate a coordinated closed change, I will sponsor this once it is ready. Please don't sponsor it even if it appears to be waiting for a while.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Looks good with one comment inline.

modules/javafx.base/src/test/java/test/JUnit5Test.java Outdated Show resolved Hide resolved
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Nov 22, 2021

I was looking for places where JUnit 4.8.2 was still being referenced and found two more places that should be changed:

  1. buildSrc/build.gradle depends on JUnit 4.8.2 -- this should be changed to JUnit 4.13.2 (and you will need to add hamcrest)
  2. Once the above is fixed, you should be able to remove the JUnit 4.8.2 artifacts from gradle/verification-metadata.xml

@kevinrushforth kevinrushforth self-requested a review Nov 22, 2021
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Looks good. I did a CI build with the latest changes.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Nov 22, 2021

@johanvos @tiainen Can one of you also review this to ensure it causes no problems with your builds?

@kevinrushforth kevinrushforth requested review from johanvos and tiainen Nov 22, 2021
mstr2
mstr2 approved these changes Nov 23, 2021
Copy link
Collaborator

@mstr2 mstr2 left a comment

The changes look good to me. The tests work as expected.

@johanvos
Copy link
Collaborator

@johanvos johanvos commented Dec 11, 2021

This looks good. I noticed that the fix for
8276142: Update gradle to version 7.3
was not in the branch for this PR, but merging it manually (which will be done when it is merged here) worked and did not cause issues.
I'm running a few more tests, and will approve when they are all green.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 11, 2021

@hjohn This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8274274: Update JUnit to version 5.8.1

Reviewed-by: kcr, mstrauss, jvos

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 50 new commits pushed to the master branch:

  • 4f9b047: 8201538: Remove implementation support for applets from JavaFX
  • 85b8e96: 8278494: Remove .hgtags
  • 7915193: 8278425: TreeTableCellStartEditTest uses deprecated TreeTableCell methods
  • 6fd4ab6: 8191995: Regression: DatePicker must commit on focusLost
  • 5805bf8: 8276313: ScrollPane scroll delta incorrectly depends on content height
  • d3fbb51: 8276553: ListView scrollTo() is broken after fix for JDK-8089589
  • aa045c5: 8272118: ListViewSkin et al: must not cancel edit on scrolling
  • 5bd72a7: 8277457: AccessControlException: access denied ("java.net.NetPermission" "getCookieHandler")
  • 3d57213: 8277475: Update JDK_DOCS property to point to JDK 17 docs
  • b257647: 8276847: JSException: ReferenceError: Can't find variable: IntersectionObserver
  • ... and 40 more: https://git.openjdk.java.net/jfx/compare/4b9cb2106c688574ed0f6602818ee9683e802ee7...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth, @johanvos) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Dec 11, 2021
@hjohn
Copy link
Collaborator Author

@hjohn hjohn commented Dec 11, 2021

/integrate

@openjdk openjdk bot added the sponsor label Dec 11, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Dec 11, 2021

@hjohn
Your change (at version 5f4f695) is now ready to be sponsored by a Committer.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Dec 11, 2021

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Dec 11, 2021

Going to push as commit ff6e8d5.
Since your change was applied there have been 50 commits pushed to the master branch:

  • 4f9b047: 8201538: Remove implementation support for applets from JavaFX
  • 85b8e96: 8278494: Remove .hgtags
  • 7915193: 8278425: TreeTableCellStartEditTest uses deprecated TreeTableCell methods
  • 6fd4ab6: 8191995: Regression: DatePicker must commit on focusLost
  • 5805bf8: 8276313: ScrollPane scroll delta incorrectly depends on content height
  • d3fbb51: 8276553: ListView scrollTo() is broken after fix for JDK-8089589
  • aa045c5: 8272118: ListViewSkin et al: must not cancel edit on scrolling
  • 5bd72a7: 8277457: AccessControlException: access denied ("java.net.NetPermission" "getCookieHandler")
  • 3d57213: 8277475: Update JDK_DOCS property to point to JDK 17 docs
  • b257647: 8276847: JSException: ReferenceError: Can't find variable: IntersectionObserver
  • ... and 40 more: https://git.openjdk.java.net/jfx/compare/4b9cb2106c688574ed0f6602818ee9683e802ee7...master

Your commit was automatically rebased without conflicts.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 11, 2021

@kevinrushforth @hjohn Pushed as commit ff6e8d5.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

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