Re: RFR: 8334332: TestIOException.java fails if run by root [v2]
On Tue, 18 Jun 2024 08:32:43 GMT, Pavel Rappo wrote: > > Does the `throw new Error(f + ": " + message);` should be replaced to > > `throw new SkippedException(f + ": " + message);`, to avoid report failure > > if a directory cannot be made read-only. > > Yes, it does! That's the whole point. Ironically, I missed that in my diff. Thanks your advice, the code has been updated according your advice, and has been verified on linux and windows. - PR Comment: https://git.openjdk.org/jdk/pull/19731#issuecomment-217950
Re: RFR: 8334332: TestIOException.java fails if run by root [v2]
On Tue, 18 Jun 2024 01:26:33 GMT, SendaoYan wrote: > Does the `throw new Error(f + ": " + message);` should be replaced to `throw > new SkippedException(f + ": " + message);`, to avoid report failure if a > directory cannot be made read-only. Yes, it does! That's the whole point. Ironically, I missed that in my diff. - PR Comment: https://git.openjdk.org/jdk/pull/19731#issuecomment-2175523628
Re: RFR: 8334332: TestIOException.java fails if run by root [v2]
On Tue, 18 Jun 2024 06:19:23 GMT, Andrey Turbanov wrote:
>> SendaoYan has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> delete an extra whitespace
>
> test/langtools/jdk/javadoc/doclet/testIOException/TestIOException.java line
> 56:
>
>> 54: public static void main(String... args) throws Exception {
>> 55: var tester = new TestIOException();
>> 56: if(Platform.isRoot() && !tester.isWindows()) {
>
> Suggestion:
>
> if (Platform.isRoot() && !tester.isWindows()) {
Thanks for the review. The code has been changed according to the review
opinions of @pavelrappo.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19731#discussion_r1643905043
Re: RFR: 8334332: TestIOException.java fails if run by root [v2]
On Sat, 15 Jun 2024 09:54:38 GMT, SendaoYan wrote:
>> Hi all,
>> Test
>> `test/langtools/jdk/javadoc/doclet/testIOException/TestIOException.java` run
>> fails with root user privileged. I think it's necessary to skip this
>> testcase when user is root.
>> Why run the jtreg test by root user? It's because during rpmbuild process
>> for linux distribution of JDK, root user is the default user to build the
>> openjdk, also is the default user to run the `make test-tier1`, this PR make
>> this testcase more robustness.
>> The change has been verified, only change the testcase, no risk.
>
> SendaoYan has updated the pull request incrementally with one additional
> commit since the last revision:
>
> delete an extra whitespace
test/langtools/jdk/javadoc/doclet/testIOException/TestIOException.java line 56:
> 54: public static void main(String... args) throws Exception {
> 55: var tester = new TestIOException();
> 56: if(Platform.isRoot() && !tester.isWindows()) {
Suggestion:
if (Platform.isRoot() && !tester.isWindows()) {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19731#discussion_r1643875959
Re: RFR: 8334332: TestIOException.java fails if run by root [v2]
On Mon, 17 Jun 2024 16:10:08 GMT, Pavel Rappo wrote:
> ```
> -private Error error(File f, String message) {
> +private Error skip(File f, String message) {
> +out.print(System.getProperty("user.name"));
> out.println(f + ": " + message);
> showAllAttributes(f.toPath());
> throw new Error(f + ": " + message);
> @@ -242,20 +234,5 @@ private void showAttributes(Path p, String attributes) {
> out.println("Error accessing attributes " + attributes + ": " +
> t);
> }
> }
> ```
Does the `throw new Error(f + ": " + message);` should be replaced to `throw
new SkippedException(f + ": " + message);`, to avoid report failure if a
directory cannot be made read-only.
-
PR Comment: https://git.openjdk.org/jdk/pull/19731#issuecomment-2174732914
Re: RFR: 8334332: TestIOException.java fails if run by root [v2]
On Mon, 17 Jun 2024 16:10:08 GMT, Pavel Rappo wrote:
> -private Error error(File f, String message) {
> +private Error skip(File f, String message) {
> +out.print(System.getProperty("user.name"));
> out.println(f + ": " + message);
> showAllAttributes(f.toPath());
> throw new Error(f + ": " + message);
> @@ -242,20 +234,5 @@ private void showAttributes(Path p, String attributes) {
> out.println("Error accessing attributes " + attributes + ": " +
> t);
> }
> }
Does the `throw new Error(f + ": " + message);` should replace to `throw new
SkippedException(f + ": " + message);`, to avoid report failure if a directory
cannot be made read-only.
-
PR Comment: https://git.openjdk.org/jdk/pull/19731#issuecomment-2174730568
Re: RFR: 8334332: TestIOException.java fails if run by root [v2]
On Sat, 15 Jun 2024 09:54:38 GMT, SendaoYan wrote:
>> Hi all,
>> Test
>> `test/langtools/jdk/javadoc/doclet/testIOException/TestIOException.java` run
>> fails with root user privileged. I think it's necessary to skip this
>> testcase when user is root.
>> Why run the jtreg test by root user? It's because during rpmbuild process
>> for linux distribution of JDK, root user is the default user to build the
>> openjdk, also is the default user to run the `make test-tier1`, this PR make
>> this testcase more robustness.
>> The change has been verified, only change the testcase, no risk.
>
> SendaoYan has updated the pull request incrementally with one additional
> commit since the last revision:
>
> delete an extra whitespace
Come to think of it, there's already a mechanism in the test to skip a Windows
machine if a directory cannot be made read-only. We could combine that
mechanism with what you propose, simplifying and making code more robust at the
same time.
Any time when we do `throw error`, it means that the test should be skipped.
Cannot create a directory? Cannot make it read-only? Can write to a directory
after having made it read-only? In any of these cases, our assumptions are
invalid; the test should be skipped, not failed. So perhaps we could modify it
like this?
diff --git
a/test/langtools/jdk/javadoc/doclet/testIOException/TestIOException.java
b/test/langtools/jdk/javadoc/doclet/testIOException/TestIOException.java
index 9127cdf86bb..fb01707511a 100644
--- a/test/langtools/jdk/javadoc/doclet/testIOException/TestIOException.java
+++ b/test/langtools/jdk/javadoc/doclet/testIOException/TestIOException.java
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2016, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -25,7 +25,7 @@
* @test
* @bug 8164130
* @summary test IOException handling
- * @library ../../lib
+ * @library ../../lib /test/lib
* @modules jdk.javadoc/jdk.javadoc.internal.tool
* @build javadoc.tester.*
* @run main TestIOException
@@ -61,16 +61,13 @@ public static void main(String... args) throws Exception {
public void testReadOnlyDirectory() {
File outDir = new File("out1");
if (!outDir.mkdir()) {
-throw error(outDir, "Cannot create directory");
+throw skip(outDir, "Cannot create directory");
}
if (!outDir.setReadOnly()) {
-if (skip(outDir)) {
-return;
-}
-throw error(outDir, "could not set directory read-only");
+throw skip(outDir, "could not set directory read-only");
}
if (outDir.canWrite()) {
-throw error(outDir, "directory is writable");
+throw skip(outDir, "directory is writable");
}
try {
@@ -93,15 +90,15 @@ public void testReadOnlyDirectory() {
public void testReadOnlyFile() throws Exception {
File outDir = new File("out2");
if (!outDir.mkdir()) {
-throw error(outDir, "Cannot create directory");
+throw skip(outDir, "Cannot create directory");
}
File index = new File(outDir, "index.html");
try (FileWriter fw = new FileWriter(index)) { }
if (!index.setReadOnly()) {
-throw error(index, "could not set index read-only");
+throw skip(index, "could not set index read-only");
}
if (index.canWrite()) {
-throw error(index, "index is writable");
+throw skip(index, "index is writable");
}
try {
@@ -139,16 +136,13 @@ public void testReadOnlySubdirectory() throws Exception {
File outDir = new File("out3");
File pkgOutDir = new File(outDir, "p");
if (!pkgOutDir.mkdirs()) {
-throw error(pkgOutDir, "Cannot create directory");
+throw skip(pkgOutDir, "Cannot create directory");
}
if (!pkgOutDir.setReadOnly()) {
-if (skip(pkgOutDir)) {
-return;
-}
-throw error(pkgOutDir, "could not set directory read-only");
+throw skip(pkgOutDir, "could not set directory read-only");
}
if (pkgOutDir.canWrite()) {
-throw error(pkgOutDir, "directory is writable");
+throw skip(pkgOutDir, "directory is writable");
}
// run javadoc and check results
@@ -192,16 +186,13 @@ public void testReadOnlyDocFilesDir() throws Exception {
File pkgOutDir = new File(outDir, "p");
File docFilesOutDir = new File(pkgOutDir, "doc-files");
if (!docFilesOutDir.mkdirs()) {
-throw error(docFilesOutDir, "Cannot create directory");
+throw skip(docFilesOutDir, "Cannot create directory");
Re: RFR: 8334332: TestIOException.java fails if run by root [v2]
On Mon, 17 Jun 2024 15:10:52 GMT, Pavel Rappo wrote:
>> SendaoYan has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> delete an extra whitespace
>
> test/langtools/jdk/javadoc/doclet/testIOException/TestIOException.java line
> 58:
>
>> 56: if(Platform.isRoot() && !tester.isWindows()) {
>> 57: throw new SkippedException("root user has privileged will
>> make this test fail.");
>> 58: }
>
> Suggestion:
>
> if (Platform.isRoot() && !tester.isWindows()) {
> throw new SkippedException("root user has privileges that will
> make this test fail");
> }
> var tester = new TestIOException();
`isWindows()` is not static, so call this function need a instance
`TestIOException` object, thus `var tester = new TestIOException();` should at
before line 56.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19731#discussion_r1643003292
Re: RFR: 8334332: TestIOException.java fails if run by root [v2]
On Mon, 17 Jun 2024 15:11:56 GMT, Pavel Rappo wrote: > I re-titled the JBS issue, please update this PR title accordingly. Okey, thanks. - PR Comment: https://git.openjdk.org/jdk/pull/19731#issuecomment-2173697821
