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

Fix code paths that cause errors on Windows #8355

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

tobias-hotz
Copy link
Contributor

Currently, when setting up the project as described on windows, you cannot run unit tests locally, as they assume LF line endings at various places.
Using the WSL on Windows means that these unit tests can be run.
Due to IntelliJ's good integration into the WSL, this works very seamless with IntelliJ as well.

Also include some minor fixes like incorrect links and outdated documentation

Checklist

  • I have read the contribution guidelines
  • Pull request provided for main branch, backports managed with label
  • Good housekeeping of code, cleaning up comments, tests, and documentation
  • Clean commit history broken into understandable chucks, avoiding big commits with hundreds of files, cautious of reformatting and whitespace changes
  • Clean commit messages, longer verbose messages are encouraged
  • API Changes are identified in commit messages
  • Testing provided for features or enhancements using automatic tests
  • User documentation provided for new features or enhancements in manual
  • Build documentation provided for development instructions in README.md files
  • Library management using pom.xml dependency management. Update build documentation with intended library use and library tutorials or documentation

Funded by LGL BW

Using the WSL on Windows means that the unit tests can be run. Due to IntelliJ's good integration into the WSL, this works very seamless with IntelliJ as well
@josegar74 josegar74 added this to the 4.2.11 milestone Sep 9, 2024
@josegar74
Copy link
Member

@tobias-hotz the pull request looks good to me, but would be good to have a list of tests failing in Windows due to the line endings to update the code to use the current system line ending.

@jodygarnett
Copy link
Contributor

Can we just fix the tests please rather than require Linux line feeds? windows is a supported environment is it not? We may as well test for it …

If it helps we can setup a windows GitHub workflow to keep the tests passing correctly …

@ianwallen ianwallen modified the milestones: 4.2.11, 4.4.6 Sep 11, 2024
This fixes some tests, like XslUtilTest and RemoveSourceMapUrlProcessorTest to include the expected line endings
This also adjusts some wro4j classes to work correctly for windows. Previously, some code paths were hardcoded to only work on the D: drive on windows. This fixes all these issues by special casing file:/ URIs, as windows FS will throw an exception when trying to convert those to paths.
Wro4j is also adjusted to take backslashes into account, as windows internally uses these instead of forward slashes for path separation.
Finally, the ES enforcer script is updated to not use project.baseUri, as this causes compilation errors on windows. This is because these variables are filled before compiling, and on windows this results in paths like "C:\Builds\core-geonetwork", which is invalid, because the backslashes need to be escaped. Use the baseUri instead, as this always contains forward slashes.
@tobias-hotz tobias-hotz changed the title Adjust docs to hint that setting up in the WSL may be beneficial Fix code paths that cause errors on Windows Sep 24, 2024
@tobias-hotz
Copy link
Contributor Author

Thanks for your feedback. I've taken a look at the errors more closely, and it turns out there are more errors than just line endings. I've fixed all test failures in the lastest commit, so all unit tests now pass on windows. Please note that this also required some adjustment to non-test classes, as there were some errors/assumptions in wro4j regarding windows handling. Please read the commit message for more details.
With these changes, all tests now pass on windows and linux.

This test currently fails sometimes because of concurrent modifications. Adjust it so it uses proper thread safe constructs
@tobias-hotz
Copy link
Contributor Author

I've also notices that MessageProducerTest sometimes fails. It seems like the test is not written thread-safe. I've fixed this test in the lastest commit to make it thread-safe

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.

4 participants