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
