Hi Lance, Thanks for addressing my points. Here my answer to those things which weren't in full agreement yet:
> I dislike a bit the fact that we introduce a new testng subfolder in > test/jdk/nio/zipfs. Wouldn’t it be possible to consolidate in the current test > folder? > > I am trying to add some organization separating the non-testng tests from > the testng tests and other testng tests will be moved here. I did the same > for JDBC a few years back ok, maybe it's a good idea to do this here and gradually move all testng tests over. > - line 387: Manfiest -> Manifest I think you missed that one > - line 417: Parameter "final Map<?, ?> > attributes" of ManifestOrderTest::verify should be renamed to > "manifestAttributes" to make it easier to understand its purpose > > > Prefer to leave as is as it gets wordy and I believe the description is clear > in > the comments Hm, I needed to look twice to grasp it. So, I'd still prefer something with "manifest" in the variable name. But I leave it to you since it's probably a personal taste thing 😉 However, two more things here: The Javadoc of verify says (line 412): * @param attributes Is there a Manifest to check You should rather go with the Javadoc of validateManifest here as well: * @param attributes A Map containing the attributes expected in the Manifest; * otherwise empty Also, I spotted in the Javadoc, line 413: * @param entries Entries to validate are in the JAR I guess the "are" is wrong here. > test/jdk/jdk/nio/zipfs/testng/util/ZipFsBaseTest.java: > - rename to ZipFSBaseTest (Capital ‘S’)?? > hmm I had that originally but did not like it… but don’t have a strong > preference Ok, leave it as you have it 😊 > - line 120: public static void verify - > this method is not used by > ManifestOrderTest. I think we should only add it when having a test that > really uses this verify method. But I generally agree that the verify method > is > a candidate for communalization > > This will be used by some tests I have created and some I will be moving so > rather add it now on the initial push. This is used by several tests that > will be > migrated > > - line 176: public void zip: dito, this method doesn’t seem used. So I suggest > to remove it for this change > Same comment as above OK. > The webrev for the above > is http://cr.openjdk.java.net/~lancea/8211917/webrev.01/index.html Looks good, apart from my comments above. Thanks Christoph
