Hi Christoph > On Mar 5, 2020, at 4:03 AM, Langer, Christoph <christoph.lan...@sap.com> > wrote: > > Hi Lance, > > Thanks for addressing my points. Here my answer to those things which weren't > in full agreement yet:
Please see below > >> 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 Fixed must have not saved it but it is there now > > >> - 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 😉 I left the variable name as to your point it sometimes is a personal preference ;-) > > 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 Updated… Thought I had done that previously as that @param was originally a boolean …. > > Also, I spotted in the Javadoc, line 413: > * @param entries Entries to validate are in the JAR > > I guess the "are" is wrong here. Fixed > >> 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. Thank you again for the review The revised webrev is at http://cr.openjdk.java.net/~lancea/8211917/webrev.02/index.html <http://cr.openjdk.java.net/~lancea/8211917/webrev.02/index.html> Best Lance > > Thanks > Christoph > <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
