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

Enable all tests on MS-Windows and run them during CI #62

Merged
merged 2 commits into from
Oct 10, 2022

Conversation

ikappaki
Copy link
Contributor

@ikappaki ikappaki commented Oct 10, 2022

Hi,

could you please consider patch to enable the two missing tests under windows and run all windows ext tests during CI. This is part of #61.

I took the library to change the whitespace-test/jvm opts test to test for outer level whitespaces correctness, e.g. -Dfoo=foo bar then check for foo="foo bar", instead of the very elusive quoted space -Dfoo="foo bar" and then check for foo="\"foo bar"\". The test seem to check for the basic white space case and does not require the extra complicated case with inner quotes, but I can as well be very wrong. I could not think of a general case where the double would be required in passing jvm options. It gets extremely tricky to be able to have Java to accept embedded spaces on windows.

It turns out that to take inner space quoting in the main opts, other than being more complicated on windows, has changed in Java 17, and thus requires extra escaping as reflected in the updated test.

Other than that, I used URIs for the temp files because the path separators \ on windows, are taken as escape characters in the strings.

I have also started migrated the script to bb.edn, let me know if you would like me to include that in this PR (if accepted) or create another one.

Thanks

src/borkdude/deps.clj Outdated Show resolved Hide resolved
@borkdude borkdude merged commit 55e8fda into borkdude:master Oct 10, 2022
@borkdude
Copy link
Owner

Thanks a lot Adding bb.edn sounds good

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