RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start

[Langer, Christoph]

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