RFR: 8192837 Need new test for release file info

[David Holmes]

On 22/12/2017 2:41 AM, Randy Crihfield wrote:
> I think I have it now.
> 
> http://cr.openjdk.java.net/~shurailine/8192837/webrev.05/

I still find the name NegativeSOURCETest less than informative. I don't 
know what naming style you are employing here but it is not a style we 
generally use in OpenJDK (though langtools does have some "neg" tests).

You still need a sponsor.

David

> 
> Thanks for the help again!
> 
> Randy
> 
> 
> On 12/20/17 08:30 PM, David Holmes wrote:
>> 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
>>>>>>>>
>>>>>>>
>>>>>
>>>
>