RFR: 8192837 Need new test for release file info

[David Holmes]

On 21/12/2017 2:51 AM, Randy Crihfield wrote:
> 
> Could this be it?
> 
> http://cr.openjdk.java.net/~shurailine/8192837/webrev.04/

29  * @run testing NegReleaseSOURCE

If you are going to write a jtreg test you have to actually run it under 
jtreg! That @run line is invalid - "testing" is not an @run action (did 
you perchance try to copy a testng @run entry?). A minimal @run for this 
test would be:

@run main NegReleaseSOURCE

As for the name ... sorry I don't know what NegReleaseSOURCE is supposed 
to mean. There's no reason to be cryptic (that's why we stopped using 
bug numbers to name tests). You could even add a subdirectory for 
release file related tests:

sanity/releaseFile/CheckSOURCE.java

Thanks,
David

> Thanks!
> 
> Randy
> 
> On 12/20/17 04:51 AM, David Holmes wrote:
>> On 20/12/2017 2:23 AM, Randy Crihfield wrote:
>>>
>>> This ought to be what you were asking for.
>>>
>>> http://cr.openjdk.java.net/~shurailine/8192837/webrev.03/
>>
>> Closer :) A few nits:
>>
>> - the test needs a proper @run tag
>> - NegSOURCE is hardly informative - how about CheckReleaseFile?
>> - there are a couple of style nits with indentation of wrapped lines:
>>
>>   71             throw new RuntimeException("File " + fileName +
>>   72                 " not found reading data!", fileExcept);
>>   73         } catch (IOException ioExcept) {
>>   74             throw new RuntimeException("Unexpected problem 
>> reading data!",
>>   75             ioExcept);
>>
>> should be:
>>
>>   71             throw new RuntimeException("File " + fileName +
>>   72                                        " not found reading 
>> data!", fileExcept);
>>   73         } catch (IOException ioExcept) {
>>   74             throw new RuntimeException("Unexpected problem 
>> reading data!",
>>   75                                         ioExcept);
>>
>> As a general style comment there's no need to use instance methods 
>> here, you could just define readFile as static, or even inline it into 
>> main directly.
>>
>> Thanks,
>> David
>>
>>>
>>> Thanks for the help
>>>
>>> Randy
>>>
>>> On 12/18/17 09:32 PM, David Holmes wrote:
>>>> Hi Randy,
>>>>
>>>> jdk/sanity/Test8192837.java
>>>>
>>>> We don't name tests with bug numbers any more - the file/class 
>>>> should be renamed to something appropriate to its actual function.
>>>>
>>>>   64                 // grab the line
>>>>   65                 if (readIn.startsWith("SOURCE="))
>>>>   66                     fishForSOURCE = readIn;
>>>>
>>>> Do you expect to find more than one SOURCE line? If not this should 
>>>> "break". If so, then you're only going to check the last one found.
>>>>
>>>>   98         if (runtime.contains("OpenJDK"))
>>>>   99             new Test8192837(jdkPath + "/release");
>>>>  100         else
>>>>  101             System.out.println("Not an OpenJDK.");
>>>>
>>>> It would be preferable if this can be done via some @requires tag 
>>>> rather than within the test. But otherwise it would be better to 
>>>> print "Test skipped: not an OpenJDK build".
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>> On 19/12/2017 3:23 AM, Erik Joelsson wrote:
>>>>> Redirecting to correct list.
>>>>>
>>>>> The test seems to do what it set out to do.
>>>>>
>>>>> /Erik
>>>>>
>>>>>
>>>>> On 2017-12-18 17:55, Randy Crihfield wrote:
>>>>>> I have created an OpenJDK negative test that confirms the closed 
>>>>>> source files are not included in the SOURCE.
>>>>>>
>>>>>> Version of the actual test for review:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~shurailine/8192837/webrev.00/
>>>>>>
>>>>>> Any comments/suggestions are welcome, also I will need a sponsor 
>>>>>> for it at the end…
>>>>>>
>>>>>> Randy
>>>>>>
>>>>>
>>>
>