Hi Per,
you are reading this correctly, make
TEST=test/hotspot/jtreg/gc/z/TestSmallHeap.java JTREG="VM_OPTIONS=-XX:+UseZGC"
won't execute gc/z/TestSmallHeap.java; and I don't see it to be incorrect. Let
me try to explain why using gc/z/TestSmallHeap.java as a running example.
A hotspot test is expected not to be just runnable in an out-of-box
configuration, but also to serve its purpose as much as possible (which is not
always 100% given some tests require special build flavor, environment setup,
etc); in other words, a test is to at least have all necessary VM flags within
it and not to hope that someone will provide them. gc/z/TestSmallHeap.java does
that, it explicitly selects zGC, so there is no need for -XX:+UseZGC to achieve
that. Given this test can be run only when zGC can be selected, it @requires
vm.gc.Z, which is set to true if zGC is already explicitly selected or if zGC
is available and no other GC is specified, and the latter holds for an
out-of-box configuration (assuming that zGC is available in the JVM under
test); thus, again, you don't have to specify -XX:+UseZGC to run this test. So
there are no "technical" reasons to run gc/z/TestSmallHeap.java (or any other
gc/z/ tests) with -XX:+UseZGC. The proposed patches don't change that fact in
any way.
The patches exclude the tests that ignore external VM flags from execution if
any significant VM flags are specified. gc/z/TestSmallHeap.java ignores all
externally provided VM flags, including -XX:+UseZGC. And although in the case
of -XX:+UseZGC, it's harmless, in almost all other cases it's not. Just to give
you a few examples:
Let's say you are fixing a bug in zGC which could be reproduced by
gc/z/TestSmallHeap.java. You came up with two alternative solutions, one of
which is guarded by `if (UseNewCode)`. To test these solutions, you ran gc/z
tests twice: with -XX:+UseZGC -XX:+UseNewCode, and all tests passed; with
XX:+UseZGC, and many tests (but not gc/z/TestSmallHeap.java) failed. So based
on these results, you decided that the guarded solution is perfect, cleaned up
the code, sent it out for review, got it pushed, and minutes later found out
that gc/z/TestSmallHeap.java and some other tests which ignore VM flags failed.
It would take you some time, to realize that you hadn't tested your UseNewCode
solution by these tests. Yet were these tests excluded from your testing, it
would be much easier for you to spot that and react accordingly.
Here is another scenario, you decided to change the default value of
ZUncommit, so you ran different tests with `XX:+UseZGC -XX:-ZUncommit`, all
green, you pushed a trivial change s/true/false in z_globals.hpp, next thing
you knew a bunch of zGC specific tests failed in CI. And again, these were the
tests that silently ignored `XX:+UseZGC -XX:-ZUncommit`.
Or a slight variation, zGC-supported was added to a future JIT, gc/z
tests were run with the flag combination which enabled the future JIT, all
passed, the victory was declared; N releases later; default JIT got changed to
the future JIT; the next CI build is a disaster, with lots of tests failing
from the bugs which had not been found N/2 years ago.
Although I understand that it might take some getting used to from you and
others who used to run gc/x tests with -XX:+Use${X}GC, I am certain that this
will improve the overall quality of hotspot, save not only machine time (from
running these tests with other flags) but engineers time from analyzing
surprising failures, and increase confidence and trust in the hotspot test
suite.
In a word, I can see how this can be a bit surprising, yet still less
surprising than the current behavior, but I don't see it as incorrect, it just
surfaces limitations of certain tests. From my (slightly biased) point of view,
it's the right thing to do.
Thanks.
-- Igor
> On Jun 5, 2020, at 1:20 AM, Per Liden <[email protected]> wrote:
>
> Hi Igor,
>
> When looking at the follow-up sub-tasks for this, I see for example this:
>
> http://cr.openjdk.java.net/~iignatyev/8246499/webrev.00/test/hotspot/jtreg/gc/z/TestSmallHeap.java.udiff.html
>
> Maybe I'm misunderstanding how this is supposed to work, but it looks like
> this test would now _not_ be executed if I do:
>
> make TEST=test/hotspot/jtreg/gc/z/TestSmallHeap.java
> JTREG="VM_OPTIONS=-XX:+UseZGC"
>
> Is that so? In that case, that seems incorrect.
>
> cheers,
> Per
>
> On 6/3/20 11:30 PM, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev//8246494/webrev.00
>>> 70 lines changed: 66 ins; 0 del; 4 mod
>> Hi all,
>> could you please review the patch which introduces a new @requires property
>> to filter out the tests which ignore externally provided JVM flags?
>> the idea behind this patch is to have a way to clearly mark tests which
>> ignore flags, so
>> a) it's obvious that they don't execute a flag-guarded code/feature, and
>> extra care should be taken to use them to verify any flag-guarded changed;
>> b) they can be easily excluded from runs w/ flags.
>> @requires and VMProps allows us to achieve both, so it's been decided to add
>> a new property `vm.flagless`. `vm.flagless` is set to false if there are any
>> XX flags other than `-XX:MaxRAMPercentage` and `-XX:CreateCoredumpOnCrash`
>> (which are known to be set almost always) or any X flags other `-Xmixed`; in
>> other words any tests w/ `@requires vm.flagless` will be excluded from runs
>> w/ any other X / XX flags passed via `-vmoption` / `-javaoption`. in rare
>> cases, when one still wants to run the tests marked by `vm.flagless` w/
>> external flags, `vm.flagless` can be forcefully set to true by setting any
>> value to `TEST_VM_FLAGLESS` env. variable.
>> this patch adds necessary common changes and marks common tests, namely
>> Scimark, GTestWrapper and TestNativeProcessBuilder. Component-specific tests
>> will be marked separately by the corresponding subtasks of 8151707[1].
>> please note, the patch depends on CODETOOLS-7902336[2], which will be
>> included in the next jtreg version, so this patch is to be integrated only
>> after jtreg5.1 is promoted and we switch to use it by 8246387[3].
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8246494
>> webrev: http://cr.openjdk.java.net/~iignatyev//8246494/webrev.00
>> testing: marked tests w/ different XX and X flags w/ and w/o
>> TEST_VM_FLAGLESS env. var, and w/o any flags
>> [1] https://bugs.openjdk.java.net/browse/JDK-8151707
>> [2] https://bugs.openjdk.java.net/browse/CODETOOLS-7902336
>> [3] https://bugs.openjdk.java.net/browse/JDK-8246387
>> Thanks,
>> -- Igor
