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

javalib WindowsPath fixes + unit tests #3299

Merged
merged 7 commits into from Jun 1, 2023

Conversation

jpsacha
Copy link
Contributor

@jpsacha jpsacha commented May 31, 2023

I added unit tests for javalib WindowsPath, fixed #relativize (#3293 and other issues), and couple of other errors in WindowsPath that were discovered with new the unit tests.

The unit tests are based on UnixPathTest with additions for drive letters on Windows. The assertions were tested by running tests on Java VM Windows.

While number of issues were fixed, there are a couple remaining. Unit tests for those issues are commended out and marked with TODO, to help with future work. I am running out of time I can spent on this currently, but WindowsPath should be in a better shape than it was before.

… a root for DriveRelative and DirectoryRelative paths
Tests are based on UnixPathTest with modifications and additions to
for Windows paths.
Tests were verified by running them vs JVM on Windows.
Some additional tests are commented out to mark known issues and help
with future development.
@WojciechMazur
Copy link
Contributor

Thank you! So far looks good.
There is one more test that fails on JDK 8, but works with newer JDKs. I've seen you've already disable some of them from running with Java 8, and you would need to adjust this one as well, Let's stick to the semantics of the latest JDK

[error] Test org.scalanative.testsuite.javalib.nio.file.WindowsPathTest.pathRelativize failed: org.junit.ComparisonFailure: expected:<[]foo> but was:<[..\]foo>, took 0.0 sec
[error]     at org.scalanative.testsuite.javalib.nio.file.WindowsPathTest.pathRelativize(WindowsPathTest.scala:314)
[error]     ...

Added IDs, so it is possible to identify which assertion is failing
just from the test output.
@jpsacha
Copy link
Contributor Author

jpsacha commented May 31, 2023

Disabling selected assertions on JVM8 should be working correctly now

Copy link
Contributor

@WojciechMazur WojciechMazur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current work looks good, thank you for this contribution.

I only have question about the remaining TODOs in WindowsPathTest: are you going to resolve them, or they're out of scope of this and you've leaved them only as a markers for future improvements ?

@jpsacha
Copy link
Contributor Author

jpsacha commented May 31, 2023

I will not have time to look at the TODOs in a short term. This PR was primarily to address #3293. I resolved that and addressed a few other issues with WindowsPath in this PR too.

The commented tests marked with TODO identified additional issues with WindowsPath beyond #3293. I left the comments in rather than deleted them to help whoever will be looking at this later. You may want to create a separate issue for that. I may be able to address them in the future, cannot promise specific time. I am working on a project that is using Scala Native: https://github.com/ij-plugins/ijp-imagej-launcher, it drives my time priorities, currently, till I get the main features implemented. That's were #3293 was discovered. If run in some other issue there I will report and help fixing if I can. For instance, I may need #266 or a workaround eventually, but that issue is opened for a long time and I am not clear how it could be addressed.

@WojciechMazur WojciechMazur merged commit ae31fcc into scala-native:main Jun 1, 2023
69 checks passed
WojciechMazur pushed a commit that referenced this pull request Jun 1, 2023
* WindowsPath: Correct detection of the path type and the extraction of a root for DriveRelative and DirectoryRelative paths

* WindowsPath: Correct parent extraction, consider DirectoryRelative

* WindowsPath: Correct path creation in `relativize`

* WindowsPath: Empty path's type should be Relative

* [WIP] `relativize` should throw exceptions on invalid input

* WindowsPath: add unit tests

Tests are based on UnixPathTest with modifications and additions to
for Windows paths.
Tests were verified by running them vs JVM on Windows.
Some additional tests are commented out to mark known issues and help
with future development.

* WindowsPath: better switching of tests on JVM 8; add assertion IDs

Added IDs, so it is possible to identify which assertion is failing
just from the test output.
LeeTibbert pushed a commit to LeeTibbert/scala-native-fork that referenced this pull request Jun 1, 2023
* WindowsPath: Correct detection of the path type and the extraction of a root for DriveRelative and DirectoryRelative paths

* WindowsPath: Correct parent extraction, consider DirectoryRelative

* WindowsPath: Correct path creation in `relativize`

* WindowsPath: Empty path's type should be Relative

* [WIP] `relativize` should throw exceptions on invalid input

* WindowsPath: add unit tests

Tests are based on UnixPathTest with modifications and additions to
for Windows paths.
Tests were verified by running them vs JVM on Windows.
Some additional tests are commented out to mark known issues and help
with future development.

* WindowsPath: better switching of tests on JVM 8; add assertion IDs

Added IDs, so it is possible to identify which assertion is failing
just from the test output.
WojciechMazur pushed a commit that referenced this pull request Jun 5, 2023
* WindowsPath: Correct detection of the path type and the extraction of a root for DriveRelative and DirectoryRelative paths

* WindowsPath: Correct parent extraction, consider DirectoryRelative

* WindowsPath: Correct path creation in `relativize`

* WindowsPath: Empty path's type should be Relative

* [WIP] `relativize` should throw exceptions on invalid input

* WindowsPath: add unit tests

Tests are based on UnixPathTest with modifications and additions to
for Windows paths.
Tests were verified by running them vs JVM on Windows.
Some additional tests are commented out to mark known issues and help
with future development.

* WindowsPath: better switching of tests on JVM 8; add assertion IDs

Added IDs, so it is possible to identify which assertion is failing
just from the test output.
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.

In native code on Windows, java.nio.file.Path#relativize creates invalid paths
2 participants