RFR JDK-8217761: Enhance run-test-prebuilt jib profile to support running tests with JCov

[Erik Joelsson]

On 2019-01-30 06:48, Magnus Ihse Bursie wrote:
>
>
> On 2019-01-29 17:55, Erik Joelsson wrote:
>> Hello,
>>
>> On 2019-01-29 03:12, Magnus Ihse Bursie wrote:
>>> If you are going down this road, you'll need to verify that 
>>> JCOV_IMAGE_DIR is not empty in RunTests. So keep the ifneq 
>>> statement, but with an $(error ...) instead. Otherwise we'll just 
>>> fail spectacularly and incomprehensible later on.
>>
>> This has already gone in with my blessing, but to address your concern.
>>
>> I don't see why we need to check that it's set in RunTest.gmk? In the 
>> local case, we set JCOV_IMAGE_DIR in spec.gmk so it's guaranteed to 
>> be set, 
>
> ... but only if we configure with --with-jcov.
>
> I'd just like the system to present a comprehensible error message if 
> the user does something like this: "make test TEST=tier1 
> TEST_OPTS=JCOV=true", but has not configured using --with-jcov. Prior 
> to this patch, we ran the tests but without coverage. This is probably 
> not what was intended, but it didn't crash. Now we're going to get a 
> malformed command line.
>
> "Error: Unable to access jarfile /lib/jcov.jar" is not a very helpful 
> error message.
>
> When I test now, something is odd with the "jcov-image" target as 
> well. Even if you have not run configure with --with-jcov, the target 
> seemingly runs and succeeds, but no jcov image is generated. Expected 
> behavior would be an error message stating that you need to use 
> --with-jcov.
>
Ah I see, but the previously existing conditional only checked if the 
variable JCOV_IMAGE_DIR was set, not if the directory it pointed to 
existed, so it wasn't any better. We set the variable JCOV_IMAGE_DIR 
unconditionally in spec.gmk, so that check would always be true anyway.

What you are asking for is adding a new check that JCOV_HOME is set 
(this is a consequence of setting --with-jcov) and that JCOV_IMAGE_DIR 
actually points to something valid (this is the result of building 
jcov-image). It would also be nice if the jcov-image target failed with 
a reasonable error if JCOV_HOME wasn't set. I agree that these checks 
would be useful.

/Erik

> /Magnus
>
>> and in the prebuilt case, it's the responsibility of 
>> RunTestPrebuilt.gmk to adjust things for the lack of a proper 
>> spec.gmk and in RunTestPrebuilt.gmk we already check that 
>> JDK_IMAGE_DIR points to an existing directory.
>>
>> /Erik
>>
>>> /Magnus
>>>
>>>> 28 jan. 2019 kl. 18:47 skrev Erik Joelsson <erik.joelsson at oracle.com>:
>>>>
>>>> Hello Shura,
>>>>
>>>> In RunTest.gmk, you removed the conditional, but the comment still 
>>>> implies that it's there. Please update the comment. Otherwise this 
>>>> looks good!
>>>>
>>>> /Erik
>>>>
>>>>> On 2019-01-25 15:20, Alexandre (Shura) Iline wrote:
>>>>> Hi,
>>>>>
>>>>> Please take a look on a change to allow JCov test execution 
>>>>> through jib.
>>>>>
>>>>> Please notice that this fix also changes behavior of RunTest.gmk 
>>>>> when conflicting combination of options is used. JCOV_IMAGE_DIR  
>>>>> is now expected to be set when TEST_OPTS_JCOV is true.
>>>>>
>>>>> Shura
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8217761
>>>>> Webrev: http://cr.openjdk.java.net/~shurailine/8217761/webrev.02/
>