Re: RFR: 8166026: refactor shell tests to java [v3]
On Thu, 3 Dec 2020 21:41:58 GMT, Roger Riggs wrote: >> Ivan Šipka has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8166026: Refactor java/lang shell tests to java > > Marked as reviewed by rriggs (Reviewer). Some update is needed before this can be integrated. The BugID has already been resolved. (And the title did not match the title of the PR). Perhaps the wrong bugid was referrenced; 8166026 is an umbrella bug for a whole series of changes. - PR: https://git.openjdk.java.net/jdk/pull/1484
Re: RFR: 8166026: refactor shell tests to java [v3]
On Thu, 3 Dec 2020 19:46:14 GMT, Ivan Šipka wrote: >> @iignatev could you please review? Thank you. >> >> note to self: >> jtreg test/jdk/java/lang/ProcessBuilder/InheritIO/InheritIoTest.java >> test/jdk/java/lang/SecurityManager/modules/CustomSecurityManagerTest.java >> test/jdk/java/lang/Thread/uncaughtexceptions/UncaughtExceptionsTest.java >> test/jdk/java/lang/annotation/loaderLeak/LoaderLeakTest.java > > Ivan Šipka has updated the pull request incrementally with one additional > commit since the last revision: > > 8166026: Refactor java/lang shell tests to java Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1484
Re: RFR: 8166026: refactor shell tests to java [v3]
> @iignatev could you please review? Thank you. > > note to self: > jtreg test/jdk/java/lang/ProcessBuilder/InheritIO/InheritIoTest.java > test/jdk/java/lang/SecurityManager/modules/CustomSecurityManagerTest.java > test/jdk/java/lang/Thread/uncaughtexceptions/UncaughtExceptionsTest.java > test/jdk/java/lang/annotation/loaderLeak/LoaderLeakTest.java Ivan Šipka has updated the pull request incrementally with one additional commit since the last revision: 8166026: Refactor java/lang shell tests to java - Changes: - all: https://git.openjdk.java.net/jdk/pull/1484/files - new: https://git.openjdk.java.net/jdk/pull/1484/files/16857464..75096d42 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1484=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1484=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1484.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1484/head:pull/1484 PR: https://git.openjdk.java.net/jdk/pull/1484
Re: RFR: 8166026: refactor shell tests to java [v2]
On Mon, 30 Nov 2020 17:44:13 GMT, Roger Riggs wrote:
>> Ivan Šipka has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains four additional
>> commits since the last revision:
>>
>> - 8166026: Refactor java/lang shell tests to java
>> - 8166026:Refactor java/lang shell tests to java
>> - 8166026: removing tab character
>> - 8166026: refactor shell tests to java
>
> test/jdk/java/lang/ProcessBuilder/InheritIO/InheritIo.java line 26:
>
>> 24: import static java.lang.ProcessBuilder.Redirect.*;
>> 25:
>> 26: class InheritIo {
>
> The rename of the class is unnecessary and less readable.
> The function being tested is inheritIO and the test name should match. (As
> does the directory it is in).
It is just one class now so there is no need for package either? I ran a rebase
on this branch since deleting it would delete this PR also, I presume. So I
deleted the files related to other tests which now have their own PR
([1](https://github.com/openjdk/jdk/pull/1579),
[2](https://github.com/openjdk/jdk/pull/1578),
[3](https://github.com/openjdk/jdk/pull/1577)) and made a diff to `master`
(`HEAD` being `692b273ec53f54a879a4bbaad6c2f5f1d5358a71`):
[JDK-8166026-refactor-shell-to-java] $ git diff --name-only
open-mainline/master
test/jdk/java/lang/ProcessBuilder/InheritIO/InheritIO.java
test/jdk/java/lang/ProcessBuilder/InheritIO/InheritIO.sh
test/jdk/java/lang/ProcessBuilder/InheritIOTest.java
-
PR: https://git.openjdk.java.net/jdk/pull/1484
Re: RFR: 8166026: refactor shell tests to java [v2]
> @iignatev could you please review? Thank you. > > note to self: > jtreg test/jdk/java/lang/ProcessBuilder/InheritIO/InheritIoTest.java > test/jdk/java/lang/SecurityManager/modules/CustomSecurityManagerTest.java > test/jdk/java/lang/Thread/uncaughtexceptions/UncaughtExceptionsTest.java > test/jdk/java/lang/annotation/loaderLeak/LoaderLeakTest.java Ivan Šipka has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - 8166026: Refactor java/lang shell tests to java - 8166026:Refactor java/lang shell tests to java - 8166026: removing tab character - 8166026: refactor shell tests to java - Changes: - all: https://git.openjdk.java.net/jdk/pull/1484/files - new: https://git.openjdk.java.net/jdk/pull/1484/files/654cef82..16857464 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1484=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1484=00-01 Stats: 18532 lines in 441 files changed: 12661 ins; 3672 del; 2199 mod Patch: https://git.openjdk.java.net/jdk/pull/1484.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1484/head:pull/1484 PR: https://git.openjdk.java.net/jdk/pull/1484
Re: RFR: 8166026: refactor shell tests to java
On Mon, 30 Nov 2020 17:53:07 GMT, Roger Riggs wrote:
>> @iignatev could you please review? Thank you.
>>
>> note to self:
>> jtreg test/jdk/java/lang/ProcessBuilder/InheritIO/InheritIoTest.java
>> test/jdk/java/lang/SecurityManager/modules/CustomSecurityManagerTest.java
>> test/jdk/java/lang/Thread/uncaughtexceptions/UncaughtExceptionsTest.java
>> test/jdk/java/lang/annotation/loaderLeak/LoaderLeakTest.java
>
> test/jdk/java/lang/Thread/uncaughtexceptions/UncaughtExceptionsTest.java line
> 40:
>
>> 38: */
>> 39: public class UncaughtExceptionsTest {
>> 40:
>
> As with InheritIO, the nested classes that are invoked can be included in a
> single .java file.
Refactored and moved to [new PR](https://github.com/openjdk/jdk/pull/1578)
-
PR: https://git.openjdk.java.net/jdk/pull/1484
Re: RFR: 8166026: refactor shell tests to java
On Mon, 30 Nov 2020 21:32:35 GMT, Igor Ignatyev wrote: >> @iignatev could you please review? Thank you. >> >> note to self: >> jtreg test/jdk/java/lang/ProcessBuilder/InheritIO/InheritIoTest.java >> test/jdk/java/lang/SecurityManager/modules/CustomSecurityManagerTest.java >> test/jdk/java/lang/Thread/uncaughtexceptions/UncaughtExceptionsTest.java >> test/jdk/java/lang/annotation/loaderLeak/LoaderLeakTest.java > > test/jdk/java/lang/Thread/uncaughtexceptions/UncaughtExceptionsTest.java line > 70: > >> 68: outputAnalyzer.shouldHaveExitValue(exitValue); >> 69: outputAnalyzer.stderrShouldMatch(stdErrMatch); >> 70: outputAnalyzer.stdoutShouldMatch(stdOutMatch); > > why do you use `ShouldMatch` and not `ShouldContain` here? Because the first two test input tuples are regular expressions, please see [new PR](https://github.com/openjdk/jdk/pull/1578) - PR: https://git.openjdk.java.net/jdk/pull/1484
Re: RFR: 8166026: refactor shell tests to java
On Mon, 30 Nov 2020 21:36:40 GMT, Igor Ignatyev wrote:
>> @iignatev could you please review? Thank you.
>>
>> note to self:
>> jtreg test/jdk/java/lang/ProcessBuilder/InheritIO/InheritIoTest.java
>> test/jdk/java/lang/SecurityManager/modules/CustomSecurityManagerTest.java
>> test/jdk/java/lang/Thread/uncaughtexceptions/UncaughtExceptionsTest.java
>> test/jdk/java/lang/annotation/loaderLeak/LoaderLeakTest.java
>
> test/jdk/java/lang/annotation/loaderLeak/LoaderLeakTest.java line 53:
>
>> 51: Files.createDirectories(REPOSITORY_PATH);
>> 52: List classes = List.of("A.class", "B.class", "C.class");
>> 53: for (String fileName : classes) {
>
> is this really needed for the test to operate correctly? or can we just use
> _regular_ `TEST_CLASSES` as CP?
CLASSES_PATH is initialized with value of TEST_CLASSES property. It is
obviously confusing variable naming so in the [new
PR](https://github.com/openjdk/jdk/pull/1577) it is renamed
-
PR: https://git.openjdk.java.net/jdk/pull/1484
Re: RFR: 8166026: refactor shell tests to java
On Fri, 27 Nov 2020 18:50:19 GMT, Ivan Šipka wrote:
> @iignatev could you please review? Thank you.
>
> note to self:
> jtreg test/jdk/java/lang/ProcessBuilder/InheritIO/InheritIoTest.java
> test/jdk/java/lang/SecurityManager/modules/CustomSecurityManagerTest.java
> test/jdk/java/lang/Thread/uncaughtexceptions/UncaughtExceptionsTest.java
> test/jdk/java/lang/annotation/loaderLeak/LoaderLeakTest.java
it would be much easier to review if it was 4 separate RFEs/RFRs.
many new files miss a newline at the end.
test/jdk/java/lang/ProcessBuilder/InheritIO/InheritIoTest.java line 60:
> 58: outputAnalyzer.shouldHaveExitValue(0);
> 59: outputAnalyzer.stderrShouldMatch(EXPECTED_RESULT_STDERR);
> 60: outputAnalyzer.stdoutShouldMatch(EXPECTED_RESULT_STDOUT);
you can chain these methods.
test/jdk/java/lang/ProcessBuilder/InheritIO/InheritIoTest.java line 61:
> 59: outputAnalyzer.stderrShouldMatch(EXPECTED_RESULT_STDERR);
> 60: outputAnalyzer.stdoutShouldMatch(EXPECTED_RESULT_STDOUT);
> 61: assertEquals(outputAnalyzer.getOutput(),EXPECTED_RESULT_STDOUT +
> EXPECTED_RESULT_STDERR);
I'd rather check stdout and stderr independently, this will also make checks at
59 and 60 redundant.
test/jdk/java/lang/Thread/uncaughtexceptions/UncaughtExceptionsTest.java line
70:
> 68: outputAnalyzer.shouldHaveExitValue(exitValue);
> 69: outputAnalyzer.stderrShouldMatch(stdErrMatch);
> 70: outputAnalyzer.stdoutShouldMatch(stdOutMatch);
why do you use `ShouldMatch` and not `ShouldContain` here?
test/jdk/java/lang/annotation/loaderLeak/LoaderLeakTest.java line 53:
> 51: Files.createDirectories(REPOSITORY_PATH);
> 52: List classes = List.of("A.class", "B.class", "C.class");
> 53: for (String fileName : classes) {
is this really needed for the test to operate correctly? or can we just use
_regular_ `TEST_CLASSES` as CP?
-
Changes requested by iignatyev (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/1484
Re: RFR: 8166026: refactor shell tests to java
On Fri, 27 Nov 2020 18:50:19 GMT, Ivan Šipka wrote:
> @iignatev could you please review? Thank you.
>
> note to self:
> jtreg test/jdk/java/lang/ProcessBuilder/InheritIO/InheritIoTest.java
> test/jdk/java/lang/SecurityManager/modules/CustomSecurityManagerTest.java
> test/jdk/java/lang/Thread/uncaughtexceptions/UncaughtExceptionsTest.java
> test/jdk/java/lang/annotation/loaderLeak/LoaderLeakTest.java
test/jdk/java/lang/ProcessBuilder/InheritIO/InheritIo.java line 26:
> 24: import static java.lang.ProcessBuilder.Redirect.*;
> 25:
> 26: class InheritIo {
The rename of the class is unnecessary and less readable.
The function being tested is inheritIO and the test name should match. (As does
the directory it is in).
test/jdk/java/lang/ProcessBuilder/InheritIO/InheritIoTest.java line 42:
> 40:
> 41: public class InheritIoTest {
> 42:
Having two classes and files with similar names is confusing. TestInhieritIO
and InheritIOTest.
Since InheritIO already has subclasses that are the ones being invoked;
that leaves methods in InheritIO to have the test cases.
test/jdk/java/lang/Thread/uncaughtexceptions/UncaughtExceptionsTest.java line
40:
> 38: */
> 39: public class UncaughtExceptionsTest {
> 40:
As with InheritIO, the nested classes that are invoked can be included in a
single .java file.
-
PR: https://git.openjdk.java.net/jdk/pull/1484
RFR: 8166026: refactor shell tests to java
@iignatev could you please review? Thank you. note to self: jtreg test/jdk/java/lang/ProcessBuilder/InheritIO/InheritIoTest.java test/jdk/java/lang/SecurityManager/modules/CustomSecurityManagerTest.java test/jdk/java/lang/Thread/uncaughtexceptions/UncaughtExceptionsTest.java test/jdk/java/lang/annotation/loaderLeak/LoaderLeakTest.java - Commit messages: - 8166026: removing tab character - 8166026: refactor shell tests to java Changes: https://git.openjdk.java.net/jdk/pull/1484/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1484=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8166026 Stats: 1002 lines in 12 files changed: 496 ins; 505 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1484.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1484/head:pull/1484 PR: https://git.openjdk.java.net/jdk/pull/1484
