Exlll / ConfigLib

A Minecraft library for saving, loading, updating, and commenting YAML configuration files
MIT License
135 stars 17 forks source link

Window Test Support #25

Closed dhugo0022 closed 11 months ago

dhugo0022 commented 11 months ago

Recently, I was trying to create a plugin and I needed a great and concise configuration library. Well, I found this one, but I found myself in a scenario that I led me to the necessity of implementing a feature that would help me use the said library in my plugin.

I forked the project and download the source on my IDE. But, when I was doing the setup and running tests, they kept failing. The motive? The way the paths were declared in the tests. See, Jimfs doesn't support either absolutes root paths nor relatives root paths on Windows. Reference:

So, in order to test the project after implementing my feature, I had to implement support for tests on Windows.

I tried to change the less I could in the code, so if, in the future, there's a way to implement tests using absolute paths on Windows, we would be able to easily remove the helper methods and use the previous implementation.

dhugo0022 commented 11 months ago

Addition, I haven't tested the commits on an Unix based machine. So, if someone could do so, I would greatly appreciate it. But, on Windows, all the tests passed flawlessly.

Exlll commented 11 months ago

Hey, thanks for your contribution!

What is not clear to me is what caused your tests to fail. Did you try to execute the existing tests (that is, the ones written by me and which only use the UNIX format for paths) and the exuction of these tests failed OR did you add new tests that use JimFS with the Windows format, and only the execution of the newly added tests failed?

If the latter is the case (which I assume, because otherwise JimFS would be broken on Windows), I don't really see a reason to complicate the tests: JimFS only keeps files in memory and never creates them on disc, so the formatting of the paths should play no role when writing tests.

dhugo0022 commented 11 months ago

Hey, thanks for your contribution!

What is not clear to me is what caused your tests to fail. Did you try to execute the existing tests (that is, the ones written by me and which only use the UNIX format for paths) and the exuction of these tests failed OR did you add new tests that use JimFS with the Windows format, and only the execution of the newly added tests failed?

If the latter is the case (which I assume, because otherwise JimFS would be broken on Windows), I don't really see a reason to complicate the tests: JimFS only keeps files in memory and never creates them on disc, so the formatting of the paths should play no role when writing tests.

I didn't add any new tests or changed the original declaration of the existing ones (except when there were unnecessary declarations of the same variable multiple times or when it was unsupported by the system). The helper methods simply convert, if necessary, the Unix path declaration to the Windows ones that Jimfs supports. Plus, the newline character in the file names isn't supported on Windows anymore, so I had to replace them with dummy regular tests.

I ran the existing tests multiples times for hours until I found a solution to fix them. All the tests that I had to use the helper methods were chronically failing on my Windows machine because of the way the paths were described.

I know I implemented a hacky way to fix this problem, but I didn't want to change much. As I stated in the comments of one of the class I changed, I wanted to do it in a way that if someone found a more succinct solution, then this cleaner version should be used instead of mine.

Exlll commented 11 months ago

I think I understand where the problem comes from, although I don't have a Windows machine to verify my assumption right now.

It should not be primarily caused by JimFS because passing com.google.common.jimfs.Configuration.unix() to all Jimfs.newFileSystem() calls should tell JimFS to only use the Unix format.

However, I assume that calls like new File and Path.of generate file resp. path objects that, when converted to a string, contain the Windows file separator instead of the Unix one. I'm not sure why newlines cause problems as well, but I'll test that once I get on a Windows machine.

Other than my comments above, your code looks good to me! :+1:

dhugo0022 commented 11 months ago

I think I understand where the problem comes from, although I don't have a Windows machine to verify my assumption right now.

It should not be primarily caused by JimFS because passing com.google.common.jimfs.Configuration.unix() to all Jimfs.newFileSystem() calls should tell JimFS to only use the Unix format.

However, I assume that calls like new File and Path.of generate file resp. path objects that, when converted to a string, contain the Windows file separator instead of the Unix one. I'm not sure why newlines cause problems as well, but I'll test that once I get on a Windows machine.

Other than my comments above, your code looks good to me! 👍

Yeah, I also don't know why the newline character isn't supported. Well, not by user direct manipulation at least... I tried to create a file with the newline character on my Windows 11 machine it wouldn't let me.

Exlll commented 11 months ago

Alright, I squashed and merged your PR. Thanks for your work! :slightly_smiling_face:

If you'd like any other features added to this library, let me know!