7903300: Add test library support to jtreg plugin #117

lgxbslgx · 2022-09-11T13:06:20Z

Hi all,

Currently, when we open a jtreg test file which uses the JUnit or TestNG, the plugin can't identify the related classes of the JUnit or TestNG. So the statement such as import org.junit.Test is marked as red with error message Cannot resolve symbol 'junit'.

This patch adds the jtreg libraries directory to the lib path of the Jetbrains IDEA when opening the JUnit or TestNG test files so that the Jetbrains IDEA can identify the related classes.

Thanks for taking the time to review.

Best Regards,
-- Guoxiong

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

CODETOOLS-7903300: Add test library support to jtreg plugin (Enhancement - P4)

Reviewers

Maurizio Cimadamore (@mcimadamore - Committer)
Jonathan Gibbons (@jonathan-gibbons - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jtreg.git pull/117/head:pull/117
$ git checkout pull/117

Update a local copy of the PR:
$ git checkout pull/117
$ git pull https://git.openjdk.org/jtreg.git pull/117/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 117

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jtreg/pull/117.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2022-09-11T13:07:36Z

👋 Welcome back gli! 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.

mlbridge · 2022-09-11T13:11:36Z

Webrevs

01: Full - Incremental (e0d865af)
00: Full (7703b0e9)

mcimadamore · 2022-09-15T13:37:26Z

plugins/idea/src/main/java/com/oracle/plugin/jtreg/components/JTRegFileManagerListener.java

+                    testInfo.jtregLib = library;
+                    rootModel.addLibrary(library);
+                } else if (testInfo.jtregLib != null) {
+                    rootModel.removeLibrary(testInfo.jtregLib);


I'm not sure this is logically correct. While I can understand adding jtreg/testng library to the model as soon as a new test file requiring it is opened, removing the library seems more problematic. In principle we should remove the testng/junit library from the model only if there's no open test that's a jtreg test. Which doesn't seem to be what's happening in the code.

The TestRootManager#addLibrary will record the reference count in the field TestRootManager.moduleLibRefCount. And the TestRootManager#removeLibrary only actually removes the lib from the module when the count becomes 1.

mcimadamore · 2022-09-15T13:38:55Z

Note that there is a workaround for the problem mentioned in this PR (not that I think it wouldn't be nice to fix). The IDE will typically suggest to add relevant test libraries when clicking on the light-bulb fix icon beside a TestNG/JUnit import.

lgxbslgx · 2022-09-15T14:19:46Z

Note that there is a workaround for the problem mentioned in this PR (not that I think it wouldn't be nice to fix). The IDE will typically suggest to add relevant test libraries when clicking on the light-bulb fix icon beside a TestNG/JUnit import.

Yes, I fixed the problem locally by using this way. But I think it is good to let the plugin fix it automatically, so I write this patch.

mcimadamore · 2022-09-15T15:02:35Z

plugins/idea/src/main/java/com/oracle/plugin/jtreg/util/JTRegUtils.java

+    /**
+     * Create (if not existing) and get the jtreg project library.
+     */
+    public static Library createJTRegLibrary(Project project, String oldDir, String newDir) {


Wouldn't it be better to split this into a function that creates the library, and one that updates it? I find it odd that the file listener is calling this with oldDir == newDir. Maybe just add option for oldDir to be null, so that we only drop the old root if one is given?

I adjusted the api, added update method and delegated the create method to it. And I also fixed the possible NPE and avoided the duplicated lib entries.

mcimadamore · 2022-09-15T15:51:57Z

Also, when updating the plugin, it is better to update the gradle.properties file to bump the version, and to include some notes (e.g. bug number and synopsis). This will allow upgrade to occur smoothly (and issues to be pinpointed correctly).

lgxbslgx · 2022-09-15T18:19:16Z

Also, when updating the plugin, it is better to update the gradle.properties file to bump the version, and to include some notes (e.g. bug number and synopsis). This will allow upgrade to occur smoothly (and issues to be pinpointed correctly).

I bumped the version and revised the notes.

I think the plugin should be released periodically (may be several months). But now it seems you want to release a new version one patch. I don't know the current release flow so I follow your command.

mcimadamore

Looks good

lgxbslgx · 2022-09-22T09:10:56Z

@mcimadamore Thanks for the review. Still waiting for a formal reviewer to review it.

bridgekeeper · 2023-12-04T19:39:54Z

@lgxbslgx This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

jonathan-gibbons

Endorsing @mcimadamore's review

openjdk · 2023-12-07T16:56:35Z

@lgxbslgx 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:

7903300: Add test library support to jtreg plugin

Reviewed-by: mcimadamore, jjg

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 53 new commits pushed to the master branch:

6cd72a0: 7903191: Fix build with "MAKEFLAGS=-j32".
a12c75c: 7903581: Typo in make file in pandoc binary detection
ef38655: 7903539: Please make jtreg build date reproducible
1f75b51: 7903553: Update GitHub checkout action to v4
48935be: 7903516: work around this-escape warnings
5c9d716: 7903524: Permit more segments in jtreg tags
07c6942: 7903518: Release jtreg 7.3.1
22f7332: 7903515: Some default env vars on Windows not set
b9c6dcd: 7903513: Update version to 7.4
e8a7968: 7903512: Update changelog for jtreg 7.3
... and 43 more: https://git.openjdk.org/jtreg/compare/1a03cc237fefc205a3703f8e035ebb72e75221c6...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 (@mcimadamore, @jonathan-gibbons) 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).

lgxbslgx · 2023-12-09T08:20:07Z

Thanks for the review. Could I get your help to sponsor this patch?

/integrate

openjdk · 2023-12-09T08:21:19Z

@lgxbslgx
Your change (at version e0d865a) is now ready to be sponsored by a Committer.

sormuras · 2023-12-18T11:25:18Z

/sponsor

openjdk · 2023-12-18T11:26:21Z

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

8c105f2: 7903601: jtr.xml logs produced with -xml argument do not contain failure justification
6cd72a0: 7903191: Fix build with "MAKEFLAGS=-j32".
a12c75c: 7903581: Typo in make file in pandoc binary detection
ef38655: 7903539: Please make jtreg build date reproducible
1f75b51: 7903553: Update GitHub checkout action to v4
48935be: 7903516: work around this-escape warnings
5c9d716: 7903524: Permit more segments in jtreg tags
07c6942: 7903518: Release jtreg 7.3.1
22f7332: 7903515: Some default env vars on Windows not set
b9c6dcd: 7903513: Update version to 7.4
... and 44 more: https://git.openjdk.org/jtreg/compare/1a03cc237fefc205a3703f8e035ebb72e75221c6...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-12-18T11:26:33Z

@sormuras @lgxbslgx Pushed as commit a396e2f.

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

CODETOOLS-7903300

7703b0e

openjdk bot changed the title ~~CODETOOLS-7903300~~ 7903300: Add test library support to jtreg plugin Sep 11, 2022

openjdk bot added the rfr Pull request is ready for review label Sep 11, 2022

mcimadamore reviewed Sep 15, 2022

View reviewed changes

lgxbslgx added 2 commits September 16, 2022 02:09

Adjust the api name. Fix the possible NPE and remove duplicated entries.

ff5370c

Bump the version and revise notes.

e0d865a

mcimadamore approved these changes Sep 15, 2022

View reviewed changes

jonathan-gibbons approved these changes Dec 7, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 7, 2023

openjdk bot added the sponsor Pull request is ready to be sponsored label Dec 9, 2023

openjdk bot added the integrated Pull request has been integrated label Dec 18, 2023

openjdk bot closed this Dec 18, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Dec 18, 2023

lgxbslgx deleted the CODETOOLS-7903300 branch December 19, 2023 01:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

7903300: Add test library support to jtreg plugin #117

7903300: Add test library support to jtreg plugin #117

lgxbslgx commented Sep 11, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 11, 2022

mlbridge bot commented Sep 11, 2022 •

edited

Loading

mcimadamore Sep 15, 2022

lgxbslgx Sep 15, 2022

mcimadamore commented Sep 15, 2022

lgxbslgx commented Sep 15, 2022

mcimadamore Sep 15, 2022

lgxbslgx Sep 15, 2022

mcimadamore commented Sep 15, 2022

lgxbslgx commented Sep 15, 2022

mcimadamore left a comment

lgxbslgx commented Sep 22, 2022

bridgekeeper bot commented Dec 4, 2023

jonathan-gibbons left a comment

openjdk bot commented Dec 7, 2023 •

edited

Loading

lgxbslgx commented Dec 9, 2023

openjdk bot commented Dec 9, 2023

sormuras commented Dec 18, 2023

openjdk bot commented Dec 18, 2023

openjdk bot commented Dec 18, 2023

7903300: Add test library support to jtreg plugin #117

7903300: Add test library support to jtreg plugin #117

Conversation

lgxbslgx commented Sep 11, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Sep 11, 2022

mlbridge bot commented Sep 11, 2022 • edited Loading

Webrevs

mcimadamore Sep 15, 2022

Choose a reason for hiding this comment

lgxbslgx Sep 15, 2022

Choose a reason for hiding this comment

mcimadamore commented Sep 15, 2022

lgxbslgx commented Sep 15, 2022

mcimadamore Sep 15, 2022

Choose a reason for hiding this comment

lgxbslgx Sep 15, 2022

Choose a reason for hiding this comment

mcimadamore commented Sep 15, 2022

lgxbslgx commented Sep 15, 2022

mcimadamore left a comment

Choose a reason for hiding this comment

lgxbslgx commented Sep 22, 2022

bridgekeeper bot commented Dec 4, 2023

jonathan-gibbons left a comment

Choose a reason for hiding this comment

openjdk bot commented Dec 7, 2023 • edited Loading

lgxbslgx commented Dec 9, 2023

openjdk bot commented Dec 9, 2023

sormuras commented Dec 18, 2023

openjdk bot commented Dec 18, 2023

openjdk bot commented Dec 18, 2023

lgxbslgx commented Sep 11, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 11, 2022 •

edited

Loading

openjdk bot commented Dec 7, 2023 •

edited

Loading