Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Window Test Support #25

Merged
merged 6 commits into from
Nov 3, 2023
Merged

Window Test Support #25

merged 6 commits into from
Nov 3, 2023

Conversation

dhugo0022
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Owner

Exlll commented Nov 2, 2023

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
Copy link
Contributor Author

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
Copy link
Owner

Exlll commented Nov 3, 2023

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! 👍

@dhugo0022
Copy link
Contributor Author

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 Exlll merged commit dc010a0 into Exlll:master Nov 3, 2023
1 check passed
@Exlll
Copy link
Owner

Exlll commented Nov 3, 2023

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants