On Mon, 23 Oct 2023 10:39:11 GMT, Sean Coffey <coff...@openjdk.org> wrote:
> ZipFile behaviour in the scenario where a file is re-opened and edited > while references to the old ZipFile exist. A "invalid LOC header" needs to be > thrown in cases where an old zip file mapping is still in use. This PR doesn't do any source changes to introduce this behaviour. It instead just adds a test to assert this existing behaviour. So that's a good thing and testing this behaviour sounds fine to me. > the behaviour of incorrectly adding extra <Key, Source> mappings to our > internal map has gone undetected for a few years. > We've no test coverage in that area. The current test closely monitors > numbers in that map now and allows for behavioural differences between > Filesystems that support and don't support fileKey(). I think that protects > us > from future changes we might make in this area also. What you propose here sounds fine. The only part that I think will need a bit more experiments is to run something like a test-repeat of 50 or more to make sure that the number of jars opened when this test is running doesn't get impacted by any unforeseen loading of classes or such (from the test framework for example) which could end up opening some more jar files which can then cause the shared static map to have a different count of jars than this test would expect. This is already a `/othervm` test so intereference is already reduced, but it would still be good to see if there's any scope of intermittent failures, by runinng a test-repeat job. > I think your test will fail on windows. The rel and abs file Keys are still > treated as different there (proposal to use getCanonicalPath seems too costly > for performance) You are right, I missed that part where the `equals()` may return false on Windows. So that part can't actually be asserted. The `hashCode()` too then needs to be asserted only if the instances are `equals()`. > While your suggested test case captures the core issue at hand, I think it > misses a few important points that I captured in the current test. Thank you for the additional details explaining the rationale behind this test. What you propose sounds fine to me then. I've a few minor nits about this test, which I'll add inline. ------------- PR Comment: https://git.openjdk.org/jdk/pull/16115#issuecomment-1775026122
