On Thu, 9 Dec 2021 18:07:58 GMT, Andrew Leonard <aleon...@openjdk.org> wrote:
>> Add a new --source-date <TIMESTAMP> (epoch seconds) option to jar and jmod
>> to allow specification of time to use for created/updated jar/jmod entries.
>> This then allows the ability to make the content deterministic.
>>
>> Signed-off-by: Andrew Leonard <anleo...@redhat.com>
>
> Andrew Leonard has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8276766: Enable jar and jmod to produce deterministic timestamped content
>
> Signed-off-by: Andrew Leonard <anleo...@redhat.com>
Hi Andrew,
Thank you for splitting out the test into its own file and addressing the test
issues we found.
Overall looks good. I have suggested some additional changes so the test is
slightly more TestNG friendly and a few places where it would be useful to add
comments
test/jdk/tools/jar/ReproducibleJar.java line 30:
> 28: * reproducible
> 29: * @modules jdk.jartool
> 30: * @run testng ReproducibleJar
Probably use othervm given you are toggling the TZ for extra safety
test/jdk/tools/jar/ReproducibleJar.java line 49:
> 47: import java.time.LocalDateTime;
> 48: import java.time.ZonedDateTime;
> 49: import java.time.ZoneId;
Duplicate import? Perhaps get rid to the import of line 43
test/jdk/tools/jar/ReproducibleJar.java line 74:
> 72: private static final File jarFileSourceDate1 = new
> File("JarEntryTimeSourceDate1.jar");
> 73: private static final File jarFileSourceDate2 = new
> File("JarEntryTimeSourceDate2.jar");
> 74:
Please consider using caps for your static final variable names
test/jdk/tools/jar/ReproducibleJar.java line 87:
> 85: "2098-02-18T00:00:00-08:00",
> 86: "2099-12-31T23:59:59+00:00"};
> 87:
I would suggest converting to a DataProvider for the test.
test/jdk/tools/jar/ReproducibleJar.java line 95:
> 93: "2006-04-06T12:38:00",
> 94: "2012-08-24T16"};
> 95:
Another good use of a DataProvider
test/jdk/tools/jar/ReproducibleJar.java line 102:
> 100: jarFileSourceDate1.delete();
> 101: jarFileSourceDate2.delete();
> 102: TimeZone.setDefault(TZ);
I believe you could just call runAfter() from within this method
test/jdk/tools/jar/ReproducibleJar.java line 114:
> 112: }
> 113:
> 114: @Test
Please add a comment introducing the intent of the test. As mentioned above,
please consider using a DataProvider vs. an array with the test.
test/jdk/tools/jar/ReproducibleJar.java line 120:
> 118: // Test --date source date
> 119: for (String sourceDate : sourceDates) {
> 120: createOuterInnerDirs(dirOuter, dirInner);
If you use a DataProvider, you could call this method from your runBeforeMethod
test/jdk/tools/jar/ReproducibleJar.java line 147:
> 145: cleanup(dirInner);
> 146: cleanup(dirOuter);
> 147: jarFileSourceDate1.delete();
Is the above needed given your `@AfterMethod`
test/jdk/tools/jar/ReproducibleJar.java line 152:
> 150:
> 151: @Test
> 152: public void testInvalidSourceDate() throws Throwable {
Please add a basic comment of the intent of the method and consider using a
DataProvider
test/jdk/tools/jar/ReproducibleJar.java line 165:
> 163:
> 164: @Test
> 165: public void testJarsReproducible() throws Throwable {
Please add a basic comment of the intent of the method and consider using a
DataProvider
test/jdk/tools/jar/ReproducibleJar.java line 193:
> 191:
> 192: // Check jars are identical
> 193: checkSameContent(jarFileSourceDate1, jarFileSourceDate2);
You could if you choose use:
Assert.assertEquals(Files.readAllBytes(jarFileSourceDate1.toPath(),
Files.readAllBytes(jarFileSourceDate2.toPath()));
test/jdk/tools/jar/ReproducibleJar.java line 199:
> 197: jarFileSourceDate1.delete();
> 198: jarFileSourceDate2.delete();
> 199: }
I believe your` @AfterMethod` will address this so the above is not needed
test/jdk/tools/jar/ReproducibleJar.java line 202:
> 200: }
> 201:
> 202: static void createOuterInnerDirs(File dirOuter, File dirInner)
> throws Throwable {
I believe you are always passing in the same parameter values, so perhaps no
need for the parameters?
test/jdk/tools/jar/ReproducibleJar.java line 208:
> 206: * foo.txt
> 207: */
> 208: Assert.assertTrue(dirOuter.mkdir());
Please move the comment above the method
test/jdk/tools/jar/ReproducibleJar.java line 219:
> 217: }
> 218:
> 219: static void checkFileTime(long now, long original) throws Throwable {
Please add a basic comment of the intent of the method
test/jdk/tools/jar/ReproducibleJar.java line 241:
> 239: }
> 240:
> 241: static void checkSameContent(File f1, File f2) throws Throwable {
See comment above for consideration regarding the use of Assert.assertEquals
test/jdk/tools/jar/ReproducibleJar.java line 249:
> 247: }
> 248:
> 249: private static boolean isTimeSettingChanged() {
Please add a basic comment describing the intent of the method
test/jdk/tools/jar/ReproducibleJar.java line 260:
> 258: }
> 259:
> 260: private static boolean isInTransition() {
Please add a basic comment of the intent of the method
test/jdk/tools/jar/ReproducibleJar.java line 278:
> 276: File[] x = dir.listFiles();
> 277: if (x != null) {
> 278: for (int i = 0; i < x.length; i++) {
Could use enhanced for loop here if you desire
test/jdk/tools/jar/ReproducibleJar.java line 286:
> 284:
> 285: static void extractJar(File jarFile) throws Throwable {
> 286: String javahome = System.getProperty("java.home");
Please add a basic comment of the intent of the method.
Any reason you chose not to use JAR_TOOL here?
-------------
Marked as reviewed by lancea (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/6481
