8287902: UnreadableRB case in MissingResourceCauseTest is not working reliably on Windows #9061

magicus · 2022-06-07T12:19:29Z

The test test/jdk/java/util/ResourceBundle/Control/MissingResourceCauseTest.java verifies different failure modes of resource bundles. One of the failures is that the runner class, MissingResourceCauseTestRun.java, creates a file UnreadableRB, and runs chmod 000 on it, to make it unreadable by the test. Then MissingResourceCauseTest is called, and the UnreadableRB file is removed.

This does not work reliably on Windows. On msys2, chmod is essentially a no-op, so the file is not made unreadable, and hence the test fails. In my personal cygwin test environment, the chmod command does have some effect, but it is still not enough to make the file unreadable, and so the test fails.

The test was originally a shell script test that got converted to Java in JDK-4354216. The original shell script code explicitly excluded Windows from testing. This was changed in the rewrite, for reasons I cannot determine.

What suprises me, though, is the "how can this ever has worked???" factor. Apparently the test passes on the current Cygwin setup on GHA. I have failed to reproduce the working conditions that makes a file actually unreadable for the owner on Windows, neither on my GHA test repo, nor locally. I've searched the web to figure out how to properly set file permissions on Windows to make the file unreadable, using Windows native tools, to no avail. I've even asked a Stack Overflow question; which as of yet is still unanswered.

Since I feel I've spent far more time than reasonable trying to get this to work properly, I suggest we instead skip the unreadable test on Windows. It is clearly unstable and highly depending on the Windows environment, the test was never originally supported or intended for Windows, and at the of the day, testing file unreadability is not an important regression test for JDK-4354216.

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-8287902: UnreadableRB case in MissingResourceCauseTest is not working reliably on Windows

Reviewers

Naoto Sato (@naotoj - Reviewer) ⚠️ Review applies to 7cd2dccf

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9061

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

Using diff file

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

… reliably on Windows

bridgekeeper · 2022-06-07T12:20:50Z

👋 Welcome back ihse! 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 · 2022-06-07T12:24:29Z

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

core-libs

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 · 2022-06-07T12:28:31Z

Webrevs

01: Full - Incremental (162e928c)
00: Full (7cd2dccf)

magicus · 2022-06-07T12:30:36Z

/label i18n

openjdk · 2022-06-07T12:31:03Z

@magicus
The i18n label was successfully added.

jaikiran · 2022-06-08T15:23:57Z

test/jdk/java/util/ResourceBundle/Control/MissingResourceCauseTest.java

@@ -45,6 +45,9 @@ private static void callGetBundle(String baseName,

    private static void callGetBundle(String baseName, Locale locale,
                                      Class<? extends Throwable> expectedCause) {
+        if (baseName.equals("UnreadableRB") && System.getProperty("os.name").startsWith("Windows")) {


Hello @magicus, perhaps we could do this check in the main of this class (line 37) before calling the callGetBundle for the UnreadableRB base name? That way this method doesn't have to bother about which baseName is being passed.

I had a quick look at how other tests do the OS name checks and they use the Platform.isWindows() API which is part of the test library. I suspect you may not be able to use it here though since this MissingResourceCauseTest class gets launched manually through the MissingResourceCauseTestRun. So using System.getProperty I think is fine. Nit - perhaps we could match this check to do whatever Platform.isWindows() is doing (which seems to be checking that the os name starts with win, ignoring the case).

jaikiran · 2022-06-08T15:24:25Z

I have failed to reproduce the working conditions that makes a file actually unreadable for the owner on Windows, neither on my GHA test repo, nor locally.

I had quick look at the CI setup we have access to. It appears that this test passes there successfully on Windows. Could it be something to do with Windows version being used?

naotoj · 2022-06-08T17:43:49Z

Thanks for the investigation, Magnus. I believe it is a bug to include the Windows environment in refactoring the shell script to Java. Why it's been working? No idea. Could be IOException has been thrown not by unreadable but for some other reason?

magicus · 2022-06-09T08:22:21Z

@naotoj Yes, that could be a likely possibility. Perhaps the file were never created, or not in the expected place?

naotoj · 2022-06-09T15:40:12Z

@naotoj Yes, that could be a likely possibility. Perhaps the file were never created, or not in the expected place?

I'd guess something like that too. Anyways, eliminating this unreliability in test is a good clean up. Approving the fix assuming Jai's comment is addressed.

openjdk · 2022-06-09T16:10:21Z

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

8287902: UnreadableRB case in MissingResourceCauseTest is not working reliably on Windows

Reviewed-by: naoto

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

0901548: 8283724: Incorrect description for jtreg-failure-handler option
dae4c49: 8286197: C2: Optimize MemorySegment shape in int loop
94b473e: 8280454: G1: ClassLoaderData verification keeps CLDs live that causes problems with VerifyDuringGC during Remark
900d967: 8287924: Avoid redundant HashMap.containsKey call in EnvHelp.mapToHashtable
d482d7f: 8286160: (fs) Files.exists returns unexpected results with C:\pagefile.sys because it's not readable
edff51e: 8284858: Start of release updates for JDK 20
2671443: 8286171: HttpClient/2 : Expect:100-Continue blocks indefinitely when response is not 100
59b0de6: 8288048: Build failure with GCC 6 after JDK-8286562
db4405d: 8288078: linux-aarch64-optimized build fails in Tier5 after JDK-8287567
7e948f7: 8287903: Reduce runtime of java.math microbenchmarks
... and 66 more: https://git.openjdk.org/jdk/compare/2d8c6490540e3ccea23b81129b2e4073915071e0...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.

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

magicus · 2022-06-10T10:06:31Z

/integrate

openjdk · 2022-06-10T10:07:20Z

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

0901548: 8283724: Incorrect description for jtreg-failure-handler option
dae4c49: 8286197: C2: Optimize MemorySegment shape in int loop
94b473e: 8280454: G1: ClassLoaderData verification keeps CLDs live that causes problems with VerifyDuringGC during Remark
900d967: 8287924: Avoid redundant HashMap.containsKey call in EnvHelp.mapToHashtable
d482d7f: 8286160: (fs) Files.exists returns unexpected results with C:\pagefile.sys because it's not readable
edff51e: 8284858: Start of release updates for JDK 20
2671443: 8286171: HttpClient/2 : Expect:100-Continue blocks indefinitely when response is not 100
59b0de6: 8288048: Build failure with GCC 6 after JDK-8286562
db4405d: 8288078: linux-aarch64-optimized build fails in Tier5 after JDK-8287567
7e948f7: 8287903: Reduce runtime of java.math microbenchmarks
... and 66 more: https://git.openjdk.org/jdk/compare/2d8c6490540e3ccea23b81129b2e4073915071e0...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-06-10T10:07:40Z

@magicus Pushed as commit 975316e.

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

8287902: UnreadableRB case in MissingResourceCauseTest is not working…

7cd2dcc

… reliably on Windows

openjdk bot added the rfr Pull request is ready for review label Jun 7, 2022

openjdk bot added the core-libs core-libs-dev@openjdk.org label Jun 7, 2022

openjdk bot added the i18n i18n-dev@openjdk.org label Jun 7, 2022

jaikiran reviewed Jun 8, 2022

View reviewed changes

naotoj approved these changes Jun 9, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 9, 2022

From code review: use more robust OS check and move it to main()

162e928

openjdk bot added the integrated Pull request has been integrated label Jun 10, 2022

openjdk bot closed this Jun 10, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 10, 2022

magicus deleted the msys-test-resourcebundles branch June 14, 2022 07:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

8287902: UnreadableRB case in MissingResourceCauseTest is not working reliably on Windows #9061

8287902: UnreadableRB case in MissingResourceCauseTest is not working reliably on Windows #9061

Uh oh!

magicus commented Jun 7, 2022 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented Jun 7, 2022

Uh oh!

openjdk bot commented Jun 7, 2022

Uh oh!

mlbridge bot commented Jun 7, 2022 •

edited

Loading

Uh oh!

magicus commented Jun 7, 2022

Uh oh!

openjdk bot commented Jun 7, 2022

Uh oh!

jaikiran Jun 8, 2022

Uh oh!

jaikiran commented Jun 8, 2022

Uh oh!

naotoj commented Jun 8, 2022

Uh oh!

magicus commented Jun 9, 2022

Uh oh!

naotoj commented Jun 9, 2022 •

edited

Loading

Uh oh!

openjdk bot commented Jun 9, 2022 •

edited

Loading

Uh oh!

magicus commented Jun 10, 2022

Uh oh!

openjdk bot commented Jun 10, 2022

Uh oh!

openjdk bot commented Jun 10, 2022

Uh oh!

Uh oh!

8287902: UnreadableRB case in MissingResourceCauseTest is not working reliably on Windows #9061

8287902: UnreadableRB case in MissingResourceCauseTest is not working reliably on Windows #9061

Uh oh!

Conversation

magicus commented Jun 7, 2022 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jun 7, 2022

Uh oh!

openjdk bot commented Jun 7, 2022

Uh oh!

mlbridge bot commented Jun 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

magicus commented Jun 7, 2022

Uh oh!

openjdk bot commented Jun 7, 2022

Uh oh!

jaikiran Jun 8, 2022

Choose a reason for hiding this comment

Uh oh!

jaikiran commented Jun 8, 2022

Uh oh!

naotoj commented Jun 8, 2022

Uh oh!

magicus commented Jun 9, 2022

Uh oh!

naotoj commented Jun 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jun 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

magicus commented Jun 10, 2022

Uh oh!

openjdk bot commented Jun 10, 2022

Uh oh!

openjdk bot commented Jun 10, 2022

Uh oh!

Uh oh!

magicus commented Jun 7, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 7, 2022 •

edited

Loading

naotoj commented Jun 9, 2022 •

edited

Loading

openjdk bot commented Jun 9, 2022 •

edited

Loading