8325201: (zipfs) Disable TestPosix.setPermissionsShouldConvertToUnix which fails on Windows

[eirbjo]

Can I please get a review for this trivial change to disable the test TestPosix.setPermissionsShouldConvertToUnix on Windows?

This test was recently introduced in JDK-8324635, but fails intermittently on Windows CI runs. (See JDK-8325199)

To reduce CI noise, I suggest we simply disable this test on Windows for now. The actual fix of the failing test will be investigated and fixed as a follow-up PR.

---

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

• JDK-8325201: (zipfs) Disable TestPosix.setPermissionsShouldConvertToUnix which fails on Windows (Bug - P4)

Reviewers

• Lance Andersen (@LanceAndersen - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17700

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17700.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back eirbjo! 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]

@eirbjo The following label will be automatically applied to this pull request:

• nio

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (b09472c7)
• 00: Full (3adb75d3)

[LanceAndersen]

Hi Eirik,

I think disabling the tests makes sense for now or perhaps just disable for windows. Maybe add something like:
private static final boolean isWindows = System.getProperty("os.name") .startsWith("Windows");

And then have the test validate isWindows in the interim?

I think we should use a separate bug to disable the test and keep this bug open

[eirbjo]

I think we should use a separate PR to disable the test and keep this bug open

Lance,

I did create a separate issue for this PR to disable the test. We can keep the other bug open, yes. Or did I misunderstand something here..?

I’m not sure how to disable this test specially for Windows.

[LanceAndersen]

I think we should use a separate PR to disable the test and keep this bug open

Lance,

I did create a separate issue for this PR to disable the test. We can keep the other bug open, yes. Or did I misunderstand something here..?

Hi Eirik,

Apologies for not being clearer

Sorry I missed that, thank you :-)

I’m not sure how to disable this test specially for Windows.

You could:
private static final boolean isWindows = System.getProperty("os.name") .startsWith("Windows");
and in the test:

if(isWindows) {
   return;  
}

You could also use the jtreg SkippedException

import jtreg.SkippedException;

if(isWindows) {
   throw new SkippedException("Running on windows, temporarily by-passing running this test");  
}

See the JTREG FAQ 4.34

[eirbjo]

Thanks Lance, I've pushed a commit which skips the test selectively on Windows. Since this test runs with a security manager, I needed to add a property read permission for os.name.

[LanceAndersen]

Thank you for the update Eirik, That looks good. I would also link this bug to the open bug in JBS when you have a moment,

[openjdk]

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

8325201: (zipfs) Disable TestPosix.setPermissionsShouldConvertToUnix which fails on Windows

Reviewed-by: lancea

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 1 new commit pushed to the master branch:

• ed06846: 8325037: x86: enable and fix hotspot/jtreg/compiler/vectorization/TestRoundVectFloat.java

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[eirbjo]

I would also link this bug to the open bug in JBS when you have a moment

This bug, 8325201 already has a "related to" link to the open test failure bug JDK-8325199.

Does that look OK to you, or did you mean something else?

[eirbjo]

/integrate

[openjdk]

Going to push as commit 692c9f8.
Since your change was applied there has been 1 commit pushed to the master branch:

• ed06846: 8325037: x86: enable and fix hotspot/jtreg/compiler/vectorization/TestRoundVectFloat.java

Your commit was automatically rebased without conflicts.

[openjdk]

@eirbjo Pushed as commit 692c9f8.

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

[LanceAndersen]

I would also link this bug to the open bug in JBS when you have a moment

This bug, 8325201 already has a "related to" link to the open test failure bug JDK-8325199.

Does that look OK to you, or did you mean something else?

It looks fine, I checked before I submitted the approval so must have been a timing issue. Thank you for the above