marschall / memoryfilesystem

An in memory implementation of a JSR-203 file system
282 stars 36 forks source link

Fix failing Maven build on Windows and improve nested ZIP tests #157

Open kriegaex opened 8 months ago

kriegaex commented 8 months ago

Fix failing Maven build on Windows

Two parametrised test iterations using symlinks on native file systems were failing on Windows, because there we do not have UNIX-style symlinks. Skipping the whole test using @DisabledOnOs would have been an option, but a suboptimal one, because then the iteration testing symlinks on the in-memory FS would also have been skipped. Therefore, an assumption is used:

Assumptions.assumeFalse(
  useDefault && OS.current().equals(OS.WINDOWS),
  "skip symbolic link test on Windows default file system"
);

Conditionally enable nested zip tests depending on JDK version

Since JDK 12, ZipFileSystem technically supports nested zips, albeit only when using Path, not URI parameters. The corresponding OpenJDK commit is https://github.com/openjdk/jdk/commit/196c20c0d1.

Unfortunately, only since JDK 13 there is an overloaded method FileSystems.newFileSystem(Path, Map), which also permits to create a new nested JAR. In order to stay JDK 8 compatible, however, we cannot use that method. Hence, the tests still use the workaround to create empty zip files via JarOutputStream. But at least it works, and we can conditionally enable two more tests using:

@EnabledForJreRange(min = JRE.JAVA_12)

One more test involving URIs for files inside zip files (non-nested ones only!) can be activated as of JDK 9+, because the problem was fixed since then.

The build now passes on JDKs 8 to 21, always executing as many tests as possible.

Relates to #156.

marschall commented 8 months ago

Thank you, I'll have a look.

marschall commented 7 months ago

I cherry picked the ZipFileSystem commit and removed the JDK regression tests. We shouldn't really have JDK regression tests in this project, only test interoperability.

I need a bit more time for the Windows tests. I need to access a Windows machine.

kriegaex commented 7 months ago

I cherry picked the ZipFileSystem commit

Thank you for finding it valuable.

and removed the JDK regression tests. We shouldn't really have JDK regression tests in this project, only test interoperability.

I was just fixing tests existing for 11+ years. Who was I to suggest removing them in my first PR? Actually, my first reaction was similar to yours. But then, I found the tests instructive. They were also contextually related to the other tests, so I found them kind of valuable, even though they tested something outside the confines of this project. I even added a similar test to my own new project. That they did not work on older JDKs, but my Maven plugin has to handle nested ZIPs, the tests even guided me to create a workaround using temp-files for JDKs 8 to 11 and a less invasive one for JDK 12. See here:

Maybe all of those are boring and irrelevant for you, but I want to thank you anyway to sparking the idea to make sure those JDK limitations are handled gracefully on all platforms.

I need a bit more time for the Windows tests. I need to access a Windows machine.

Sure, take your time. If it helps to make you feel better, Windows is my primary development platform, i.e. I test my code there the most during development. I would rather notice tests failing there than on other platforms, even though I have just set up a corresponding CI job to cover a whole matrix of OS, JDK version and Maven version. They are all green for now.

marschall commented 7 months ago

and removed the JDK regression tests. We shouldn't really have JDK regression tests in this project, only test interoperability.

I was just fixing tests existing for 11+ years. Who was I to suggest removing them in my first PR? Actually, my first reaction was similar to yours. But then, I found the tests instructive. They were also contextually related to the other tests, so I found them kind of valuable, even though they tested something outside the confines of this project. I even added a similar test to my own new project. That they did not work on older JDKs, but my Maven plugin has to handle nested ZIPs, the tests even guided me to create a workaround using temp-files for JDKs 8 to 11 and a less invasive one for JDK 12. See here:

Maybe all of those are boring and irrelevant for you, but I want to thank you anyway to sparking the idea to make sure those JDK limitations are handled gracefully on all platforms.

I'm happy to learn it was valuable to you and grateful you had an independent look at code. Much of the project was and continues to be a learning opportunity for me. Nonetheless I think it is now time to remove things that were here from the beginning for learning and experimentation. I understand that these can be important for helping people learn and understand but I don't think this should be focus of this project.

I need a bit more time for the Windows tests. I need to access a Windows machine.

Sure, take your time. If it helps to make you feel better, Windows is my primary development platform, i.e. I test my code there the most during development. I would rather notice tests failing there than on other platforms, even though I have just set up a corresponding CI job to cover a whole matrix of OS, JDK version and Maven version. They are all green for now.

I hate to give you the following answer. I tested locally with a Windows 11 developer VM with developer mode enabled and it "works on my machine". Is your Windows machine a corporate machine? Can you check if you have the Create Symbolic Links privilege enabled?

kriegaex commented 7 months ago

I am running Windows 10 Pro, am a member of the Administrators group and as such have the necessary rights. The tests are still failing, if I remove the Assumptions in those tests. The C drive is also an NTFS drive. I even enabled remote to local and remote to remote symlinks, which are off by default, even if symlinks are otherwise activated.

C:\Users\me> fsutil behavior query symlinkevaluation
Die symbolischen Links für lokal zu lokal sind aktiviert.
Die symbolischen Links für lokal zu remote sind aktiviert.
Die symbolischen Links für remote zu lokal sind aktiviert.
Die symbolischen Links für remote zu remote sind aktiviert.

Sorry for the German console language. I guess, you understand it anyway.

Either way, you cannot expect everyone building your project to run with an admin account and have enabled symlinks, which are a rather esoteric feature on Windows, even though I remember I tried them a few years ago for pure curiosity, also hard links. My recommendation is to either use my test assumptions as they are to enable normal users to build and test project or to find a way to determine which rights might be necessary and if the user has them in a more complex assumption, before running the test.

kriegaex commented 7 months ago

OK, I found out that mklink also needs to be run from an elevated (admin) prompt to succeed. You really cannot expect anyone to run a Maven build or the whole IDE as an administrator. But I can confirm that the tests pass when run as an admin.

kriegaex commented 7 months ago

What do you think about commit 03b4104? This really is gold plating, but I was curious how I could find out if the current user is admin.