Re: RFR: JDK-8275650 Problemlist java/io/File/createTempFile/SpecialTempFile.java for Windows 11 [v5]
On Fri, 29 Oct 2021 10:38:41 GMT, Ivan Šipka wrote: >> cc @ctornqvi > > Ivan Šipka has updated the pull request incrementally with one additional > commit since the last revision: > > removed file added by accident I guess the proper course of actions would be not to mention any confidential comments/information (including their existence) in open communication at all, and instead provided open community with the enough publicly available information so they can understand the bug, patch, testing, …, in this particular case it would mean describing the testing w/o providing any details about oracle resources - PR: https://git.openjdk.java.net/jdk/pull/6025
Re: RFR: JDK-8275650 Problemlist java/io/File/createTempFile/SpecialTempFile.java for Windows 11 [v5]
On Fri, 29 Oct 2021 10:38:41 GMT, Ivan Šipka wrote:
>> cc @ctornqvi
>
> Ivan Šipka has updated the pull request incrementally with one additional
> commit since the last revision:
>
> removed file added by accident
Thanks for the clarification, David. I guess my recollection of jtreg code
isn’t as good as I thought, and `-` isn’t mandatory, though this means there is
a (theoretically possible) ambiguity, e.g. `{os=windows; version=11}` vs
`{os=windows1; version=1}`
-
PR: https://git.openjdk.java.net/jdk/pull/6025
Re: RFR: JDK-8275650 Problemlist java/io/File/createTempFile/SpecialTempFile.java for Windows 11 [v5]
On Wed, 3 Nov 2021 23:11:36 GMT, Ivan Šipka wrote: >> That doesn’t seem right as jtreg expects `-` not `` > >> That doesn’t seem right as jtreg expects `-` not `` > > It has been tested, details in ticket comment. I’m sorry @frkator but there is nothing in the ticket. - PR: https://git.openjdk.java.net/jdk/pull/6025
Re: RFR: JDK-8275650 Problemlist java/io/File/createTempFile/SpecialTempFile.java for Windows 11 [v5]
On Fri, 29 Oct 2021 10:38:41 GMT, Ivan Šipka wrote: >> cc @ctornqvi > > Ivan Šipka has updated the pull request incrementally with one additional > commit since the last revision: > > removed file added by accident That doesn’t seem right as jtreg expects `-` not `` - PR: https://git.openjdk.java.net/jdk/pull/6025
Re: RFR: 8275650: Problemlist java/io/File/createTempFile/SpecialTempFile.java for Windows 11 [v2]
On Tue, 26 Oct 2021 19:48:47 GMT, Ivan Šipka wrote: >> cc @ctornqvi > > 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 two additional commits since > the last revision: > > - fixed OS identifier > - 8274122: added to problem list And now you use an incorrect bug id in the problem list, it should be 8274122 - PR: https://git.openjdk.java.net/jdk/pull/6025
Re: RFR: 8275650: Problemlist java/io/File/createTempFile/SpecialTempFile.java for Windows 11
On Wed, 20 Oct 2021 00:04:08 GMT, Ivan Šipka wrote: > cc @ctornqvi as it has been discussed internally, jtreg doesn’t recognize $os-$arch-$version pattern in problem list (but understands $os-$version) so for the test to be excluded only on windows 11, you should use `windows-11` - Changes requested by iignatyev (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6025
Re: RFR: 8275650: Problemlist java/io/File/createTempFile/SpecialTempFile.java for Windows 11
On Wed, 20 Oct 2021 00:04:08 GMT, Ivan Šipka wrote: > cc @ctornqvi Marked as reviewed by iignatyev (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6025
Re: RFR: 8275512: Upgrade required version of jtreg to 6.1
On Tue, 19 Oct 2021 13:51:45 GMT, Weijun Wang wrote: > As a follow up of JEP 411, we will soon disallow security manager by default. > jtreg 6.1 does not set its own security manager if JDK version is >= 18. LGTM - Marked as reviewed by iignatyev (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6012
Re: RFR: 8273314: Add tier4 test groups [v4]
On Fri, 17 Sep 2021 06:59:09 GMT, Aleksey Shipilev wrote:
>> During the review of JDK-8272914 that added hotspot:tier{2,3} groups,
>> @iignatev suggested to create tier4 groups that capture all tests not in
>> tiers{1,2,3}.
>>
>> Caveats:
>> - I excluded `applications` from `hotspot:tier4`, because they require test
>> dependencies (e.g. jcstress).
>> - `jdk:tier4` only runs well with `JTREG_KEYWORDS=!headful` or reduced
>> concurrency with `TEST_JOBS=1`, because headful tests cannot run in parallel
>>
>> Sample run with `JTREG_KEYWORDS=!headful`:
>>
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
jtreg:test/hotspot/jtreg:tier4 3585 3584 0 1
<<
jtreg:test/jdk:tier4 2893 2887 5 1
<<
>>jtreg:test/langtools:tier40 0 0 0
>>
>>jtreg:test/jaxp:tier4 0 0 0 0
>>
>> ==
>>
>> real 699m39.462s
>> user 6626m8.448s
>> sys 1110m43.704s
>>
>>
>> There are interesting test failures on my machine, which I would address
>> separately.
>
> Aleksey Shipilev 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 five additional
> commits since the last revision:
>
> - Merge branch 'master' into JDK-8273314-tier4
> - Merge branch 'master' into JDK-8273314-tier4
> - Drop applications and fix the comment
> - Drop exceptions
> - Add tier4 test groups
LGTM
-
Marked as reviewed by iignatyev (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5357
Re: RFR: 8247980: Exclusive execution of java/util/stream tests slows down tier1
On Thu, 19 Aug 2021 15:15:31 GMT, Aleksey Shipilev wrote: > See the bug report for more details. I would appreciate if people with their > corporate testing systems would run this through their systems to avoid > surprises. (ping @RealCLanger, @iignatev). Marked as reviewed by iignatyev (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5189
Re: RFR: 8273314: Add tier4 test groups [v3]
On Mon, 6 Sep 2021 13:22:03 GMT, Aleksey Shipilev wrote:
>> During the review of JDK-8272914 that added hotspot:tier{2,3} groups,
>> @iignatev suggested to create tier4 groups that capture all tests not in
>> tiers{1,2,3}. I have excluded `vmTestbase` and `hotspot:tier4,` because they
>> take 10+ hours on my highly parallel machine. I have also excluded
>> `applications` from `hotspot:tier4`, because they require test dependencies
>> (e.g. jcstress).
>>
>> Sample run:
>>
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
jtreg:test/hotspot/jtreg:tier4 426 425 1 0
<<
jtreg:test/jdk:tier4 2891 2885 4 2
<<
>>jtreg:test/langtools:tier40 0 0 0
>>
>>jtreg:test/jaxp:tier4 0 0 0 0
>>
>> ==
>>
>> real 64m13.994s
>> user 1462m1.213s
>> sys 39m38.032s
>>
>>
>> There are interesting test failures on my machine, which I would address
>> separately.
>
> Aleksey Shipilev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Drop applications and fix the comment
> _Mailing list message from [David Holmes](mailto:david.hol...@oracle.com) on
> [hotspot-dev](mailto:hotspot-...@mail.openjdk.java.net):_
>
> On 7/09/2021 1:17 am, Aleksey Shipilev wrote:
>
> > @dholmes-ora: Generally speaking, all `tierX` definitions are rather
> > arbitrary, as there seem to be nothing intrinsic about the tests to be in a
> > particular tier. In other words, what `tierX` consists of is a matter of
> > agreement. I'd say `hotspot:tier4` is "all assorted Hotspot tests that are
> > not application-specific suites"
>
> The difference is that your previous work just consolidated the existing
> subsystem tier 1-3 definitions, but here you are choosing to define "all
> the rest" as tier 4. I don't think it is actually helpful/useful to
> anyone - and it bears no resemblance whatsoever to what we call "tier
> 4", so that will just lead to unnecessary confusion IMO.
@dholmes-ora , although I fully agree that this might lead to some
misunderstanding b/w Oracle and non-Oracle folks, I don't see how it's
different from the previous patch, which introduced `hotspot:tier2` and
`hotspot:tier3`. even if we reduce `tierN` to just a set of tests, the test
groups added by 8272914 bear as much resemblance to the test sets used in
Oracle's tier2-3 as the suggested `hotspot:tier4` groups in this patch to the
actual `tier4` definition used in Oracle's internal system, e.g.
`hotspot:tier2` group has 0 tests from `test/hotspot/jtreg/compiler`, but
Oracle's `tier2` does include a number of `test/hotspot/jtreg/compiler` tests
(which aren't part of `:tier1`). I believe that this patch actually moves us
closer to a convergence point, as the union of `hotspot:tier1` --
`hotspot:tier4` test groups is very close to the test sets used in hotspot
parts of Oracle's `tier1` -- `tier4` definitions.
Thanks,
-- Igor
-
PR: https://git.openjdk.java.net/jdk/pull/5357
Re: RFR: 8273314: Add tier4 test groups
On Fri, 3 Sep 2021 18:40:14 GMT, Aleksey Shipilev wrote: > > > <...> I have excluded `vmTestbase` and `hotspot:tier4`<...> I have also > > > excluded `applications` from `hotspot:tier4` <...> > > > > > > assuming the goal of tier4 is to catch the rest of the tests, I don't think > > we should exclude `vmTestbase`, `applications` or any other tests from > > tier4. unless you also want to create tier5 for them. > > Apart from practicality of using `tier4`, I think `vmTestbase` and > `applications` are separate test suites in their own right. `tier4` is > catching all the assorted Hotspot tests that are not part of larger suites. > Makes sense? to some extent. I agree that `applications` tests can/should be seen as a separate test suite, yet I differ on `vmTestbase` as the end goal for `vmTestbase` tests is (and always was) to become just another test within hotspot jtreg test suite, hence I don't think we should treat them any different than other jtreg tests. there is also a plan (which I need to formalize and share w/ a broader audience) to rearrange `vmTestbase` tests so they will be placed within the corresponding component subdirectories, which would bring us closer to the end goal and at the same time make it slightly harder to select all `vmTestbase` tests. -- Igor - PR: https://git.openjdk.java.net/jdk/pull/5357
Re: RFR: 8273314: Add tier4 test groups
On Fri, 3 Sep 2021 09:10:20 GMT, Aleksey Shipilev wrote:
> During the review of JDK-8272914 that added hotspot:tier{2,3} groups,
> @iignatev suggested to create tier4 groups that capture all tests not in
> tiers{1,2,3}. I have excluded `vmTestbase` and `hotspot:tier4,` because they
> take 10+ hours on my highly parallel machine. I have also excluded
> `applications` from `hotspot:tier4`, because they require test dependencies
> (e.g. jcstress).
>
> Sample run:
>
>
> ==
> Test summary
> ==
>TEST TOTAL PASS FAIL ERROR
>
>>> jtreg:test/hotspot/jtreg:tier4 426 425 1 0 <<
>>> jtreg:test/jdk:tier4 2891 2885 4 2 <<
>jtreg:test/langtools:tier40 0 0 0
>
>jtreg:test/jaxp:tier4 0 0 0 0
>
> ==
>
> real 64m13.994s
> user 1462m1.213s
> sys 39m38.032s
>
>
> There are interesting test failures on my machine, which I would address
> separately.
> <...> I have excluded `vmTestbase` and `hotspot:tier4`<...> I have also
> excluded `applications` from `hotspot:tier4` <...>
assuming the goal of tier4 is to catch the rest of the tests, I don't think we
should exclude `vmTestbase`, `applications` or any other tests from tier4.
unless you also want to create tier5 for them.
-- Igor
-
PR: https://git.openjdk.java.net/jdk/pull/5357
Re: RFR: 8272836: Limit run time for java/lang/invoke/LFCaching tests [v2]
On Wed, 25 Aug 2021 16:31:50 GMT, Aleksey Shipilev wrote: >> See the RFE for discussion. >> >> Current PR improves the test time like this: >> >> >> $ make run-test TEST=java/lang/invoke/LFCaching/ >> >> # Before >> real 3m51.608s >> user 5m21.612s >> sys 0m5.391s >> >> # After >> real 1m13.606s >> user 2m26.827s >> sys 0m4.761s > > Aleksey Shipilev 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 three additional > commits since the last revision: > > - Make the timeout depend on global timeout with lower multiplier > - Merge branch 'master' into JDK-8272836-perf-test-lfcaching > - 8272836: Optimize run time for java/lang/invoke/LFCaching tests LGTM - Marked as reviewed by iignatyev (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5214
Re: RFR: 8272836: Limit run time for java/lang/invoke/LFCaching tests
On Mon, 23 Aug 2021 11:33:35 GMT, Aleksey Shipilev wrote: > See the RFE for discussion. > > Current PR improves the test time like this: > > > $ make run-test TEST=java/lang/invoke/LFCaching/ > > # Before > real 3m51.608s > user 5m21.612s > sys 0m5.391s > > # After > real 1m13.606s > user 2m26.827s > sys 0m4.761s the reason we tie time-budget in this test (and other similar stress tests) to timeout is to give a test chance to do actual testing in slow configurations (which will set higher timeout factor), for example, runs w/ -Xcomp on debug builds. if we use hardcoded value, the test might spend (almost) all its allocated time to just init and wouldn't perform any testing. ] so instead of using the hardcode limit, I'd prefer to adjust the multiplication, `0.25` will give you the same 60s w/ current default timeout factor. -- Igor - PR: https://git.openjdk.java.net/jdk/pull/5214
Re: RFR: 8247980: Exclusive execution of java/util/stream tests slows down tier1
On Thu, 19 Aug 2021 15:15:31 GMT, Aleksey Shipilev wrote: > See the bug report for more details. I would appreciate if people with their > corporate testing systems would run this through their systems to avoid > surprises. (ping @RealCLanger, @iignatev). the testing in our infra returned green. - PR: https://git.openjdk.java.net/jdk/pull/5189
Re: RFR: 8247980: Exclusive execution of java/util/stream tests slows down tier1
On Thu, 19 Aug 2021 15:15:31 GMT, Aleksey Shipilev wrote: > See the bug report for more details. I would appreciate if people with their > corporate testing systems would run this through their systems to avoid > surprises. (ping @RealCLanger, @iignatev). Hi @shipilev , I've submitted testing for this patch in our system. meanwhile, I'd like to hear @PaulSandoz's and/or @AlanBateman's option on this as I don't think that the underlying issue, that forced us to add these tests to `exclusiveAccess.dirs` by [8058204](https://bugs.openjdk.java.net/browse/JDK-8058204), was resolved. Thanks, -- Igor - PR: https://git.openjdk.java.net/jdk/pull/5189
Re: RFR: 8272398: Update DockerTestUtils.buildJdkDockerImage() [v2]
On Tue, 17 Aug 2021 14:58:48 GMT, Mikhailo Seledtsov wrote: >> Please review this change that updates the buildJdkDockerImage() test >> library API. >> >> This work originated while working on "8195809: [TESTBUG] jps and jcmd -l >> support for containers is not tested". >> The initial intent was to extend the buildJdkDockerImage() API of >> DockerTestUtils to accept custom Dockerfile content. >> As I analyzed the usage of buildJdkDockerImage() I realized that: >> - 2nd argument "dockerfile" is always the same: "Dockerfile-BasicTest" >> its use has been obsolete for some time, in favor of Dockerfile >> generated by DockerTestUtils >> - 3rd argument "buildDirName" is also always the same: "jdk-docker" >> >> Hence I thought it would be a good idea to simplify this API and make it >> up-to-date. >> >> Also, since the method signature is being updated, I thought it would be a >> good idea to also change the name to use more generic container terminology: >> buildJdkDockerImage() --> buildJdkContainerImage() > > Mikhailo Seledtsov has updated the pull request incrementally with one > additional commit since the last revision: > > Addressing review feedback Looks good and trivial to me. - Marked as reviewed by iignatyev (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5134
Re: RFR: 8272398: Update DockerTestUtils.buildJdkDockerImage()
On Mon, 16 Aug 2021 23:31:41 GMT, Mikhailo Seledtsov
wrote:
> Please review this change that updates the buildJdkDockerImage() test library
> API.
>
> This work originated while working on "8195809: [TESTBUG] jps and jcmd -l
> support for containers is not tested".
> The initial intent was to extend the buildJdkDockerImage() API of
> DockerTestUtils to accept custom Dockerfile content.
> As I analyzed the usage of buildJdkDockerImage() I realized that:
> - 2nd argument "dockerfile" is always the same: "Dockerfile-BasicTest"
> its use has been obsolete for some time, in favor of Dockerfile
> generated by DockerTestUtils
> - 3rd argument "buildDirName" is also always the same: "jdk-docker"
>
> Hence I thought it would be a good idea to simplify this API and make it
> up-to-date.
>
> Also, since the method signature is being updated, I thought it would be a
> good idea to also change the name to use more generic container terminology:
> buildJdkDockerImage() --> buildJdkContainerImage()
Changes requested by iignatyev (Reviewer).
test/lib/jdk/test/lib/containers/docker/DockerTestUtils.java line 145:
> 143:
> 144: // Create an image build/staging directory
> 145: Path buildDir = Paths.get(".", "image-build");
to make it more robust and possible to have more than one container within a
test, I'd use `imagName` + "image-build" as build dir name.
test/lib/jdk/test/lib/containers/docker/DockerTestUtils.java line 179:
> 177: * @throws Exception
> 178: */
> 179: public static void buildImage(String imageName, Path buildDir)
> throws Exception {
it seems there are no users of this method, do we need to keep it public?
-
PR: https://git.openjdk.java.net/jdk/pull/5134
Re: RFR: 8272398: Update DockerTestUtils.buildJdkDockerImage()
On Mon, 16 Aug 2021 23:47:04 GMT, Igor Ignatyev wrote:
>> Please review this change that updates the buildJdkDockerImage() test
>> library API.
>>
>> This work originated while working on "8195809: [TESTBUG] jps and jcmd -l
>> support for containers is not tested".
>> The initial intent was to extend the buildJdkDockerImage() API of
>> DockerTestUtils to accept custom Dockerfile content.
>> As I analyzed the usage of buildJdkDockerImage() I realized that:
>> - 2nd argument "dockerfile" is always the same: "Dockerfile-BasicTest"
>> its use has been obsolete for some time, in favor of Dockerfile
>> generated by DockerTestUtils
>> - 3rd argument "buildDirName" is also always the same: "jdk-docker"
>>
>> Hence I thought it would be a good idea to simplify this API and make it
>> up-to-date.
>>
>> Also, since the method signature is being updated, I thought it would be a
>> good idea to also change the name to use more generic container terminology:
>> buildJdkDockerImage() --> buildJdkContainerImage()
>
> test/lib/jdk/test/lib/containers/docker/DockerTestUtils.java line 145:
>
>> 143:
>> 144: // Create an image build/staging directory
>> 145: Path buildDir = Paths.get(".", "image-build");
>
> to make it more robust and possible to have more than one container within a
> test, I'd use `imagName` + "image-build" as build dir name.
you also don't need to have `.` as a first argument, `Paths.get("x")` returns
you a path to "x" in current directory.
-
PR: https://git.openjdk.java.net/jdk/pull/5134
[jdk17] Integrated: 8067223: [TESTBUG] Rename Whitebox API package
On Wed, 28 Jul 2021 17:13:49 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this big tedious and trivial(-ish) patch which moves > `sun.hotspot.WhiteBox` and related classes to `jdk.test.whitebox` package? > > the majority of the patch is the following substitutions: > - `s~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g` > - `s/sun.hotspot.parser/jdk.test.whitebox.parser/g` > - `s/sun.hotspot.cpuinfo/jdk.test.whitebox.cpuinfo/g` > - `s/sun.hotspot.code/jdk.test.whitebox.code/g` > - `s/sun.hotspot.gc/jdk.test.whitebox.gc/g` > - `s/sun.hotspot.WhiteBox/jdk.test.whitebox.WhiteBox/g` > > testing: tier1-4 > > Thanks, > -- Igor This pull request has now been integrated. Changeset: ada58d13 Author:Igor Ignatyev URL: https://git.openjdk.java.net/jdk17/commit/ada58d13f78eb8a240220c45c573335eeb47cf07 Stats: 532 lines in 36 files changed: 449 ins; 0 del; 83 mod 8067223: [TESTBUG] Rename Whitebox API package Reviewed-by: dholmes, kvn - PR: https://git.openjdk.java.net/jdk17/pull/290
Re: [jdk17] RFR: 8067223: [TESTBUG] Rename Whitebox API package [v2]
On Mon, 2 Aug 2021 15:56:39 GMT, Vladimir Kozlov wrote: > I agree with these revised changes for JDK 17. Thanks for your review, Vladimir. I'll rerun my testing before integrating (just for good luck). -- Igor - PR: https://git.openjdk.java.net/jdk17/pull/290
Re: [jdk17] RFR: 8067223: [TESTBUG] Rename Whitebox API package [v2]
On Sat, 31 Jul 2021 20:42:10 GMT, Igor Ignatyev wrote: >> Hi all, >> >> could you please review this big tedious and trivial(-ish) patch which moves >> `sun.hotspot.WhiteBox` and related classes to `jdk.test.whitebox` package? >> >> the majority of the patch is the following substitutions: >> - `s~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g` >> - `s/sun.hotspot.parser/jdk.test.whitebox.parser/g` >> - `s/sun.hotspot.cpuinfo/jdk.test.whitebox.cpuinfo/g` >> - `s/sun.hotspot.code/jdk.test.whitebox.code/g` >> - `s/sun.hotspot.gc/jdk.test.whitebox.gc/g` >> - `s/sun.hotspot.WhiteBox/jdk.test.whitebox.WhiteBox/g` >> >> testing: tier1-4 >> >> Thanks, >> -- Igor > > Igor Ignatyev has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. The pull request contains 12 new > commits since the last revision: > > - fixed ctw build > - updated runtime/cds/appcds/JarBuilder to copy j.t.w.WhiteBox's inner class > - updated requires.VMProps > - updated TEST.ROOT > - adjusted hotspot source > - added test > - moved and adjusted WhiteBox tests (test/lib-test/sun/hotspot/whitebox) > - updated ClassFileInstaller to copy j.t.w.WhiteBox's inner class > - removed sun/hotspot/parser/DiagnosticCommand > - deprecated sun/hotspot classes >disallowed s.h.WhiteBox w/ security manager > - ... and 2 more: > https://git.openjdk.java.net/jdk17/compare/8f12f2cf...237e8860 Hi David, > This set of changes seems much more manageable to me. thank you for your review, David. > Not sure about the new deprecation warnings for the old WB classes .. might > that not itself trigger some failures? If not then I don't see how the > deprecation warnings help with transitioning to the new WB class? the deprecation warnings (hopefully) will help people not to forget that they should use the new WB class in new tests. Thanks, -- Igor Hi Jie, > Shall we also update the copyright year like > test/lib/sun/hotspot/cpuinfo/CPUInfo.java? we most certainly shall, just pushed the commit that updates the copyright years in the touched files. Cheers, -- Igor - PR: https://git.openjdk.java.net/jdk17/pull/290
Re: [jdk17] RFR: 8067223: [TESTBUG] Rename Whitebox API package [v3]
> Hi all, > > could you please review this big tedious and trivial(-ish) patch which moves > `sun.hotspot.WhiteBox` and related classes to `jdk.test.whitebox` package? > > the majority of the patch is the following substitutions: > - `s~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g` > - `s/sun.hotspot.parser/jdk.test.whitebox.parser/g` > - `s/sun.hotspot.cpuinfo/jdk.test.whitebox.cpuinfo/g` > - `s/sun.hotspot.code/jdk.test.whitebox.code/g` > - `s/sun.hotspot.gc/jdk.test.whitebox.gc/g` > - `s/sun.hotspot.WhiteBox/jdk.test.whitebox.WhiteBox/g` > > testing: tier1-4 > > Thanks, > -- Igor Igor Ignatyev has updated the pull request incrementally with two additional commits since the last revision: - copyright update - fixed typo in ClassFileInstaller - Changes: - all: https://git.openjdk.java.net/jdk17/pull/290/files - new: https://git.openjdk.java.net/jdk17/pull/290/files/237e8860..fcaf41f8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17=290=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=290=01-02 Stats: 10 lines in 10 files changed: 0 ins; 0 del; 10 mod Patch: https://git.openjdk.java.net/jdk17/pull/290.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/290/head:pull/290 PR: https://git.openjdk.java.net/jdk17/pull/290
Re: [jdk17] RFR: 8067223: [TESTBUG] Rename Whitebox API package [v2]
On Sat, 31 Jul 2021 20:42:10 GMT, Igor Ignatyev wrote: >> Hi all, >> >> could you please review this big tedious and trivial(-ish) patch which moves >> `sun.hotspot.WhiteBox` and related classes to `jdk.test.whitebox` package? >> >> the majority of the patch is the following substitutions: >> - `s~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g` >> - `s/sun.hotspot.parser/jdk.test.whitebox.parser/g` >> - `s/sun.hotspot.cpuinfo/jdk.test.whitebox.cpuinfo/g` >> - `s/sun.hotspot.code/jdk.test.whitebox.code/g` >> - `s/sun.hotspot.gc/jdk.test.whitebox.gc/g` >> - `s/sun.hotspot.WhiteBox/jdk.test.whitebox.WhiteBox/g` >> >> testing: tier1-4 >> >> Thanks, >> -- Igor > > Igor Ignatyev has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. Vladimir, David, I've (forced) pushed a smaller version of the renaming. instead of removing `sun.hotspot` classes, it copies them to `jdk.test.whitebox` (w/ `s.h.parser.DiagnosticCommand` being removed as it's used in WhiteBox signature and it was easier to update a few tests that use it), updates hotspot code to register native methods for both `sun.hotspot.WhiteBox` and `jdk.test.whitebox.WhiteBox` classes. To make it easier and not to introduce extra dependency, I've made it impossible to use `s.h.WB` w/ a security manager enabled, otherwise there would be a dependency b/w `s.h.WB` and `j.t.w.WB$WhiteBoxPermission` or there would be 2 permissions. There are no open JDK tests that are impacted by this limitation. With minor tweaks in closed source, the patch successfully passes Oracle's tier1-4. -- Igor - PR: https://git.openjdk.java.net/jdk17/pull/290
Re: [jdk17] RFR: 8067223: [TESTBUG] Rename Whitebox API package [v2]
> Hi all, > > could you please review this big tedious and trivial(-ish) patch which moves > `sun.hotspot.WhiteBox` and related classes to `jdk.test.whitebox` package? > > the majority of the patch is the following substitutions: > - `s~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g` > - `s/sun.hotspot.parser/jdk.test.whitebox.parser/g` > - `s/sun.hotspot.cpuinfo/jdk.test.whitebox.cpuinfo/g` > - `s/sun.hotspot.code/jdk.test.whitebox.code/g` > - `s/sun.hotspot.gc/jdk.test.whitebox.gc/g` > - `s/sun.hotspot.WhiteBox/jdk.test.whitebox.WhiteBox/g` > > testing: tier1-4 > > Thanks, > -- Igor Igor Ignatyev has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains 12 new commits since the last revision: - fixed ctw build - updated runtime/cds/appcds/JarBuilder to copy j.t.w.WhiteBox's inner class - updated requires.VMProps - updated TEST.ROOT - adjusted hotspot source - added test - moved and adjusted WhiteBox tests (test/lib-test/sun/hotspot/whitebox) - updated ClassFileInstaller to copy j.t.w.WhiteBox's inner class - removed sun/hotspot/parser/DiagnosticCommand - deprecated sun/hotspot classes disallowed s.h.WhiteBox w/ security manager - ... and 2 more: https://git.openjdk.java.net/jdk17/compare/8f12f2cf...237e8860 - Changes: - all: https://git.openjdk.java.net/jdk17/pull/290/files - new: https://git.openjdk.java.net/jdk17/pull/290/files/8f12f2cf..237e8860 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17=290=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=290=00-01 Stats: 3248 lines in 939 files changed: 969 ins; 0 del; 2279 mod Patch: https://git.openjdk.java.net/jdk17/pull/290.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/290/head:pull/290 PR: https://git.openjdk.java.net/jdk17/pull/290
Re: [jdk17] RFR: 8067223: [TESTBUG] Rename Whitebox API package
On Thu, 29 Jul 2021 01:30:37 GMT, Vladimir Kozlov wrote: >> Hi all, >> >> could you please review this big tedious and trivial(-ish) patch which moves >> `sun.hotspot.WhiteBox` and related classes to `jdk.test.whitebox` package? >> >> the majority of the patch is the following substitutions: >> - `s~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g` >> - `s/sun.hotspot.parser/jdk.test.whitebox.parser/g` >> - `s/sun.hotspot.cpuinfo/jdk.test.whitebox.cpuinfo/g` >> - `s/sun.hotspot.code/jdk.test.whitebox.code/g` >> - `s/sun.hotspot.gc/jdk.test.whitebox.gc/g` >> - `s/sun.hotspot.WhiteBox/jdk.test.whitebox.WhiteBox/g` >> >> testing: tier1-4 >> >> Thanks, >> -- Igor > > I know that tests fixes could be pushed during RDP2 without approval. > But these one is very big and it is not a fix - it is enhancement. > > What is the reason you want to push it into JDK 17 just few days before first > Release Candidate? Instead of pushing it into JDK 18. > > And I can't even review it because GutHub UI hangs on these big changes. @vnkozlov, @dholmes-ora, Thank you for looking at this! I want this to be integrated into JDK 17 b/c some "external" libraries use (used to use) WhiteBox API, e.g. jcstress[[2]] used WhiteBox API to deoptimize compiled methods[[3]], and it would be easier for maintainers of such libraries to condition package name based on JDK major version. Also, given JDK 17 is an LTS, it would be beneficial for everyone not to have big differences in test bases b/w it and the mainline. according to JEP 3, test RFEs are allowed until the very end and don't require late enhancement approval: "Enhancements to tests and documentation during RDP 1 and RDP 2 do not require approval, as long as the relevant issues are identified with a noreg-self or noreg-doc label, as appropriate"[[1]]. So, process-wise, I don't see any issues w/ integrating this RFE, yet, I fully agree that due to its size, this patch can be disruptive and can cause massive failures, which is something we obviously don't want at the current stage of JDK 17. I like David's idea about phasing this clean-up, and, due to the reasons described above, I would like to integrate the first part, copying WhiteBox classes to the new package structure and associated changes w/o updating all the tests, into JDK 17 and update the tests on the mainline (w/ backporting into jdk17u). WDYT? Cheers, -- Igor [1]: https://openjdk.java.net/jeps/3#Late-Enhancement-Request-Process [2]: https://github.com/openjdk/jcstress [3]: https://github.com/openjdk/jcstress/blob/df83b446f187ae0b0fa31fa54decb59db9e955da/jcstress-core/src/main/java/org/openjdk/jcstress/vm/WhiteBoxSupport.java - PR: https://git.openjdk.java.net/jdk17/pull/290
[jdk17] RFR: 8067223: [TESTBUG] Rename Whitebox API package
Hi all, could you please review this big tedious and trivial(-ish) patch which moves `sun.hotspot.WhiteBox` and related classes to `jdk.test.whitebox` package? the majority of the patch is the following substitutions: - `s~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g` - `s/sun.hotspot.parser/jdk.test.whitebox.parser/g` - `s/sun.hotspot.cpuinfo/jdk.test.whitebox.cpuinfo/g` - `s/sun.hotspot.code/jdk.test.whitebox.code/g` - `s/sun.hotspot.gc/jdk.test.whitebox.gc/g` - `s/sun.hotspot.WhiteBox/jdk.test.whitebox.WhiteBox/g` testing: tier1-4 Thanks, -- Igor - Commit messages: - copyright year - update TEST.ROOT - jdk: s/sun\.hotspot\.gc/jdk.test.whitebox.gc/g - jdk: s/sun\.hotspot\.code/jdk.test.whitebox.code/g - jdk: s/sun\.hotspot\.WhiteBox/jdk.test.whitebox.WhiteBox/g - hotspot: 's~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g' - hotspot: s/sun\.hotspot\.parser/jdk.test.whitebox.parser/g - hotspot: s/sun\.hotspot\.cpuinfo/jdk.test.whitebox.cpuinfo/g - hotspot: s/sun\.hotspot\.code/jdk.test.whitebox.code/g - hotspot: 's/sun\.hotspot\.gc/jdk.test.whitebox.gc/g' - ... and 9 more: https://git.openjdk.java.net/jdk17/compare/c8ae7e5b...8f12f2cf Changes: https://git.openjdk.java.net/jdk17/pull/290/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=290=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8067223 Stats: 2310 lines in 949 files changed: 0 ins; 0 del; 2310 mod Patch: https://git.openjdk.java.net/jdk17/pull/290.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/290/head:pull/290 PR: https://git.openjdk.java.net/jdk17/pull/290
Re: [jdk17] RFR: 8269534: Remove java/util/concurrent/locks/Lock/TimedAcquireLeak.java from ProblemList.txt
On Mon, 28 Jun 2021 17:05:49 GMT, Daniel D. Daugherty wrote: > A trivial fix to remove java/util/concurrent/locks/Lock/TimedAcquireLeak.java > from ProblemList.txt Marked as reviewed by iignatyev (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/164
Re: RFR: 8267448: Add "ulimit -a" to environment.html
On Thu, 10 Jun 2021 06:26:53 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this small patch that does $subj? > > Thanks, > -- Igor > > attn: @plummercj closing in favor of openjdk/jdk17#2 - PR: https://git.openjdk.java.net/jdk/pull/4451
Withdrawn: 8267448: Add "ulimit -a" to environment.html
On Thu, 10 Jun 2021 06:26:53 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this small patch that does $subj? > > Thanks, > -- Igor > > attn: @plummercj This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/4451
RFR: 8267448: Add "ulimit -a" to environment.html
Hi all, could you please review this small patch that does $subj? Thanks, -- Igor attn: @plummercj - Commit messages: - 8267448 Changes: https://git.openjdk.java.net/jdk/pull/4451/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4451=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8267448 Stats: 15 lines in 3 files changed: 15 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4451.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4451/head:pull/4451 PR: https://git.openjdk.java.net/jdk/pull/4451
Re: RFR: 8267893: Improve jtreg test failure handler do get native/mixed stack traces for cores and live processes [v6]
On Wed, 9 Jun 2021 19:00:39 GMT, Leonid Mesnik wrote: >> EFH is improved to process cores and get mixed stack traces with jhsdb and >> native stack traces with gdb/lldb. It might be useful because hs_err doesn't >> contain info about all threads, sometimes it is even not generated. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > fxies Marked as reviewed by iignatyev (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4234
Re: RFR: 8267893: Improve jtreg test failure handler do get native/mixed stack traces for cores and live processes [v5]
On Wed, 2 Jun 2021 01:00:53 GMT, Leonid Mesnik wrote: >> EFH is improved to process cores and get mixed stack traces with jhsdb and >> native stack traces with gdb/lldb. It might be useful because hs_err doesn't >> contain info about all threads, sometimes it is even not generated. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > spaces updated. Changes requested by iignatyev (Reviewer). test/failure_handler/src/share/classes/jdk/test/failurehandler/GathererFactory.java line 32: > 30: import java.io.FileWriter; > 31: import java.io.PrintWriter; > 32: import java.nio.file.Files; I don't see why we need these 3 new imports. test/failure_handler/src/share/classes/jdk/test/failurehandler/ToolKit.java line 28: > 26: import jdk.test.failurehandler.action.ActionSet; > 27: import jdk.test.failurehandler.action.ActionHelper; > 28: import jdk.test.failurehandler.action.PatternAction; redundant import test/failure_handler/src/share/conf/linux.properties line 62: > 60: cores=native.gdb > 61: native.gdb.app=gdb > 62: native.gdb.args=%java\0-c\0%p\0-batch\0-ex\0thread apply all backtrace could you please add a comment similar to the one in `common.properties` file? test/failure_handler/src/share/conf/mac.properties line 71: > 69: native.lldb.app=lldb > 70: native.lldb.delimiter=\0 > 71: native.lldb.args=--core\0%p\0%java\0-o\0thread backtrace all\0-o\0quit could you please add a comment similar to the one in common.properties file? test/failure_handler/src/share/conf/mac.properties line 72: > 70: native.lldb.delimiter=\0 > 71: native.lldb.args=--core\0%p\0%java\0-o\0thread backtrace all\0-o\0quit > 72: native.lldb.params.timeout=360 why does `lldb` require an increases timeout, but `gdb` and `jhsdb` do not? - PR: https://git.openjdk.java.net/jdk/pull/4234
Re: RFR: 8267893: Improve jtreg test failure handler do get native/mixed stack traces for cores and live processes
On Thu, 27 May 2021 22:05:55 GMT, Leonid Mesnik wrote:
> EFH is improved to process cores and get mixed stack traces with jhsdb and
> native stack traces with gdb/lldb. It might be useful because hs_err doesn't
> contain info about all threads, sometimes it is even not generated.
@lmesnik , how has this been tested?
test/failure_handler/Makefile line 41:
> 39: CONF_DIR = src/share/conf
> 40:
> 41: JAVA_RELEASE = 7
could you please update `DEPENDENCES` section in `test/failure_handler/README`?
test/failure_handler/src/share/classes/jdk/test/failurehandler/ToolKit.java
line 69:
> 67: }
> 68: } catch (IOException ioe) {
> 69: // just silently skip gzipped core processing
we don't silently ignore exceptions in `failure_handler`, we tend to print them
so we at least have some echoes of what happened.
test/failure_handler/src/share/classes/jdk/test/failurehandler/ToolKit.java
line 71:
> 69: // just silently skip gzipped core processing
> 70: }
> 71: unpackedCore.toFile().delete();
Suggestion:
Paths.delete(unpackedCore);
``` ?
test/failure_handler/src/share/classes/jdk/test/failurehandler/Utils.java line
73:
> 71: InputStream stream =
> Utils.class.getResourceAsStream(resourceName);
> 72:
> 73: // EFH_CONF_DIR might re-defined to load custom configs for
> development purposes
this also seems to be unrelated to the subject and does require documentation
in `test/failure_handler/README`.
test/failure_handler/src/share/classes/jdk/test/failurehandler/action/PatternAction.java
line 63:
> 61: }
> 62: for (int i = 0, n = args.length; i < n; ++i) {
> 63: args[i] = args[i].replace("%java",
> helper.findApp("java").getAbsolutePath());
are we sure that `java` from `PATH` (which is what `findApp` returns) is the
right `java`?
test/failure_handler/src/share/conf/common.properties line 38:
> 36: jcmd.vm.system_properties \
> 37: jcmd.gc.heap_info jcmd.gc.class_histogram jcmd.gc.finalizer_info \
> 38: jcmd.vm.info \
this seems to be unrelated to the RFE you are working on.
test/failure_handler/src/share/conf/common.properties line 67:
> 65: cores=jhsdb.jstack
> 66: jhsdb.jstack.app=jhsdb
> 67: jhsdb.jstack.args=jstack --mixed --core %p --exe %java
strictly speaking, we can't guarantee that the executable is always `bin/java`,
but it's the most common and the most interesting case, so this assumption is
good enough for our pourposes.
could you please add a comment explaining this so the further reading won't be
puzzled?
test/failure_handler/src/share/conf/mac.properties line 68:
> 66: native.jhsdb.app=bash
> 67: native.jhsdb.jstack.delimiter=\0
> 68: native.jhsdb.jstack.args=-c\0DevToolsSecurity --status | grep -q enabled
> && jhsdb jstack --mixed --pid %p
AFAICS `jhsdb` does check "Developer mode" on macos before trying to attach and
will just report 'lack of privileges' (as opposed to hanging with a modal
window asking for permission), so you can move `jshdb.jstack` to
`common.properties`.
-
Changes requested by iignatyev (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4234
Re: RFR: 8267751: (test) jtreg.SkippedException has no serial VersionUID [v2]
On Wed, 26 May 2021 17:52:31 GMT, Roger Riggs wrote: >> The class `test/lib/jtreg/SkippedException.java` is missing a >> serialVersionUID causing additional noise in compiler output of tests. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Removed unintended classfile LGTM - Marked as reviewed by iignatyev (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4197
Re: RFR: 8264806: Remove the experimental JIT compiler [v2]
On Tue, 13 Apr 2021 09:30:23 GMT, Doug Simon wrote: > I guess this should really be named `isJVMCICompilerEnabled` now and the > `vm.graal.enabled` predicate renamed to `vm.jvmcicompiler.enabled` but maybe > that's too big a change (or can be done later). @dougxc, I don't think that we should (or even can) rename it to `vm.jvmcicompiler.enabled`. although the value of the property is indeed `true` whenever a jvmci compiler is enabled, it is used to filter out tests that are incompatible with a particular compiler -- graal. so what we can do is to change `requires/VMProps.java` to set `vm.graal.enabled` only if the jvmci compiler is indeed graal, but on the other hand, we haven't heard anyone complaining that the test coverage for their jvmci compiler has been reduced, so I don't see much of the benefit here. -- Igor - PR: https://git.openjdk.java.net/jdk/pull/3421
Re: RFR: 8264806: Remove the experimental JIT compiler [v2]
On Sat, 10 Apr 2021 16:36:54 GMT, Vladimir Kozlov wrote: >> should we remove `sun.hotspot.code.Compiler::isGraalEnabled` method and >> update a few of its users accordingly? >> what about `vm.graal.enabled` `@requires` property? > > @iignatev If you think that I should clean tests anyway I will file follow > up RFE to do that. > > should we remove `sun.hotspot.code.Compiler::isGraalEnabled` method and > > update a few of its users accordingly? > > what about `vm.graal.enabled` `@requires` property? > > Thank you, @iignatev for looking on changes. > > I forgot to mention that `Compiler::isGraalEnabled()` returns always false > now. Because 94 tests uses `@requires !vm.graal.enabled` I don't want to > include them in these changes which are already very big. I am not sure if I > should modify tests if GraalVM group wants to run all these tests. > If you think that I should clean tests anyway I will file follow up RFE to do > that. changing `Compiler::isGraalEnabled()` to always return false effectively makes these tests unrunnable for GraalVM group (unless they are keep the modified `sun/hotspot/code/Compiler` and/or `requires/VMProps` in their forks). on top of that, I foresee that there will be more tests incompatible w/ Graal yet given it won't be run/tested in OpenJDK, these tests won't be marked and hence will fail when run w/ Graal. so Graal people will have to either do marking themselves (I guess in both upstream and their fork) or maintain a list of inapplicable tests in a format that works best for their setup. that's to say, I think we should clean up the tests, yet I totally agree there is no need to do it as part of this PR. we can discuss how to do it better for both OpenJDK and GraalVM folks in the follow-up RFE. - PR: https://git.openjdk.java.net/jdk/pull/3421
Re: RFR: 8264806: Remove the experimental JIT compiler [v2]
On Fri, 9 Apr 2021 22:26:40 GMT, Vladimir Kozlov wrote: >> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related >> to Java-based JIT compiler (Graal) from JDK: >> >> - `jdk.internal.vm.compiler` — the Graal compiler >> - `jdk.internal.vm.compiler.management` — Graal's `MBean` >> - `test/hotspot/jtreg/compiler/graalunit` — Graal's unit tests >> >> Remove Graal related code in makefiles. >> >> Note, next two `module-info.java` files are preserved so that the JVMCI >> module `jdk.internal.vm.ci` continues to build: >> src/jdk.internal.vm.compiler/share/classes/module-info.java >> src/jdk.internal.vm.compiler.management/share/classes/module-info.java >> >> @AlanBateman suggested that we can avoid it by using Module API to export >> packages at runtime . It requires changes in GraalVM's JVMCI too so I will >> file followup RFE to implement it. >> >> Tested hs-tier1-4 > > Vladimir Kozlov 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 three additional > commits since the last revision: > > - Restore Graal Builder image makefile > - Merge latest changes from branch 'JDK-8264805' into JDK-8264806 > - 8264806: Remove the experimental JIT compiler Marked as reviewed by iignatyev (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3421
Re: RFR: 8264806: Remove the experimental JIT compiler [v2]
On Fri, 9 Apr 2021 22:26:40 GMT, Vladimir Kozlov wrote: >> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related >> to Java-based JIT compiler (Graal) from JDK: >> >> - `jdk.internal.vm.compiler` — the Graal compiler >> - `jdk.internal.vm.compiler.management` — Graal's `MBean` >> - `test/hotspot/jtreg/compiler/graalunit` — Graal's unit tests >> >> Remove Graal related code in makefiles. >> >> Note, next two `module-info.java` files are preserved so that the JVMCI >> module `jdk.internal.vm.ci` continues to build: >> src/jdk.internal.vm.compiler/share/classes/module-info.java >> src/jdk.internal.vm.compiler.management/share/classes/module-info.java >> >> @AlanBateman suggested that we can avoid it by using Module API to export >> packages at runtime . It requires changes in GraalVM's JVMCI too so I will >> file followup RFE to implement it. >> >> Tested hs-tier1-4 > > Vladimir Kozlov 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 three additional > commits since the last revision: > > - Restore Graal Builder image makefile > - Merge latest changes from branch 'JDK-8264805' into JDK-8264806 > - 8264806: Remove the experimental JIT compiler should we remove `sun.hotspot.code.Compiler::isGraalEnabled` method and update a few of its users accordingly? what about `vm.graal.enabled` `@requires` property? - Changes requested by iignatyev (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3421
Re: RFR: 8264806: Remove the experimental JIT compiler
On Fri, 9 Apr 2021 22:30:32 GMT, Vladimir Kozlov wrote: >> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related >> to Java-based JIT compiler (Graal) from JDK: >> >> - `jdk.internal.vm.compiler` — the Graal compiler >> - `jdk.internal.vm.compiler.management` — Graal's `MBean` >> - `test/hotspot/jtreg/compiler/graalunit` — Graal's unit tests >> >> Remove Graal related code in makefiles. >> >> Note, next two `module-info.java` files are preserved so that the JVMCI >> module `jdk.internal.vm.ci` continues to build: >> src/jdk.internal.vm.compiler/share/classes/module-info.java >> src/jdk.internal.vm.compiler.management/share/classes/module-info.java >> >> @AlanBateman suggested that we can avoid it by using Module API to export >> packages at runtime . It requires changes in GraalVM's JVMCI too so I will >> file followup RFE to implement it. >> >> Tested hs-tier1-4 > > Thankyou, @erikj79, for review. I restored code as you asked. > For some reasons incremental webrev shows update only in Cdiffs. none of the full webrevs seem to render even the list of changed files? is it just me? - PR: https://git.openjdk.java.net/jdk/pull/3421
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Thu, 8 Apr 2021 17:24:38 GMT, Vladimir Kozlov wrote: >> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related >> to Ahead-of-Time Compiler from JDK: >> >> - `jdk.aot` module — the `jaotc` tool >> - `src/hotspot/share/aot` — loads AoT compiled code into VM for execution >> - related HotSpot code guarded by `#if INCLUDE_AOT` >> >> Additionally, remove tests as well as code in makefiles related to AoT >> compilation. >> >> Tested hs-tier1-4 > > Vladimir Kozlov has updated the pull request incrementally with one > additional commit since the last revision: > > Remove exports from Graal module to jdk.aot Test changes look good to me. - Marked as reviewed by iignatyev (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3398
Integrated: 8263556: remove `@modules java.base` from tests
On Sat, 13 Mar 2021 20:26:42 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this trivial cleanup? > from JBS: > >> jtreg `@modules X` directive does two things: >> - exclude a test from execution if JDK under test doesn't have module X >> - if JDK under test has module X, make sure it's resolved >> >> both these things have no sense for `java.base` module as it's always >> available and is always resolved. > > > Thanks, > -- Igor This pull request has now been integrated. Changeset: d825198e Author:Igor Ignatyev URL: https://git.openjdk.java.net/jdk/commit/d825198e Stats: 21 lines in 13 files changed: 0 ins; 13 del; 8 mod 8263556: remove `@modules java.base` from tests Reviewed-by: dcubed, naoto, iris - PR: https://git.openjdk.java.net/jdk/pull/2990
Re: RFR: 8263556: remove `@modules java.base` from tests
On Mon, 15 Mar 2021 16:25:48 GMT, Iris Clark wrote: >> Hi all, >> >> could you please review this trivial cleanup? >> from JBS: >> >>> jtreg `@modules X` directive does two things: >>> - exclude a test from execution if JDK under test doesn't have module X >>> - if JDK under test has module X, make sure it's resolved >>> >>> both these things have no sense for `java.base` module as it's always >>> available and is always resolved. >> >> >> Thanks, >> -- Igor > > Marked as reviewed by iris (Reviewer). Iris, Naoto, Dan, thank you for your reviews! -- Igor - PR: https://git.openjdk.java.net/jdk/pull/2990
RFR: 8263556: remove `@modules java.base` from tests
Hi all, could you please review this trivial cleanup? from JBS: > jtreg `@modules X` directive does two things: > - exclude a test from execution if JDK under test doesn't have module X > - if JDK under test has module X, make sure it's resolved > > both these things have no sense for `java.base` module as it's always > available and is always resolved. Thanks, -- Igor - Commit messages: - update copyright year - 8263556: remove `@modules java.base` from tests Changes: https://git.openjdk.java.net/jdk/pull/2990/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2990=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8263556 Stats: 21 lines in 13 files changed: 0 ins; 13 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/2990.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2990/head:pull/2990 PR: https://git.openjdk.java.net/jdk/pull/2990
Re: RFR: 8263549: 8263412 can cause jtreg testlibrary split [v3]
On Sat, 13 Mar 2021 14:20:20 GMT, Daniel D. Daugherty wrote: >> Igor Ignatyev has refreshed the contents of this pull request, and previous >> commits have been removed. The incremental views will show differences >> compared to the previous content of the PR. > > I downloaded the patch and used Ioi's cmd pattern to scroll through > the changes. I can't honestly say that I looked at every line since 867 > changed files would overwhelm anyone's brain... > > I did notice a couple of `@run main` instead of `@run driver` calls > to the ClassFileInstaller, but those are pre-existing. > > Thumbs up. Hi Dan, Thanks for your review! > I did notice a couple of @run main instead of @run driver calls to the > ClassFileInstaller, but those are pre-existing. I noticed this too, planning to fix that with a separate RFE. -- Igor - PR: https://git.openjdk.java.net/jdk/pull/2985
Integrated: 8263549: 8263412 can cause jtreg testlibrary split
On Sat, 13 Mar 2021 04:31:31 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this dull patch that replaces `ClassFileInstaller` w/ > `jdk.test.lib.helpers.ClassFileInstaller` in all jtreg test descriptions to > ensure we won't get split testlibrary, and removes > `jdk/test/lib/ClassFileInstaller.java` (so it won't be accidentally used). > > from JBS: >> after JDK-8263412, we might (again) encounter NCDFE b/c parts of >> testlibraries aren't on the classpath. this happens when jtreg builds >> `jdk.test.lib.helpers.ClassFileInstaller` as a part of test-specific code, >> but `ClassFileInstaller` as part of shared testibrary directory in one test, >> when in the following test, jtreg sees `ClassFileInstaller` in the shared >> directory, hence javac won't recompile it/its dependencies, but in runtime >> `jdk.test.lib.helpers.ClassFileInstaller` is nowhere to be found, hence we >> get NCDFE. > > testing: > - [x] `grep ' ClassFileInstaller[^.]` > - [ ] tier1-3 > > Thanks, > -- Igor This pull request has now been integrated. Changeset: a7aba2b6 Author:Igor Ignatyev URL: https://git.openjdk.java.net/jdk/commit/a7aba2b6 Stats: 1738 lines in 867 files changed: 2 ins; 67 del; 1669 mod 8263549: 8263412 can cause jtreg testlibrary split Reviewed-by: iklam, dcubed - PR: https://git.openjdk.java.net/jdk/pull/2985
Re: RFR: 8263549: 8263412 can cause jtreg testlibrary split [v3]
On Sat, 13 Mar 2021 06:16:37 GMT, Ioi Lam wrote: >> Igor Ignatyev has refreshed the contents of this pull request, and previous >> commits have been removed. The incremental views will show differences >> compared to the previous content of the PR. The pull request contains one >> new commit since the last revision: >> >> fix compilation error in IncorrectAOTLibraryTest test > > I did this and scanned the differences (with the diff file from the webrev) > and it looks reasonable to me. > > grep '^[+-]' diff.txt | grep -v Copyright | grep -v '^.[+-]' | less > > It looks like most of the changes are mechanical. There were only a few cases > where manual changes were made. I trusted that you have tested those cases > individually. > > But I don't understand why this error can happen. It seems like jtreg would > allow two test cases to interfere with each other. Hi Ioi, thanks for review this, I ran the whole tier1-3 jobs which should provide enough coverage. as oracle builds don't have AOT feature enabled, I missed a compilation error in `IncorrectAOTLibraryTest` test. the test failed in GitHub action and should be fixed by [3a3b7a8](https://github.com/openjdk/jdk/pull/2985/commits/3a3b7a846289181b466b3c1eb478a0a571d9468b). -- Igor - PR: https://git.openjdk.java.net/jdk/pull/2985
Re: RFR: 8263549: 8263412 can cause jtreg testlibrary split [v3]
> Hi all, > > could you please review this dull patch that replaces `ClassFileInstaller` w/ > `jdk.test.lib.helpers.ClassFileInstaller` in all jtreg test descriptions to > ensure we won't get split testlibrary, and removes > `jdk/test/lib/ClassFileInstaller.java` (so it won't be accidentally used). > > from JBS: >> after JDK-8263412, we might (again) encounter NCDFE b/c parts of >> testlibraries aren't on the classpath. this happens when jtreg builds >> `jdk.test.lib.helpers.ClassFileInstaller` as a part of test-specific code, >> but `ClassFileInstaller` as part of shared testibrary directory in one test, >> when in the following test, jtreg sees `ClassFileInstaller` in the shared >> directory, hence javac won't recompile it/its dependencies, but in runtime >> `jdk.test.lib.helpers.ClassFileInstaller` is nowhere to be found, hence we >> get NCDFE. > > testing: > - [x] `grep ' ClassFileInstaller[^.]` > - [ ] tier1-3 > > Thanks, > -- Igor Igor Ignatyev has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: fix compilation error in IncorrectAOTLibraryTest test - Changes: - all: https://git.openjdk.java.net/jdk/pull/2985/files - new: https://git.openjdk.java.net/jdk/pull/2985/files/ff6d4f91..3a3b7a84 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2985=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2985=01-02 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2985.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2985/head:pull/2985 PR: https://git.openjdk.java.net/jdk/pull/2985
Re: RFR: 8263549: 8263412 can cause jtreg testlibrary split [v2]
> Hi all, > > could you please review this dull patch that replaces `ClassFileInstaller` w/ > `jdk.test.lib.helpers.ClassFileInstaller` in all jtreg test descriptions to > ensure we won't get split testlibrary, and removes > `jdk/test/lib/ClassFileInstaller.java` (so it won't be accidentally used). > > from JBS: >> after JDK-8263412, we might (again) encounter NCDFE b/c parts of >> testlibraries aren't on the classpath. this happens when jtreg builds >> `jdk.test.lib.helpers.ClassFileInstaller` as a part of test-specific code, >> but `ClassFileInstaller` as part of shared testibrary directory in one test, >> when in the following test, jtreg sees `ClassFileInstaller` in the shared >> directory, hence javac won't recompile it/its dependencies, but in runtime >> `jdk.test.lib.helpers.ClassFileInstaller` is nowhere to be found, hence we >> get NCDFE. > > testing: > - [x] `grep ' ClassFileInstaller[^.]` > - [ ] tier1-3 > > Thanks, > -- Igor Igor Ignatyev has updated the pull request incrementally with one additional commit since the last revision: fix compilation error in IncorrectAOTLibraryTest test - Changes: - all: https://git.openjdk.java.net/jdk/pull/2985/files - new: https://git.openjdk.java.net/jdk/pull/2985/files/6e53ad97..ff6d4f91 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2985=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2985=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2985.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2985/head:pull/2985 PR: https://git.openjdk.java.net/jdk/pull/2985
Re: RFR: 8263549: 8263412 can cause jtreg testlibrary split
On Sat, 13 Mar 2021 04:31:31 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this dull patch that replaces `ClassFileInstaller` w/ > `jdk.test.lib.helpers.ClassFileInstaller` in all jtreg test descriptions to > ensure we won't get split testlibrary, and removes > `jdk/test/lib/ClassFileInstaller.java` (so it won't be accidentally used). > > from JBS: >> after JDK-8263412, we might (again) encounter NCDFE b/c parts of >> testlibraries aren't on the classpath. this happens when jtreg builds >> `jdk.test.lib.helpers.ClassFileInstaller` as a part of test-specific code, >> but `ClassFileInstaller` as part of shared testibrary directory in one test, >> when in the following test, jtreg sees `ClassFileInstaller` in the shared >> directory, hence javac won't recompile it/its dependencies, but in runtime >> `jdk.test.lib.helpers.ClassFileInstaller` is nowhere to be found, hence we >> get NCDFE. > > testing: > - [x] `grep ' ClassFileInstaller[^.]` > - [ ] tier1-3 > > Thanks, > -- Igor note for reviewers: the big part of the patch is just `sed -i 's/ ClassFileInstaller / jdk.test.lib.helpers.ClassFileInstaller /g'` - PR: https://git.openjdk.java.net/jdk/pull/2985
RFR: 8263549: 8263412 can cause jtreg testlibrary split
Hi all, could you please review this dull patch that replaces `ClassFileInstaller` w/ `jdk.test.lib.helpers.ClassFileInstaller` in all jtreg test descriptions to ensure we won't get split testlibrary, and removes `jdk/test/lib/ClassFileInstaller.java` (so it won't be accidentally used). from JBS: > after JDK-8263412, we might (again) encounter NCDFE b/c parts of > testlibraries aren't on the classpath. this happens when jtreg builds > `jdk.test.lib.helpers.ClassFileInstaller` as a part of test-specific code, > but `ClassFileInstaller` as part of shared testibrary directory in one test, > when in the following test, jtreg sees `ClassFileInstaller` in the shared > directory, hence javac won't recompile it/its dependencies, but in runtime > `jdk.test.lib.helpers.ClassFileInstaller` is nowhere to be found, hence we > get NCDFE. testing: - [x] `grep ' ClassFileInstaller[^.]` - [ ] tier1-3 Thanks, -- Igor - Commit messages: - fixup - update copyright year - rm test/lib/ClassFileInstaller.java - 's/ ClassFileInstaller / jdk.test.lib.helpers.ClassFileInstaller /g' Changes: https://git.openjdk.java.net/jdk/pull/2985/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2985=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8263549 Stats: 1736 lines in 867 files changed: 0 ins; 67 del; 1669 mod Patch: https://git.openjdk.java.net/jdk/pull/2985.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2985/head:pull/2985 PR: https://git.openjdk.java.net/jdk/pull/2985
Integrated: 8263412: ClassFileInstaller can't be used by classes outside of default package
On Thu, 11 Mar 2021 05:47:00 GMT, Igor Ignatyev wrote:
> Hi all,
>
> could you please review the patch which moves `ClassFileInstaller` class to
> `jdk.test.lib.helpers` package?
> to reduce changes in the tests, `ClassFileInstaller` in the default package
> is kept w/ just `main` method that calls `jdk.test.lib.helpers.
> ClassFileInstaller::main`.
>
> from JBS:
>> ClassFileInstaller is in the default package hence it's impossible to use it
>> directly by classes in any other package. although ClassFileInstaller is
>> mainly used directly from jtreg test description, there are cases when one
>> needs to use it in another "driver" class (e.g. to reduce boilerplate), yet
>> to do. that these driver classes have to either be in a default package or
>> use reflection, both approaches have drawbacks.
>>
>> to solve that, we can move ClassFileInstaller implementation to a package,
>> and to avoid unneeded churn, keep package-less ClassFileInstaller class
>> which will call package-full impl.
>>
>> the need for this patch has raised as part of
>> [JDK-8254129](https://bugs.openjdk.java.net/browse/JDK-8254129).
>
> testing:
> - [x] `:jdk_core` against `{macos,windows,linux}-x64`
> - [x] `:jdk_svc` against `{macos,windows,linux}-x64`
> - [x] `test/hotspot/jtreg` w/o `applications` and `vmTestbase` directories
> against `{macos,windows,linux}-x64`
>
> Thanks,
> -- Igor
This pull request has now been integrated.
Changeset: e834f99d
Author:Igor Ignatyev
URL: https://git.openjdk.java.net/jdk/commit/e834f99d
Stats: 472 lines in 114 files changed: 145 ins; 229 del; 98 mod
8263412: ClassFileInstaller can't be used by classes outside of default package
Reviewed-by: iklam, coleenp, mseledtsov
-
PR: https://git.openjdk.java.net/jdk/pull/2932
Re: RFR: 8263412: ClassFileInstaller can't be used by classes outside of default package
On Fri, 12 Mar 2021 02:08:09 GMT, Mikhailo Seledtsov
wrote:
>> Hi all,
>>
>> could you please review the patch which moves `ClassFileInstaller` class to
>> `jdk.test.lib.helpers` package?
>> to reduce changes in the tests, `ClassFileInstaller` in the default package
>> is kept w/ just `main` method that calls `jdk.test.lib.helpers.
>> ClassFileInstaller::main`.
>>
>> from JBS:
>>> ClassFileInstaller is in the default package hence it's impossible to use
>>> it directly by classes in any other package. although ClassFileInstaller is
>>> mainly used directly from jtreg test description, there are cases when one
>>> needs to use it in another "driver" class (e.g. to reduce boilerplate), yet
>>> to do. that these driver classes have to either be in a default package or
>>> use reflection, both approaches have drawbacks.
>>>
>>> to solve that, we can move ClassFileInstaller implementation to a package,
>>> and to avoid unneeded churn, keep package-less ClassFileInstaller class
>>> which will call package-full impl.
>>>
>>> the need for this patch has raised as part of
>>> [JDK-8254129](https://bugs.openjdk.java.net/browse/JDK-8254129).
>>
>> testing:
>> - [x] `:jdk_core` against `{macos,windows,linux}-x64`
>> - [x] `:jdk_svc` against `{macos,windows,linux}-x64`
>> - [x] `test/hotspot/jtreg` w/o `applications` and `vmTestbase` directories
>> against `{macos,windows,linux}-x64`
>>
>> Thanks,
>> -- Igor
>
> Marked as reviewed by mseledtsov (Committer).
Ioi, Coleen, Misha, thanks for your reviews.
-- Igor
-
PR: https://git.openjdk.java.net/jdk/pull/2932
RFR: 8263412: ClassFileInstaller can't be used by classes outside of default package
Hi all,
could you please review the patch which moves `ClassFileInstaller` class to
`jdk.test.lib.helpers` package?
to reduce changes in the tests, `ClassFileInstaller` in the default package is
kept w/ just `main` method that calls `jdk.test.lib.helpers.
ClassFileInstaller::main`.
from JBS:
> ClassFileInstaller is in the default package hence it's impossible to use it
> directly by classes in any other package. although ClassFileInstaller is
> mainly used directly from jtreg test description, there are cases when one
> needs to use it in another "driver" class (e.g. to reduce boilerplate), yet
> to do. that these driver classes have to either be in a default package or
> use reflection, both approaches have drawbacks.
>
> to solve that, we can move ClassFileInstaller implementation to a package,
> and to avoid unneeded churn, keep package-less ClassFileInstaller class which
> will call package-full impl.
>
> the need for this patch has raised as part of
> [JDK-8254129](https://bugs.openjdk.java.net/browse/JDK-8263412).
testing:
- [x] `:jdk_core` against `{macos,windows,linux}-x64`
- [x] `:jdk_svc` against `{macos,windows,linux}-x64`
- [x] `test/hotspot/jtreg` w/o `applications` and `vmTestbase` directories
against `{macos,windows,linux}-x64`
Thanks,
-- Igor
-
Commit messages:
- make ClassFileInstaller.Manifest::from* public
- updated copyright years
- import j.t.l.helpers.ClassFileInstaller
- added jdk/test/lib/helpers/ClassFileInstaller
Changes: https://git.openjdk.java.net/jdk/pull/2932/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2932=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8263412
Stats: 473 lines in 114 files changed: 145 ins; 229 del; 99 mod
Patch: https://git.openjdk.java.net/jdk/pull/2932.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2932/head:pull/2932
PR: https://git.openjdk.java.net/jdk/pull/2932
Re: RFR: 8259267: Refactor LoaderLeak shell test as java test. [v15]
On Wed, 3 Mar 2021 19:56:07 GMT, Ivan Šipka wrote: >> Refactor `test/jdk/java/lang/annotation/loaderLeak/LoaderLeak.sh` as java >> test. > > 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 iignatyev (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1577
Re: RFR: 8259267: Refactor LoaderLeak shell test as java test. [v14]
On Wed, 3 Mar 2021 15:55:03 GMT, Ivan Šipka wrote:
>> Refactor `test/jdk/java/lang/annotation/loaderLeak/LoaderLeak.sh` as java
>> test.
>
> Ivan Šipka has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8166026: Refactor java/lang shell tests to java
test/jdk/java/lang/annotation/LoaderLeakTest.java line 74:
> 72: catch (NullPointerException npe) {
> 73: throw new AssertionError("c.get() should never return
> null", npe);
> 74: }
I don't think it's the right way to handle that, you don't know if this NPE is
from `c.get`, so the exception messages might be misleading. I'd just remove
try/catch, if NPE occurs jtreg will report the test as failed w/ NPE as a
reason, so whoever analyzes it will be able to understand.
alternatively, you can save c.get into a local variable which you nulls later
one, e.g.
```
static void doTest(boolean readAnn) throws Exception {
...
var tmp = c.get();
if (t == null) throw new AssertionError("c.get is null");
if (t.getClassLoader() != loader) throw new AssertionError("wrong classloader");
t = null;
-
PR: https://git.openjdk.java.net/jdk/pull/1577
Re: RFR: 8259267: Refactor LoaderLeak shell test as java test. [v8]
On Mon, 22 Feb 2021 15:24:19 GMT, Ivan Šipka wrote:
>> Refactor `test/jdk/java/lang/annotation/loaderLeak/LoaderLeak.sh` as java
>> test.
>
> 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 requested by iignatyev (Reviewer).
test/jdk/java/lang/annotation/LoaderLeakTest.java line 80:
> 78: System.gc();
> 79: System.gc();
> 80: if (c.get() == null) throw new AssertionError("class missing");
it would be better to use a different message here, so it would uniquely
identify a failed check, e.g. `class missing after GC but before loader is
unreachable`
test/jdk/java/lang/annotation/LoaderLeakTest.java line 74:
> 72: ClassLoader loader = new SimpleClassLoader();
> 73: var c = new WeakReference>(loader.loadClass("C"));
> 74: if (c.get() == null) throw new AssertionError();
please add an exception message here as well.
test/jdk/java/lang/annotation/LoaderLeakTest.java line 68:
> 66: public static void main(String[] args) throws Exception {
> 67: for (int i=0; i<100; i++)
> 68: doTest(args.length != 0);
nit: it's usually better to use `{`/`}` even for a one-line loop block.
test/jdk/java/lang/annotation/LoaderLeakTest.java line 99:
> 97: }
> 98:
> 99: @java.lang.annotation.Retention(RUNTIME)
nit: `import java.lang.annotation.Retention;`
test/jdk/java/lang/annotation/LoaderLeakTest.java line 59:
> 57: private void runJavaProcessExpectSuccessExitCode(String ... command)
> throws Throwable {
> 58: var processBuilder =
> ProcessTools.createJavaProcessBuilder(command)
> 59:
> .directory(Paths.get(Utils.TEST_CLASSES).toFile());
nit: in the case of chain calls, lines should be aligned by `.`
test/jdk/java/lang/annotation/LoaderLeakTest.java line 28:
> 26: * @bug 5040740
> 27: * @summary annotations cause memory leak
> 28: * @author gafter
not sure about etiquette in core-libs testsuite, but in hotspot testsuite, we
tend not to use `@author` tag.
-
PR: https://git.openjdk.java.net/jdk/pull/1577
Re: RFR: 8259267: Refactor LoaderLeak shell test as java test. [v8]
On Mon, 22 Feb 2021 15:33:30 GMT, Igor Ignatyev 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
>
> test/jdk/java/lang/annotation/LoaderLeakTest.java line 68:
>
>> 66: public static void main(String[] args) throws Exception {
>> 67: for (int i=0; i<100; i++)
>> 68: doTest(args.length != 0);
>
> nit: it's usually better to use `{`/`}` even for a one-line loop block.
nit 2: `=` and `<` should be surrounded by a space.
-
PR: https://git.openjdk.java.net/jdk/pull/1577
Re: RFR: 8259267: Refactor LoaderLeak shell test as java test. [v3]
On Fri, 11 Dec 2020 13:39:04 GMT, Igor Ignatyev 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
>
> test/jdk/java/lang/annotation/LoaderLeakTest.java line 64:
>
>> 62: @Test
>> 63: public void testWithoutReadingAnnotations() throws Throwable {
>> 64: runJavaProcessExpectSuccessExitCode("Main");
>
> indentation here looks wierd
indentation still looks weird here.
-
PR: https://git.openjdk.java.net/jdk/pull/1577
Re: RFR: 8259267: Refactor LoaderLeak shell test as java test. [v5]
On Mon, 8 Feb 2021 20:12:03 GMT, Ivan Šipka wrote:
>> Refactor `test/jdk/java/lang/annotation/loaderLeak/LoaderLeak.sh` as java
>> test.
>
> Ivan Šipka has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains four commits:
>
> - 8166026: Refactor java/lang shell tests to java
> - 8166026: Refactor java/lang shell tests to java
> - 8166026: Refactor java/lang shell tests to java
> - 8166026: Refactor java/lang shell tests to java
Changes requested by iignatyev (Reviewer).
test/jdk/java/lang/annotation/LoaderLeakTest.java line 2:
> 1: /*
> 2: * Copyright (c) 2004, 2020, Oracle and/or its affiliates. All rights
> reserved.
it's 2021
test/jdk/java/lang/annotation/LoaderLeakTest.java line 78:
> 76: // URL classes = new URL("file://" +
> System.getProperty("user.dir") + "/classes");
> 77: // URL[] path = { classes };
> 78: // URLClassLoader loader = new URLClassLoader(path);
please remove the commented out lines (L76-78)
test/jdk/java/lang/annotation/LoaderLeakTest.java line 128:
> 126:
> 127: @Override
> 128: public synchronized Class loadClass(String className, boolean
> resolveIt)
does it need to be `synchronized` ?
test/jdk/java/lang/annotation/LoaderLeakTest.java line 123:
> 121: return Files.readAllBytes(Paths.get(className + ".class"));
> 122: } catch (Exception e) {
> 123: throw new Error(e);
could you please use a descriptive message here (and include `className` value
into it), so it will be easier to analyze test failures?
test/jdk/java/lang/annotation/LoaderLeakTest.java line 119:
> 117:
> 118: private byte[] getClassImplFromDataBase(String className) {
> 119: byte result[];
unused
test/jdk/java/lang/annotation/LoaderLeakTest.java line 80:
> 78: // URLClassLoader loader = new URLClassLoader(path);
> 79: ClassLoader loader = new SimpleClassLoader();
> 80: WeakReference> c = new
> WeakReference>(loader.loadClass("C"));
nit: I'd use `var` here.
test/jdk/java/lang/annotation/LoaderLeakTest.java line 82:
> 80: WeakReference> c = new
> WeakReference>(loader.loadClass("C"));
> 81: if (c.get() == null) throw new AssertionError();
> 82: if (c.get().getClassLoader() != loader) throw new
> AssertionError();
please use the messages which explain what went wrong, so when the failure
happens, the person who analyze it won't have to open test code every time to
just understand which of assertions was hit.
test/jdk/java/lang/annotation/LoaderLeakTest.java line 106:
> 104: }
> 105:
> 106:
> @java.lang.annotation.Retention(java.lang.annotation.RetentionPolicy.RUNTIME)
nit: I'd import these types from j.l.a
-
PR: https://git.openjdk.java.net/jdk/pull/1577
Re: RFR: 8259268: Refactor InheritIO shell test as java test [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 you need to fix a missed comma in the copyright header to avoid tie1 failures. test/jdk/java/lang/ProcessBuilder/InheritIOTest.java line 2: > 1: /* > 2: * Copyright (c) 2020 Oracle and/or its affiliates. All rights reserved. missed comma - Changes requested by iignatyev (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1484
Re: RFR: 8259267: Refactor LoaderLeak shell test as java test. [v3]
On Tue, 5 Jan 2021 20:58:37 GMT, Ivan Šipka wrote:
>> test/jdk/java/lang/annotation/LoaderLeakTest.java line 54:
>>
>>> 52: List classes = List.of("A.class", "B.class", "C.class");
>>> 53: for (String fileName : classes) {
>>> 54: Files.move(
>>
>> I don't think it's a good idea to move files created and managed by `jtreg`.
>> I'd recommend you copying them here and, in `runJavaProcess...` constructing
>> `ProcessBuilder` youself:
>>
>> var args = new ArrayList(command.length + 1);
>> args.add(JDKToolFinder.getJDKTool("java"));
>> Collections.addAll(args, command);
>> var pb = new
>> ProcessBuilder(args).directory(Paths.get(Utils.TEST_CLASSES).toFile());
>
> They are intentionally moved out of class path so that the application class
> loader can not find them. If they are not moved they will be loaded by the
> application class loader and they need to be
> [loaded](https://github.com/openjdk/jdk/blob/bc56a63702b8730abc1d0aebee133a5884145fa1/test/jdk/java/lang/annotation/LoaderLeakTest.java#L96)
> by the tests very own `SimpleClassLoader` in order to be able to test for
> the leakage.
I understand the intention, but it can be achieved w/o messing around w/ jtreg
managed directories, for example by changing `SimpleClassLoader` to not
delegate the loading of `A`, `B`, `C` classes to the application CL.
-
PR: https://git.openjdk.java.net/jdk/pull/1577
Re: RFR: 8166026: Refactor java/lang shell tests to java [v3]
On Wed, 9 Dec 2020 15:19:58 GMT, Ivan Šipka wrote:
>> Refactor `test/jdk/java/lang/annotation/loaderLeak/LoaderLeak.sh` as java
>> test.
>
> 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 requested by iignatyev (Reviewer).
test/jdk/java/lang/annotation/LoaderLeakTest.java line 79:
> 77:
> .directory(Paths.get(Utils.TEST_CLASSES).toFile()
> 78: )
> 79: ).shouldHaveExitValue(0);
indentation here looks wierd
test/jdk/java/lang/annotation/LoaderLeakTest.java line 54:
> 52: List classes = List.of("A.class", "B.class", "C.class");
> 53: for (String fileName : classes) {
> 54: Files.move(
I don't think it's a good idea to move files created and managed by `jtreg`.
I'd recommend you copying them here and, in `runJavaProcess...` constructing
`ProcessBuilder` youself:
var args = new ArrayList(command.length + 1);
args.add(JDKToolFinder.getJDKTool("java"));
Collections.addAll(args, command);
var pb = new
ProcessBuilder(args).directory(Paths.get(Utils.TEST_CLASSES).toFile());
-
PR: https://git.openjdk.java.net/jdk/pull/1577
Re: RFR: 8257516: define test group for manual tests [v3]
On Thu, 3 Dec 2020 22:54:13 GMT, Ivan Šipka wrote: >> Defined new test groups as defined in ticket. @fguallini > > Ivan Šipka has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. The pull request contains one new > commit since the last revision: > > 8257516: define test group for manual tests LGTM - Marked as reviewed by iignatyev (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1416
Re: RFR: 8256894: define test groups [v2]
On Mon, 30 Nov 2020 21:10:13 GMT, Igor Ignatyev wrote: >> Ivan Šipka has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8257516: removing trailing space > > @frkator, you will need to open a new JBS ticket for this change. > @iignatev as for: > "[#1416 > (comment)](https://github.com/openjdk/jdk/pull/1416#discussion_r532904345)" > accidentaly resolved. Yes they are but the goal is to list all manual tests > under one label for counting purposes. I understand that; my comment was rather to @AlanBateman 's request not to add manual tests into `:jdk_code`. they already are, and removing explicit inclusion of `:jdk_core_manual` to `:jdk_core` won't change that. -- Igor - PR: https://git.openjdk.java.net/jdk/pull/1416
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: JDK-8249836 java/io/IOException/LastErrorString.java should have bug-id as 1st word in @ignore
On Fri, 27 Nov 2020 16:11:54 GMT, Mahendra Chhipa wrote: > …id as 1st word in @ignore > > https://bugs.openjdk.java.net/browse/JDK-8249836 LGTM - Marked as reviewed by iignatyev (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1482
Re: RFR: 8256894: define test groups
On Tue, 24 Nov 2020 16:13:59 GMT, Ivan Šipka wrote: > Defined new test groups as defined in ticket. @fguallini @frkator, you will need to open a new JBS ticket for this change. test/jdk/TEST.groups line 326: > 324: :jdk_text \ > 325: :core_tools \ > 326: :jdk_other it would seem you have introduced a trailing space - Changes requested by iignatyev (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1416
Re: RFR: 8256894: define test groups
On Wed, 25 Nov 2020 20:31:50 GMT, Ivan Šipka wrote: >> test/jdk/TEST.groups line 327: >> >>> 325: :core_tools \ >>> 326: :jdk_other \ >>> 327: :jdk_core_manual >> >> Please don't add manual tests to jdk_core. > > Removed. aren't they already a part of `jdk_core` test group? e.g. `java/net/HugeDataTransferTest.java` is in `:jdk_net` (as `jdk_net` includes `java/net` directory), and `:jdk_core` includes `:jdk_net`? - PR: https://git.openjdk.java.net/jdk/pull/1416
Re: RFR: 8256244: java/lang/ProcessHandle/PermissionTest.java fails with TestNG 7.1
On Thu, 12 Nov 2020 15:48:33 GMT, Roger Riggs wrote: > TestNG 7.1 changed/corrected the way that @BeforeGroups are selected at > runtime. > The test was depending on @BeforeGroups to initialize common security policy > for a number of tests. > The tests are modified to individually setup the needed security manager and > policy. Marked as reviewed by iignatyev (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1190
Re: RFR: 8255964: Add all details to jstack log in jtreg timeout handler [v2]
On Fri, 6 Nov 2020 20:25:13 GMT, Nils Eliasson wrote: >> This patch adds jcmd Thread.print to the jtreg timeout handler. >> >> Please review. > > Nils Eliasson has updated the pull request incrementally with one additional > commit since the last revision: > > add extended printing to jstack Marked as reviewed by iignatyev (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1080
Re: RFR: 8255964: Add jcmd Thread.print to jtreg timeout handler
On Thu, 5 Nov 2020 17:09:58 GMT, Nils Eliasson wrote: > This patch adds jcmd Thread.print to the jtreg timeout handler. > > Please review. Hi Nils, It looks alright, but could you please elaborate on why we need it when there is already `jstack` action? — Igor - PR: https://git.openjdk.java.net/jdk/pull/1080
Re: RFR: 8238263: Create at-requires mechanism for containers [v2]
On Mon, 26 Oct 2020 18:13:29 GMT, Harold Seigel wrote: >> Please review this change to add an @requires mechanism called >> "jdk.containerized" to help mark tests that are incompatible with >> containers. Users would add "@requires jdk.containerized != true" to the >> incompatible tests and then use "make test ... >> OPTIONS=-Djdk.containerized=true" or "bash jib.sh mach5 -- >> remote-build-and-test ... --test-make-args >> JTREG=OPTIONS=-Djdk.containerized=true" to exclude those tests when testing >> with containers. > > Harold Seigel has updated the pull request incrementally with one additional > commit since the last revision: > > 8238263: Create at-requires mechanism for containers LGTM, modulo my earlier comment/doubt about the name, but I don’t think it’s important enough - Marked as reviewed by iignatyev (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/844
Re: RFR: 8238263: Create at-requires mechanism for containers
On Mon, 26 Oct 2020 13:49:26 GMT, Harold Seigel wrote: > Defining an environment variable works when running JTReg from the command > line. But, mach5 does not pass environment variable settings to its JTReg > test runs. Some mach5 special command args would still be needed. right, yet given you also need to explicitly say mach5 that you want to run testing within docker, that's not a huge problem. this is assuming we default env. variable to `false`, `make` propagates this env. variable to `jtreg` and `jtreg` propagates it to the JVM which runs `VMProps` class. - PR: https://git.openjdk.java.net/jdk/pull/844
Re: RFR: 8238263: Create at-requires mechanism for containers
On Fri, 23 Oct 2020 19:54:40 GMT, Igor Ignatyev wrote: >> Hi Igor, >> I think it depends on whether the tests will be permanently or temporarily >> excluded from running with containers. I thought this mechanism would be to >> permanently exclude the tests. That's why I used @requires. >> Thanks, Harold > >> I think it depends on whether the tests will be permanently or temporarily >> excluded from running with containers. I thought this mechanism would be to >> permanently exclude the tests. That's why I used `@requires`. > > I see, if this is for permanent exclusion then yes I agree that `@requires` > is a better choice. > >>> enable this option based on an environment variable so we don?t have to >>> remember the > cryptic command line sequence. >> I'll look into basing the option on an environment variable. > > one will still need to pass an environment variable to jtreg, and hence will > need to remember some sort of "cryptic command line sequence". a solution for > that might be to default `jdk.containerized` to `false` in `VMProps.java` and > when only _containerized_ runs will have to set it up. > > > btw, I'm not sure that `jdk.containerized` is the best name for this property > as _containerization_ is more of an environmental characteristic than that of > jdk. how about smth like `env.containerized` or `testenv.containerized`? > > one will still need to pass an environment variable to jtreg, and hence > > will need to remember some sort of "cryptic command line sequence". a > > solution for that might be to default `jdk.containerized` to `false` in > > `VMProps.java` and when only _containerized_ runs will have to set it up. > > Why? Environment variables are inherited. For developers running jtreg, all > they need to do is export the variable. > > % export JAVA_CONTAINERIZED=1 > % bash > % echo $JAVA_CONTAINERIZED > % 1 b/c jtreg strips most of the environment variables, they might still be defined in the process which runs `VMProps` though (I have never checked that) - PR: https://git.openjdk.java.net/jdk/pull/844
Re: RFR: 8238263: Create at-requires mechanism for containers
On Fri, 23 Oct 2020 19:25:27 GMT, Harold Seigel wrote: > I think it depends on whether the tests will be permanently or temporarily > excluded from running with containers. I thought this mechanism would be to > permanently exclude the tests. That's why I used `@requires`. I see, if this is for permanent exclusion then yes I agree that `@requires` is a better choice. >> enable this option based on an environment variable so we don?t have to >> remember the cryptic command line sequence. > I'll look into basing the option on an environment variable. one will still need to pass an environment variable to jtreg, and hence will need to remember some sort of "cryptic command line sequence". a solution for that might be to default `jdk.containerized` to `false` in `VMProps.java` and when only _containerized_ runs will have to set it up. btw, I'm not sure that `jdk.containerized` is the best name for this property as _containerization_ is more of an environmental characteristic than that of jdk. how about smth like `env.containerized` or `testenv.containerized`? - PR: https://git.openjdk.java.net/jdk/pull/844
Re: RFR: 8238263: Create at-requires mechanism for containers
On Fri, 23 Oct 2020 18:44:54 GMT, Harold Seigel wrote: > Please review this change to add an @requires mechanism called > "jdk.containerized" to help mark tests that are incompatible with containers. > Users would add "@requires jdk.containerized != true" to the incompatible > tests and then use "make test ... OPTIONS=-Djdk.containerized=true" or "bash > jib.sh mach5 -- remote-build-and-test ... --test-make-args > JTREG=OPTIONS=-Djdk.containerized=true" to exclude those tests when testing > with containers. Hi Harold, I actually still think that having a separate ProblemList (as we do for graal, zgc, Xcomp, aot) is a better solution. why did you choose `@requires` over it? -- Igor - PR: https://git.openjdk.java.net/jdk/pull/844
Re: RFR: 8253667: ProblemList tools/jlink/JLinkReproducible{, 3}Test.java on linux-aarch64 [v2]
On Mon, 28 Sep 2020 16:32:52 GMT, Daniel D. Daugherty
wrote:
>> 8253667: ProblemList tools/jlink/JLinkReproducible{,3}Test.java on
>> linux-aarch64
>
> Daniel D. Daugherty has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Update existing entry for tools/jlink/JLinkReproducibleTest.java instead of
> creating a new one.
Marked as reviewed by iignatyev (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/382
Re: RFR: 8253667: ProblemList tools/jlink/JLinkReproducible{, 3}Test.java on linux-aarch64
On Mon, 28 Sep 2020 16:16:52 GMT, Igor Ignatyev wrote:
>> 8253667: ProblemList tools/jlink/JLinkReproducible{,3}Test.java on
>> linux-aarch64
>
> test/jdk/ProblemList.txt line 859:
>
>> 857: tools/jlink/JLinkReproducibleTest.java 8217166
>> windows-all
>> 858: tools/jlink/plugins/CompressorPluginTest.java 8247407
>> generic-all
>> 859: tools/jlink/JLinkReproducibleTest.java 8217166
>> linux-aarch64
>
> wouldn't it be better to "join" JLinkReproducibleTest entries?
> ```suggestion
> tools/jlink/JLinkReproducibleTest.java 8217166
> windows-all,linux-aarch64
> tools/jlink/plugins/CompressorPluginTest.java 8247407
> generic-all
actually, you have to join them, as only jtreg honors only the last entry for a
test --
[CODETOOLS-7902481](https://bugs.openjdk.java.net/browse/CODETOOLS-7902481)
-
PR: https://git.openjdk.java.net/jdk/pull/382
Re: RFR: 8253667: ProblemList tools/jlink/JLinkReproducible{, 3}Test.java on linux-aarch64
On Mon, 28 Sep 2020 16:05:48 GMT, Daniel D. Daugherty
wrote:
> 8253667: ProblemList tools/jlink/JLinkReproducible{,3}Test.java on
> linux-aarch64
Marked as reviewed by iignatyev (Reviewer).
test/jdk/ProblemList.txt line 859:
> 857: tools/jlink/JLinkReproducibleTest.java 8217166
> windows-all
> 858: tools/jlink/plugins/CompressorPluginTest.java 8247407
> generic-all
> 859: tools/jlink/JLinkReproducibleTest.java 8217166
> linux-aarch64
wouldn't it be better to "join" JLinkReproducibleTest entries?
```suggestion
tools/jlink/JLinkReproducibleTest.java 8217166
windows-all,linux-aarch64
tools/jlink/plugins/CompressorPluginTest.java 8247407
generic-all
-
PR: https://git.openjdk.java.net/jdk/pull/382
Re: [15] RFR(S) : 6501010 : test/java/io/File/GetXSpace.java fails on Windows
Hi Arno,
We haven't seen such failures in our infra, I guess we just need to add ^ to
the beginning of the pattern. please file a bug and send a proper RFR for it
and will be happy to run the patch in our testing infra for you.
Cheers,
-- Igor
> On Aug 11, 2020, at 6:39 AM, Zeller, Arno wrote:
>
> Hi Igor,
>
> after our push I see test/java/io/File/GetXSpace.java failing on our Windows
> test machines. The issue seems to be that the 'df' call produces several
> lines of output on my windows machine and the pattern uses \n as line-ending.
>
> In a short test I changed the regex to match $ instead of \n in the end and
> added the MULTILINE flag.
>
> --
> diff -r 16625b2b71f7 test/jdk/java/io/File/GetXSpace.java
> --- a/test/jdk/java/io/File/GetXSpace.java Tue Aug 11 07:29:45 2020 -0400
> +++ b/test/jdk/java/io/File/GetXSpace.java Tue Aug 11 15:35:15 2020 +0200
> @@ -55,7 +55,7 @@
> private static final boolean IS_WIN = OS_NAME.startsWith("Windows");
>
> // FileSystem Total Used Available Use% MountedOn
> -private static final Pattern DF_PATTERN =
> Pattern.compile("([^\\s]+)\\s+(\\d+)\\s+\\d+\\s+(\\d+)\\s+\\d+%\\s+([^\\s].*)\n");
> +private static final Pattern DF_PATTERN =
> Pattern.compile("([^\\s]+)\\s+(\\d+)\\s+\\d+\\s+(\\d+)\\s+\\d+%\\s+([^\\s].*)$",
> Pattern.MULTILINE);
>
> private static int fail = 0;
> private static int pass = 0;
> --
>
> Now the test passes for me and in our infrastructure. I did not completely
> understand why the old pattern did loose the first character in the line -
> but this seems to fix the issue.
>
> Before my change:
> --System.out:(24/869)--
> --- Testing df
> C:/cygwin64 998257472 664791328 333466144 67% /
> K: 262144000 129993392 132150608 50% /cygdrive/k
> O: 734003200 641218112 92785088 88% /cygdrive/o
>
>
> SecurityManager = null
> C:/cygwin64:
> df total= 1022215651328 free =0 usable = 341469331456
> getX total= 1022215651328 free = 341469323264 usable = 341469323264
> ::
> df total= 268435456000 free =0 usable = 13532592
> getX total=0 free =0 usable =0
> FAILED
>
>
> After my change:
> --- Testing df
> C:/cygwin64 998257472 665305100 332952372 67% /
> K: 262144000 129993392 132150608 50% /cygdrive/k
> O: 734003200 641211140 92792060 88% /cygdrive/o
>
>
> SecurityManager = null
> C:/cygwin64:
> df total= 1022215651328 free =0 usable = 340943228928
> getX total= 1022215651328 free = 340943286272 usable = 340943286272
> K::
> df total= 268435456000 free =0 usable = 13532592
> getX total= 268435456000 free = 13532592 usable = 13532592
> O::
> df total= 751619276800 free =0 usable = 95019069440
> getX total= 751619276800 free = 95019069440 usable = 95019069440
> ...
> --
>
>
> The drives K: and O: are mapped network drives.
> Do you see something similar in your landscape? Can you perhaps try to
> reproduce this?
>
> Best regards,
> Arno
>
>> -Original Message-
>> From: core-libs-dev On Behalf Of Igor
>> Ignatyev
>> Sent: Freitag, 31. Juli 2020 04:45
>> To: Brian Burkhalter
>> Cc: core-libs-dev
>> Subject: Re: [15] RFR(S) : 6501010 : test/java/io/File/GetXSpace.java fails
>> on
>> Windows
>>
>> Hi Brian,
>>
>> thanks for your review. I've decided to push into jdk/jdk instead of jdk15.
>> please let me know if you think this patch needs to be in jdk15, and I'll
>> backport
>> it there.
>>
>> Cheers,
>> -- Igor
>>
>>> On Jul 30, 2020, at 2:39 PM, Brian Burkhalter
>> wrote:
>>>
>>> Hi Igor,
>>>
>>> This test looks all right to me.
>>>
>>> Sorry for the late review.
>>>
>>> Thanks,
>>>
>>> Brian
>>>
>>>> On Jul 20, 2020, at 11:28 AM, Igor Ignatyev > <mailto:igor.ignat...@oracle.com>> wrote:
>>>>
>>>> http://cr.openjdk.java.net/~iignatyev//6501010/webrev.00
>> <http://cr.openjdk.java.net/~iignatyev//6501010/webrev.00>
>>>>> 98 lines changed: 18 ins; 31 del; 49 mod;
>>>>
>>>> Hi all,
>>>>
>>>> could you please review this small fix to make java/io/File/GetXSpace.java
>> work again on windows?
>>>>
>>>> the test has been updated to work under cygwin, which includes the
>> following changes:
>>>> - GetXSpace.sh accepts CYGWIN_* as known OS and sets windows path to
>> TMP var
>>>> - uses the same regexp to parse df on windows as on other platforms
>>>> - but uses 'FileSystem' field as Space's name, as opposed to 'MountedOn' on
>> *nix, as on cygwin, MountedOn is cygwin path, while FileSystem is
>> corresponding windows path, and the test expect Space's name to be
>> appropriate path
>>>
>
Re: [15] RFR(S) : 6501010 : test/java/io/File/GetXSpace.java fails on Windows
Hi Brian, thanks for your review. I've decided to push into jdk/jdk instead of jdk15. please let me know if you think this patch needs to be in jdk15, and I'll backport it there. Cheers, -- Igor > On Jul 30, 2020, at 2:39 PM, Brian Burkhalter > wrote: > > Hi Igor, > > This test looks all right to me. > > Sorry for the late review. > > Thanks, > > Brian > >> On Jul 20, 2020, at 11:28 AM, Igor Ignatyev > <mailto:igor.ignat...@oracle.com>> wrote: >> >> http://cr.openjdk.java.net/~iignatyev//6501010/webrev.00 >> <http://cr.openjdk.java.net/~iignatyev//6501010/webrev.00> >>> 98 lines changed: 18 ins; 31 del; 49 mod; >> >> Hi all, >> >> could you please review this small fix to make java/io/File/GetXSpace.java >> work again on windows? >> >> the test has been updated to work under cygwin, which includes the following >> changes: >> - GetXSpace.sh accepts CYGWIN_* as known OS and sets windows path to TMP var >> - uses the same regexp to parse df on windows as on other platforms >> - but uses 'FileSystem' field as Space's name, as opposed to 'MountedOn' on >> *nix, as on cygwin, MountedOn is cygwin path, while FileSystem is >> corresponding windows path, and the test expect Space's name to be >> appropriate path >
Re: [15] RFR(S) : 6501010 : test/java/io/File/GetXSpace.java fails on Windows
ping?
-- Igor
> On Jul 20, 2020, at 11:28 AM, Igor Ignatyev wrote:
>
> http://cr.openjdk.java.net/~iignatyev//6501010/webrev.00
>> 98 lines changed: 18 ins; 31 del; 49 mod;
>
> Hi all,
>
> could you please review this small fix to make java/io/File/GetXSpace.java
> work again on windows?
>
> the test has been updated to work under cygwin, which includes the following
> changes:
> - GetXSpace.sh accepts CYGWIN_* as known OS and sets windows path to TMP var
> - uses the same regexp to parse df on windows as on other platforms
> - but uses 'FileSystem' field as Space's name, as opposed to 'MountedOn' on
> *nix, as on cygwin, MountedOn is cygwin path, while FileSystem is
> corresponding windows path, and the test expect Space's name to be
> appropriate path
>
> the patch also includes some small faceliftings:
> - printing out a bit more information to facilitate failure analyses, e.g. df
> output, exception stack traces
> - usage of try-w/-resources
> - remove of code duplication
> - usage of generics
>
> webrev: http://cr.openjdk.java.net/~iignatyev//6501010/webrev.00
> testing: java/io/File/GetXSpace.java 100 times on {linux,windows}-x64
> JBS: https://bugs.openjdk.java.net/browse/JDK-6501010
>
> Thanks,
> -- Igor
Re: RFR (trivial) 8249940: Remove unnecessary includes of jni_util.h in native tests
Hi David, looks good to me. -- Igor > On Jul 22, 2020, at 4:00 PM, David Holmes wrote: > > Bug: https://bugs.openjdk.java.net/browse/JDK-8249940 > webrev: http://cr.openjdk.java.net/~dholmes/8249940/webrev/ > > A number of native tests in hotspot and jdk include the jni_util.h header > file which is part of the sources for libjava and not part of the testing > framework, nor an exported interface for the JDK. This seems to have occurred > through copy-and-paste when creating the tests as the include is not needed. > > test/hotspot/jtreg/runtime/jni/FindClass/libbootLoaderTest.c > test/hotspot/jtreg/runtime/jni/registerNativesWarning/libregisterNativesWarning.c > test/hotspot/jtreg/runtime/jni/terminatedThread/libterminatedThread.c > test/jdk/java/lang/ClassLoader/nativeLibrary/libnativeLibraryTest.c > test/jdk/java/lang/ProcessBuilder/checkHandles/libCheckHandles.c > test/jdk/jdk/internal/loader/NativeLibraries/libnativeLibrariesTest.c > > There is one test that includes jni_util.h and uses the utility function > declared there: > ./jdk/java/lang/String/nativeEncoding/libstringPlatformChars.c > so that is left as-is. > > Thanks, > David > -
Re: [15] RFR(T) : 8249697 : java/lang/invoke/RicochetTest.java should use @requires instead of @ignore
Hi Mandy,
you are right, it's better to have just one @run, and as I don't think that
7197210 changes '-XX:-VerifyDependencies' nor '/timeout=3600' are needed
anymore, I suggest to restore the test to its original version w/ `@run
junit/othervm -DRicochetTest.MAX_ARITY=255 test.java.lang.invoke.RicochetTest`,
so the patch (http://cr.openjdk.java.net/~iignatyev//8249697/webrev.03) would
be just:
> -/* @test
> +/*
> + * @test
> * @summary unit tests for recursive method handles
> - * @run junit/othervm/timeout=3600 -XX:+IgnoreUnrecognizedVMOptions
> -XX:-VerifyDependencies -DRicochetTest.MAX_ARITY=10
> test.java.lang.invoke.RicochetTest
> - */
> -/*
> - * @ignore The following test creates an unreasonable number of adapters in
> -Xcomp mode (7049122)
> * @run junit/othervm -DRicochetTest.MAX_ARITY=255
> test.java.lang.invoke.RicochetTest
> */
and then the bug's summary would be smth like 'remove temporary fixes from
java/lang/invoke/RicochetTest.java' .
sure there is no reason for it to be pushed into 15, I've retargeted to 16.
-- Igor
> On Jul 20, 2020, at 11:57 AM, Mandy Chung wrote:
>
> Hi Igor,
>
> OK. Should this revert the change by 7049122 then? i.e. simply change
> -DRicochetTest.MAX_ARITY=10 to 255
>
> Your proposed patch adds a new @run instead of modifying the existing @run
> command:
>
> * @run junit/othervm/timeout=3600 -XX:+IgnoreUnrecognizedVMOptions
> -XX:-VerifyDependencies -DRicochetTest.MAX_ARITY=10
> test.java.lang.invoke.RicochetTest
>
> I looked at the history and this @run was modified by JDK-7197210 that adds
> -XX:+IgnoreUnrecognizedVMOptions -XX:-VerifyDependencies options and reduce
> MAX_ARITY from 50 to 10.
>
> This issue is not critical to target for 15. It may worth considering target
> this test fix for 16. Just a suggestion.
>
> Mandy
>
> On 7/20/20 10:13 AM, Igor Ignatyev wrote:
>> Hi Mandy,
>>
>> that's actually the opposite, the 2nd subtest is run only in modes other
>> than Xcomp, as w/ Xcomp the test creates lots of adapters and used to lead
>> to JVM failure as described in 7049122. I tried to reproduce this failure,
>> but in vain,.. after a bit more historical digging, I realized that the
>> underlying problem was 7009641, which has been fixed in hs25/jdk8. so I've
>> changed the fix for 8249697 to simply return run w/
>> '-DRicochetTest.MAX_ARITY=255':
>> http://cr.openjdk.java.net/~iignatyev//8249697/webrev.02
>> <http://cr.openjdk.java.net/~iignatyev//8249697/webrev.02>
>>
>> I've verified that the test passes w/ Xcomp and
>> - -XX:+TieredCompilation (c1 + c2);
>> - -XX:-TieredCompilation (c2-only);
>> - -XX:+NeverActAsServerClassMachine (emulated-client, c1-only)
>>
>> the test was run 100 times on {linux,windows,macos}-x64 w/ 0 failures.
>>
>> Thanks,
>> -- Igor
>>
>>> On Jul 18, 2020, at 9:32 PM, Mandy Chung >> <mailto:mandy.ch...@oracle.com>> wrote:
>>>
>>>
>>>
>>> On 7/17/20 8:54 PM, Igor Ignatyev wrote:
>>>> http://cr.openjdk.java.net/~iignatyev/8249697/webrev.00/
>>>> <http://cr.openjdk.java.net/~iignatyev/8249697/webrev.00/>
>>>>
>>>
>>> I suggest to change this:
>>> 32 * @comment The following test creates an unreasonable number of
>>> adapters in -Xcomp mode (7049122)
>>>
>>> To:
>>>
>>>@bug 8249697
>>>@summary verify very high number of adapters in -Xcomp mode
>>>
>>> Otherwise, looks fine.
>>>
>>> Mandy
>>>> Hi all,
>>>>
>>>> could you please review this small and trivial patch for
>>>> java/lang/invoke/RicochetTest.java test?
>>>> from JBS:
>>>>> a run of java/lang/invoke/RicochetTest.java w/ MAX_ARITY=255 was removed
>>>>> from all configurations by JDK-7049122, yet the problem manifests itself
>>>>> only w/ Xcomp. as now we have @requires to filter out tests from certain
>>>>> configurations, the test can be updated to run MAX_ARITY=255 in all
>>>>> configs but Xcomp.
>>>> the patch splits the test into two subtests, each one w/ one @run, and use
>>>> @requires to exclude one w/ MAX_ARITY=255 from execution if Xcomp flag is
>>>> used.
>>>>
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8249697
>>>> <https://bugs.openjdk.java.net/browse/JDK-8249697>
>>>> webrev: http://cr.openjdk.java.net/~iignatyev/8249697/webrev.00/
>>>> <http://cr.openjdk.java.net/~iignatyev/8249697/webrev.00/>
>>>> testing: java/lang/invoke/RicochetTest.java on {linux,windows,macos}-x64
>>>> w/ and w/o -Xcomp; Xcomp runs, as expected, had only 1 test run
>>>>
>>>> Thanks,
>>>> -- Igor
>>>>
>>>> JDK-7049122 : https://bugs.openjdk.java.net/browse/JDK-7049122
>>>> <https://bugs.openjdk.java.net/browse/JDK-7049122>
>>
>
[15] RFR(S) : 6501010 : test/java/io/File/GetXSpace.java fails on Windows
http://cr.openjdk.java.net/~iignatyev//6501010/webrev.00
> 98 lines changed: 18 ins; 31 del; 49 mod;
Hi all,
could you please review this small fix to make java/io/File/GetXSpace.java work
again on windows?
the test has been updated to work under cygwin, which includes the following
changes:
- GetXSpace.sh accepts CYGWIN_* as known OS and sets windows path to TMP var
- uses the same regexp to parse df on windows as on other platforms
- but uses 'FileSystem' field as Space's name, as opposed to 'MountedOn' on
*nix, as on cygwin, MountedOn is cygwin path, while FileSystem is corresponding
windows path, and the test expect Space's name to be appropriate path
the patch also includes some small faceliftings:
- printing out a bit more information to facilitate failure analyses, e.g. df
output, exception stack traces
- usage of try-w/-resources
- remove of code duplication
- usage of generics
webrev: http://cr.openjdk.java.net/~iignatyev//6501010/webrev.00
testing: java/io/File/GetXSpace.java 100 times on {linux,windows}-x64
JBS: https://bugs.openjdk.java.net/browse/JDK-6501010
Thanks,
-- Igor
Re: [15] RFR(T) : 8249697 : java/lang/invoke/RicochetTest.java should use @requires instead of @ignore
Hi Mandy,
that's actually the opposite, the 2nd subtest is run only in modes other than
Xcomp, as w/ Xcomp the test creates lots of adapters and used to lead to JVM
failure as described in 7049122. I tried to reproduce this failure, but in
vain,.. after a bit more historical digging, I realized that the underlying
problem was 7009641, which has been fixed in hs25/jdk8. so I've changed the fix
for 8249697 to simply return run w/ '-DRicochetTest.MAX_ARITY=255':
http://cr.openjdk.java.net/~iignatyev//8249697/webrev.02
I've verified that the test passes w/ Xcomp and
- -XX:+TieredCompilation (c1 + c2);
- -XX:-TieredCompilation (c2-only);
- -XX:+NeverActAsServerClassMachine (emulated-client, c1-only)
the test was run 100 times on {linux,windows,macos}-x64 w/ 0 failures.
Thanks,
-- Igor
> On Jul 18, 2020, at 9:32 PM, Mandy Chung wrote:
>
>
>
> On 7/17/20 8:54 PM, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev/8249697/webrev.00/
>> <http://cr.openjdk.java.net/~iignatyev/8249697/webrev.00/>
>>
>
> I suggest to change this:
> 32 * @comment The following test creates an unreasonable number of
> adapters in -Xcomp mode (7049122)
>
> To:
>
>@bug 8249697
>@summary verify very high number of adapters in -Xcomp mode
>
> Otherwise, looks fine.
>
> Mandy
>> Hi all,
>>
>> could you please review this small and trivial patch for
>> java/lang/invoke/RicochetTest.java test?
>> from JBS:
>>> a run of java/lang/invoke/RicochetTest.java w/ MAX_ARITY=255 was removed
>>> from all configurations by JDK-7049122, yet the problem manifests itself
>>> only w/ Xcomp. as now we have @requires to filter out tests from certain
>>> configurations, the test can be updated to run MAX_ARITY=255 in all configs
>>> but Xcomp.
>> the patch splits the test into two subtests, each one w/ one @run, and use
>> @requires to exclude one w/ MAX_ARITY=255 from execution if Xcomp flag is
>> used.
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8249697
>> <https://bugs.openjdk.java.net/browse/JDK-8249697>
>> webrev: http://cr.openjdk.java.net/~iignatyev/8249697/webrev.00/
>> <http://cr.openjdk.java.net/~iignatyev/8249697/webrev.00/>
>> testing: java/lang/invoke/RicochetTest.java on {linux,windows,macos}-x64 w/
>> and w/o -Xcomp; Xcomp runs, as expected, had only 1 test run
>>
>> Thanks,
>> -- Igor
>>
>> JDK-7049122 : https://bugs.openjdk.java.net/browse/JDK-7049122
>> <https://bugs.openjdk.java.net/browse/JDK-7049122>
Re: [15] RFR(S) : 8249700 : java/io/File/GetXSpace.java should be added to exclude list, and not @ignore-d
Hi Alan, thanks for the review, pushed to jdk15. -- Igor > On Jul 19, 2020, at 8:18 AM, Alan Bateman wrote: > > On 19/07/2020 05:28, Igor Ignatyev wrote: >> : >> the patch also includes minimal changes to make the test runnable on macosx, >> which reveled that the test might fail on macos. 8249703 has been filed for >> that issue and the appropriate changes are done in ProblemList.txt. >> >> webrev: http://cr.openjdk.java.net/~iignatyev//8249700/webrev.01/ >> JBS: https://bugs.openjdk.java.net/browse/JDK-8249700 >> testing: java/io/File/GetXSpace.java on linux-x64 100 times, 100% passed; >> macos-x64 100 times, 71% passed >> >> PS the test is still failing on windows albeit due to the reason other than >> in 6501010; it seems that the test should be just updated to recognize >> cygwin's `df` output. I'm currently working on the fix and will send it out >> for review shortly. >> > This looks okay to me. > > -Alan
Re: [15] RFR(T) : 8249698 : java/lang/invoke/LFCaching/LFGarbageCollectedTest.java should be ProblemList-ed and not @ignored
Mandy, Vladimir, thanks for your reviews, pushed to jdk15. -- Igor > On Jul 18, 2020, at 9:33 PM, Mandy Chung wrote: > > +1 > > Mandy > > On 7/17/20 8:57 PM, Igor Ignatyev wrote: >> http://cr.openjdk.java.net/~iignatyev//8249698/webrev.00 >> <http://cr.openjdk.java.net/~iignatyev//8249698/webrev.00> >>> 3 lines changed: 1 ins; 1 del; 1 mod; >> >> Hi all, >> >> could you please review this trivial patch which removes @ignore from >> LFGarbageCollectedTest and adds it into problem-list instead? >> >> from 8249698: >>> java/lang/invoke/LFCaching/LFGarbageCollectedTest.java is excluded from >>> execution due to JDK-8078602. although the test might indeed fail due to >>> JDK-8078602, it still can be useful and isn't harmful to run, therefore >>> this test should be put in ProblemList.txt and @ignore is to be removed. >> from main issue(8249618): >>> although ProblemList and @ignore achieve the same end result (test >>> exclusion), their server different goals and have slightly different >>> meanings, simplified @ignore should be used to exclude useless or harmful >>> tests, and ProblemList in all other cases (see yet-not-integrated >>> `ProblemListing or `@ignore`-ing a Test` section of dev guide, PR -- >>> https://github.com/openjdk/guide/pull/21 >>> <https://github.com/openjdk/guide/pull/21> for more details). >>> >>> due to different reasons, this hasn't been always followed and some >>> currently @ignore-d tests should rather be ProblemList-ed, and some of >>> ProblemList-ed should be @ignore-d, this issue is to clean up the current >>> state in a hope that this will reduce further confusion. >> >> webrev: http://cr.openjdk.java.net/~iignatyev//8249698/webrev.00 >> <http://cr.openjdk.java.net/~iignatyev//8249698/webrev.00> >> JBS: https://bugs.openjdk.java.net/browse/JDK-8249698 >> <https://bugs.openjdk.java.net/browse/JDK-8249698> >> >> Thanks, >> -- Igor >> >> 8078602: https://bugs.openjdk.java.net/browse/JDK-8078602 >> <https://bugs.openjdk.java.net/browse/JDK-8078602> >> 8249618: https://bugs.openjdk.java.net/browse/JDK-8249618 >> <https://bugs.openjdk.java.net/browse/JDK-8249618>
[15] RFR(S) : 8249700 : java/io/File/GetXSpace.java should be added to exclude list, and not @ignore-d
http://cr.openjdk.java.net/~iignatyev//8249700/webrev.01/ > 9 lines changed: 1 ins; 1 del; 7 mod Hi all, could you please review this small patch which removes @ignore from java/io/File/GetXSpace.java and instead adds an entry to ProblemList.txt? from JBS: > java/io/File/GetXSpace.java is currently @ignore due to JDK-6492634 and > JDK-6501010, as running this test isn't harmful and can still be useful to > find other bugs, the test should be added to ProblemList.txt and @ignore tag > should be removed. > given JDK-6492634 is solaris-specific, and solaris port has been removed from > jdk15, the test will be problem-listed only due to JDK-6501010 and only on > windows. from main issue(8249618): > although ProblemList and @ignore achieve the same end result (test > exclusion), their server different goals and have slightly different > meanings, simplified @ignore should be used to exclude useless or harmful > tests, and ProblemList in all other cases (see yet-not-integrated > `ProblemListing or `@ignore`-ing a Test` section of dev guide, PR -- > https://github.com/openjdk/guide/pull/21 for more details). > > due to different reasons, this hasn't been always followed and some currently > @ignore-d tests should rather be ProblemList-ed, and some of ProblemList-ed > should be @ignore-d, this issue is to clean up the current state in a hope > that this will reduce further confusion. the patch also includes minimal changes to make the test runnable on macosx, which reveled that the test might fail on macos. 8249703 has been filed for that issue and the appropriate changes are done in ProblemList.txt. webrev: http://cr.openjdk.java.net/~iignatyev//8249700/webrev.01/ JBS: https://bugs.openjdk.java.net/browse/JDK-8249700 testing: java/io/File/GetXSpace.java on linux-x64 100 times, 100% passed; macos-x64 100 times, 71% passed PS the test is still failing on windows albeit due to the reason other than in 6501010; it seems that the test should be just updated to recognize cygwin's `df` output. I'm currently working on the fix and will send it out for review shortly. Thanks, -- Igor JDK-6492634: https://bugs.openjdk.java.net/browse/JDK-8249700 JDK-6501010: https://bugs.openjdk.java.net/browse/JDK-6501010 JDK-6492634: https://bugs.openjdk.java.net/browse/JDK-6492634 JDK-8249703: https://bugs.openjdk.java.net/browse/JDK-8249703
[15] RFR(T) : 8249698 : java/lang/invoke/LFCaching/LFGarbageCollectedTest.java should be ProblemList-ed and not @ignored
http://cr.openjdk.java.net/~iignatyev//8249698/webrev.00 > 3 lines changed: 1 ins; 1 del; 1 mod; Hi all, could you please review this trivial patch which removes @ignore from LFGarbageCollectedTest and adds it into problem-list instead? from 8249698: > java/lang/invoke/LFCaching/LFGarbageCollectedTest.java is excluded from > execution due to JDK-8078602. although the test might indeed fail due to > JDK-8078602, it still can be useful and isn't harmful to run, therefore this > test should be put in ProblemList.txt and @ignore is to be removed. from main issue(8249618): > although ProblemList and @ignore achieve the same end result (test > exclusion), their server different goals and have slightly different > meanings, simplified @ignore should be used to exclude useless or harmful > tests, and ProblemList in all other cases (see yet-not-integrated > `ProblemListing or `@ignore`-ing a Test` section of dev guide, PR -- > https://github.com/openjdk/guide/pull/21 for more details). > > due to different reasons, this hasn't been always followed and some currently > @ignore-d tests should rather be ProblemList-ed, and some of ProblemList-ed > should be @ignore-d, this issue is to clean up the current state in a hope > that this will reduce further confusion. webrev: http://cr.openjdk.java.net/~iignatyev//8249698/webrev.00 JBS: https://bugs.openjdk.java.net/browse/JDK-8249698 Thanks, -- Igor 8078602: https://bugs.openjdk.java.net/browse/JDK-8078602 8249618: https://bugs.openjdk.java.net/browse/JDK-8249618
[15] RFR(T) : 8249697 : java/lang/invoke/RicochetTest.java should use @requires instead of @ignore
http://cr.openjdk.java.net/~iignatyev/8249697/webrev.00/
> 7 lines changed: 4 ins; 0 del; 3 mod;
Hi all,
could you please review this small and trivial patch for
java/lang/invoke/RicochetTest.java test?
from JBS:
> a run of java/lang/invoke/RicochetTest.java w/ MAX_ARITY=255 was removed from
> all configurations by JDK-7049122, yet the problem manifests itself only w/
> Xcomp. as now we have @requires to filter out tests from certain
> configurations, the test can be updated to run MAX_ARITY=255 in all configs
> but Xcomp.
the patch splits the test into two subtests, each one w/ one @run, and use
@requires to exclude one w/ MAX_ARITY=255 from execution if Xcomp flag is used.
JBS: https://bugs.openjdk.java.net/browse/JDK-8249697
webrev: http://cr.openjdk.java.net/~iignatyev/8249697/webrev.00/
testing: java/lang/invoke/RicochetTest.java on {linux,windows,macos}-x64 w/ and
w/o -Xcomp; Xcomp runs, as expected, had only 1 test run
Thanks,
-- Igor
JDK-7049122 : https://bugs.openjdk.java.net/browse/JDK-7049122
Re: RFR: 8249264: Build validate-headers task fails after JDK-8248261
LGTM — Igor > On Jul 13, 2020, at 5:35 PM, alexander.matv...@oracle.com wrote: > > Please review the jpackage fix for bug [1] at [2]. > > Added missing ",". > > [1] https://bugs.openjdk.java.net/browse/JDK-8249264 > [2] http://cr.openjdk.java.net/~almatvee/8249264/webrev.00/ > > Thanks, > Alexander
Re: RFR(T): 8249097: test/lib/jdk/test/lib/util/JarBuilder.java has a bad copyright
LGTM, thanks for fixing, and sorry for introducing that. — Igor > On Jul 8, 2020, at 2:22 PM, Daniel D. Daugherty > wrote: > > Greetings, > > A trivial fix to make the validate-header Tier1 task happy. > > Here's the context diff: > > $ hg diff -r qparent > diff -r c5202ed40b86 test/lib/jdk/test/lib/util/JarBuilder.java > --- a/test/lib/jdk/test/lib/util/JarBuilder.javaWed Jul 08 20:35:36 2020 > +0100 > +++ b/test/lib/jdk/test/lib/util/JarBuilder.javaWed Jul 08 17:20:01 2020 > -0400 > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 2015, 2020 Oracle and/or its affiliates. All rights > reserved. > + * Copyright (c) 2015, 2020, 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 > > > Thanks, in advance, for any comments, questions, or suggestions. > > Dan >
Re: RFR(T): 8248358: ProblemList sun/nio/ch/TestMaxCachedBufferSize.java on macOSX
Hi Dan, LGTM Thanks, -- Igor > JDK-8248358 ProblemList > serviceability/jvmti/ModuleAwareAgents/ThreadStart/MAAThreadStart.java on > Windows > https://bugs.openjdk.java.net/browse/JDK-8248358 I guess you meant JDK-8248358 ProblemList sun/nio/ch/TestMaxCachedBufferSize.java on macOSX > On Jun 25, 2020, at 2:45 PM, Daniel D. Daugherty > wrote: > > Greetings, > > I'm doing another round of reduce-the-noise in the CI in preparation > for the upcoming weekend... So I have another trivial review... > > Here's the bug for the failures: > > JDK-8212812 sun/nio/ch/TestMaxCachedBufferSize.java timeout > https://bugs.openjdk.java.net/browse/JDK-8212812 > > and here's the bug for the ProblemListing: > > JDK-8248358 ProblemList > serviceability/jvmti/ModuleAwareAgents/ThreadStart/MAAThreadStart.java on > Windows > https://bugs.openjdk.java.net/browse/JDK-8248358 > > Here's the context diff: > > $ hg diff > diff -r cf65909b98c5 test/jdk/ProblemList.txt > --- a/test/jdk/ProblemList.txtThu Jun 25 15:00:59 2020 -0400 > +++ b/test/jdk/ProblemList.txtThu Jun 25 17:39:01 2020 -0400 > @@ -630,6 +630,8 @@ > > java/nio/channels/Selector/Wakeup.java 6963118 windows-all > > +sun/nio/ch/TestMaxCachedBufferSize.java 8212812 macosx-all > + > > > > Thanks, in advance, for any comments, questions or suggestions. > > Dan > > > > > >
Re: RFR(S) : 8211977 : move testlibrary tests into one place
Magnus, Erik, thanks for your reviews, pushed w/ a newline being added at L#654 of make/Main.gmk. Cheers, -- Igor > On Jun 16, 2020, at 7:03 AM, Magnus Ihse Bursie > wrote: > > On 2020-06-16 15:06, Erik Joelsson wrote: >> >> On 2020-06-15 17:39, Igor Ignatyev wrote: >>> @David, Erik, Magnus, >>> >>> please find the answers to your comments at the bottom of this email. >>> >>> @all, >>> >>> David's and Erik's comments made me realize that some parts of the original >>> patch were stashed away and didn't make it to webrev.00. I'm truly sorry >>> for the confusion and inconvenience. I also agree w/ David that it's hard >>> to follow/review, and my gaffe just made it worse, therefore I'd like to >>> start it over again from a clean sate >>> >>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02 >>> <http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02>Build changes >>> look good to me now, except for a missing newline in Main.gmk. (No need for >>> new review.) >> > > Ditto. > > /Magnus >> >> /Erik >> >>>> 833 lines changed: 228 ins; 591 del; 14 mod; >>> >>> could you please review the patch which puts all tests for common >>> testlibrary classes (which is ones located in /test/lib) into one location >>> under /test/lib-test? please note this intentionally doesn't move all >>> "testlibrary" tests, e.g. tests for CTW, hotspot-specific testlibrary, are >>> left in /test/hotspot/jtreg/testlibrary_tests/ctw . >>> >>> to simplify review, the patch has been split into several webrevs, with >>> each of them accomplishes a more-or-less distinct thing, but is not >>> necessary self-contained: >>> >>> 0. trivial move of tests from test/jdk and test/hotspot/jtreg test suites >>> to test/lib-test: >>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.00-move/ >>> <http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.00-move/> >>> >>> 1. removal of hotspot's AssertsTest, OutputAnalyzerReportingTest and >>> "merge" of hotspot's and jdk's OutputAnalyzerTest: >>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.01-merge/ >>> <http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.01-merge/> >>> >>> 2. simplification of TestNativeProcessBuilder tests: converts Test class >>> used by TestNativeProcessBuilder into a static nested class: >>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.02-TestNativeProcessBuilder >>> >>> <http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.02-TestNativeProcessBuilder> >>> >>> 3. makefiles changes to build native parts of lib-test tests. in past, >>> there were no tests w/ native parts in this test suite, >>> TestNativeProcessBuilder is the 1st such test; this part also removes now >>> unneeded lines from hotspot test suite makefile: >>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.03-NativeLibTest >>> <http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.03-NativeLibTest> >>> >>> 4. clean ups in hotspot test suite: adjusted location of >>> SimpleClassFileLoadHookTest test, which is a test for hotspot-specific test >>> library (located in /test/hotspot/jtreg/testlibrary/jvmti) and hence should >>> not be moved to /test/lib-test; removed >>> TestMutuallyExclusivePlatformPredicates from TEST.groups: >>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.04-hotspot/ >>> <http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.04-hotspot/> >>> >>> 5. LingeredAppTest fix (which apparently has never been run before): >>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.05-LingeredAppTest/ >>> <http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.05-LingeredAppTest/> >>> >>> 6. changes to make test/lib-test a fully supported and working test suite, >>> and add in into tier1, includes adding ProblemList, TEST.groups file, and >>> 'randomness' k/w: >>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.06-TESTROOT/ >>> <http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.06-TESTROOT/> >>> >>> webrev: http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02 >>> <http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02> >>> JBS: https://bugs.openjdk.java.net/browse/JDK-8211977
Re: RFR(S) : 8211977 : move testlibrary tests into one place
Hi David,
thanks for your review. re: LingeredAppTest, I agree that the test doesn't look
very useful, yet I'd remind that the goal of this (and other test in
/test/lib-test) is to (sanity) test testlibrary in order to easily spot bugs in
testlibrary in a clear manner so one would not have to investigate failures of
actual "product" tests and go thru their sometimes convoluted logic just to
find out that the problem was in LingeredApp. w/ that being said, this test can
do a better job in testing LingeredApp, so I'll file a JBS ticket against
hotspo/svc to evaluate/improve/discuss the fate of the test.
Thanks,
-- Igor
> On Jun 16, 2020, at 12:14 AM, David Holmes wrote:
>
> Hi Igor,
>
> On 16/06/2020 10:39 am, Igor Ignatyev wrote:
>> @David, Erik, Magnus,
>> please find the answers to your comments at the bottom of this email.
>> @all,
>> David's and Erik's comments made me realize that some parts of the original
>> patch were stashed away and didn't make it to webrev.00. I'm truly sorry for
>> the confusion and inconvenience. I also agree w/ David that it's hard to
>> follow/review, and my gaffe just made it worse, therefore I'd like to start
>> it over again from a clean sate
>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02
>>> 833 lines changed: 228 ins; 591 del; 14 mod;
>> could you please review the patch which puts all tests for common
>> testlibrary classes (which is ones located in /test/lib) into one location
>> under /test/lib-test? please note this intentionally doesn't move all
>> "testlibrary" tests, e.g. tests for CTW, hotspot-specific testlibrary, are
>> left in /test/hotspot/jtreg/testlibrary_tests/ctw .
>
> Ok.
>
>> to simplify review, the patch has been split into several webrevs, with each
>> of them accomplishes a more-or-less distinct thing, but is not necessary
>> self-contained:
>
> Many thanks for doing this!
>
>> 0. trivial move of tests from test/jdk and test/hotspot/jtreg test suites to
>> test/lib-test:
>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.00-move/
>
> Looks good.
>
>> 1. removal of hotspot's AssertsTest, OutputAnalyzerReportingTest and "merge"
>> of hotspot's and jdk's OutputAnalyzerTest:
>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.01-merge/
>
> Looks good.
>
>> 2. simplification of TestNativeProcessBuilder tests: converts Test class
>> used by TestNativeProcessBuilder into a static nested class:
>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.02-TestNativeProcessBuilder
>
> Looks good.
>
>> 3. makefiles changes to build native parts of lib-test tests. in past, there
>> were no tests w/ native parts in this test suite, TestNativeProcessBuilder
>> is the 1st such test; this part also removes now unneeded lines from hotspot
>> test suite makefile:
>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.03-NativeLibTest
>
> Hmm okay. Not sure this needed its own category of build logic but ...
>
> Aside: Makefiles should not have the classpath exception version of the
> license header. But they all do for some reason. I'll check if we need to do
> a separate cleanup of that.
>
>> 4. clean ups in hotspot test suite: adjusted location of
>> SimpleClassFileLoadHookTest test, which is a test for hotspot-specific test
>> library (located in /test/hotspot/jtreg/testlibrary/jvmti) and hence should
>> not be moved to /test/lib-test; removed
>> TestMutuallyExclusivePlatformPredicates from TEST.groups:
>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.04-hotspot/
>
> Looks good. Took me a while to understand the connection to the test library
> here.
>
>> 5. LingeredAppTest fix (which apparently has never been run before):
>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.05-LingeredAppTest/
>
> Someone from serviceability should evaluate this test. It may not be needed.
>
>> 6. changes to make test/lib-test a fully supported and working test suite,
>> and add in into tier1, includes adding ProblemList, TEST.groups file, and
>> 'randomness' k/w:
>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.06-TESTROOT/
>
> Seems okay.
>
> Thanks,
> David
> -
>
>> webrev: http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8211977
>> testing:
>> - make test TEST=tier1 locally on macosx
>> - test/lib-test on {windows,linux,macosx}-x64
>> - tier1 job (in progress)
>> Thanks,
>> -- Igor
>>> On Jun 14, 2020, at 11:
Re: RFR(S) : 8211977 : move testlibrary tests into one place
@David, Erik, Magnus,
please find the answers to your comments at the bottom of this email.
@all,
David's and Erik's comments made me realize that some parts of the original
patch were stashed away and didn't make it to webrev.00. I'm truly sorry for
the confusion and inconvenience. I also agree w/ David that it's hard to
follow/review, and my gaffe just made it worse, therefore I'd like to start it
over again from a clean sate
http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02
> 833 lines changed: 228 ins; 591 del; 14 mod;
could you please review the patch which puts all tests for common testlibrary
classes (which is ones located in /test/lib) into one location under
/test/lib-test? please note this intentionally doesn't move all "testlibrary"
tests, e.g. tests for CTW, hotspot-specific testlibrary, are left in
/test/hotspot/jtreg/testlibrary_tests/ctw .
to simplify review, the patch has been split into several webrevs, with each of
them accomplishes a more-or-less distinct thing, but is not necessary
self-contained:
0. trivial move of tests from test/jdk and test/hotspot/jtreg test suites to
test/lib-test:
http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.00-move/
1. removal of hotspot's AssertsTest, OutputAnalyzerReportingTest and "merge" of
hotspot's and jdk's OutputAnalyzerTest:
http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.01-merge/
2. simplification of TestNativeProcessBuilder tests: converts Test class used
by TestNativeProcessBuilder into a static nested class:
http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.02-TestNativeProcessBuilder
3. makefiles changes to build native parts of lib-test tests. in past, there
were no tests w/ native parts in this test suite, TestNativeProcessBuilder is
the 1st such test; this part also removes now unneeded lines from hotspot test
suite makefile:
http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.03-NativeLibTest
4. clean ups in hotspot test suite: adjusted location of
SimpleClassFileLoadHookTest test, which is a test for hotspot-specific test
library (located in /test/hotspot/jtreg/testlibrary/jvmti) and hence should not
be moved to /test/lib-test; removed TestMutuallyExclusivePlatformPredicates
from TEST.groups:
http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.04-hotspot/
5. LingeredAppTest fix (which apparently has never been run before):
http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.05-LingeredAppTest/
6. changes to make test/lib-test a fully supported and working test suite, and
add in into tier1, includes adding ProblemList, TEST.groups file, and
'randomness' k/w:
http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.06-TESTROOT/
webrev: http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02
JBS: https://bugs.openjdk.java.net/browse/JDK-8211977
testing:
- make test TEST=tier1 locally on macosx
- test/lib-test on {windows,linux,macosx}-x64
- tier1 job (in progress)
Thanks,
-- Igor
> On Jun 14, 2020, at 11:23 PM, David Holmes wrote:
> <...>
> This doesn't seem to move everything under
> test/hotspot/jtreg/testlibrary_tests/ e.g. ctw directory.
this is intentionally, ctw test-library is hotspot specific, hence its tests
are to reside in hotspot test suite (until we decide to move the library to
/test/lib), the same is true for SimpleClassFileLoadHookTest.
>
> You did not modify hotspot/jtreg/TEST.groups but it refers to:
> testlibrary_tests/TestMutuallyExclusivePlatformPredicates.java \
> Plus it should probably refer to the new native_sanity tests somewhere.
the actual version of the patch removed reference to
TestMutuallyExclusivePlatformPredicates from hotspot's TEST.groups and had
TestNativeProcessBuilder moved to /test/test-lib, so no updates w.r.t.
native_sanity are needed
> The newly added copyright notice needs to have the correct initial copyright
> year based on when the file was first added to the repo.
/test/lib-test/TEST.ROOT file was created by JDK-8243010 on 2020-04-16, hence
the added copyright has 2020 as the initial copyright year.
> You used the wrong license header - makefiles don't use the classpath
> exception.
JtregNativeLibTest.gmk is based on make/test/JtregNativeJdk.gmk which also have
classpath exception. quick grep showed that make directory has 794 files which
has '"Classpath" exception', out of 850 which contain 'GNU General Public
License version 2' string. And although I agree that makefiles shouldn't have
the classpath exception, I'd prefer to keep JtregNativeLibTest.gmk in sync w/
the its siblings and would leave it up to build team to decide how it's best to
handle.
> On Jun 15, 2020, at 8:12 AM, Magnus Ihse Bursie
> wrote:
>
> A few comments:
>
> This seems like code copied from elsewhere:
> 57 # This evaluation is expensive and should only be done if this target was
> 58 # explicitly called.
> 59 ifneq ($(filter build-test-libtest-jtreg-native, $(MAKECMDGOALS)), )
> I don't agree that
Re: RFR 15 8247521: (test) jdk/test/lib/hexdump/HexPrinterTest.java fails on windows
Hi Roger, LGTM, Thanks, — Igor > On Jun 15, 2020, at 7:10 AM, Roger Riggs wrote: > > Roger
