Re: RFR: 8192837 Need new test for release file info

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8192837 Need new test for release file info

Erik Joelsson
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
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8192837 Need new test for release file info

David Holmes
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
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8192837 Need new test for release file info

Randy Crihfield-2
>
> This ought to be what you were asking for.
>
> http://cr.openjdk.java.net/~shurailine/8192837/webrev.03/
>
>
> 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
>>>>
>>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8192837 Need new test for release file info

David Holmes
In reply to this post by David Holmes
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
>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8192837 Need new test for release file info

Randy Crihfield-2

Could this be it?

http://cr.openjdk.java.net/~shurailine/8192837/webrev.04/

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
>>>>>
>>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: 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
>>>>>>
>>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8192837 Need new test for release file info

Randy Crihfield-2
I think I have it now.

http://cr.openjdk.java.net/~shurailine/8192837/webrev.05/


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
>>>>>>>
>>>>>>
>>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: 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
>>>>>>>>
>>>>>>>
>>>>>
>>>
>