search

neo4j / graph-data-science

Source code for the Neo4j Graph Data Science library of graph algorithms.
https://neo4j.com/docs/graph-data-science/current/
Other
597 stars 157 forks source link

A few tests fail on Windows #195

Closed yuvalr1 closed 2 years ago

yuvalr1 commented 2 years ago

Describe the bug I'm using Windows 10. I downloaded the OpenGDS library for the first time and running the tests on master.

I'm not sure whether I've done something wrong, trying to do something unsupported, or stumbled into a bug in the tests. I didn't find other issues mentioning this, so I decided to open this issue to have a documentation for the problem I face.

I installed both JDK 11 and 17. I also have JDK 8 installed on my machine. Running ./gradlew :open-packaging:shadowCopy to compile the library works perfectly fine. A 33M jar is being created in the right path. However running ./gradlew check on a clean build without changes fails.

To Reproduce Clone open-gds, install the required JDKs and run the tests on windows.

GDS version: open-gds-2.1.0-alpha03 Operating system: Windows 10

Expected behavior All the tests to pass

Additional context At least some of the tests fail due to CR;LF differences between OSs:

NodeSimilarityTest: shouldGiveCorrectResultsWithOverlap
SccDocTest: stream1, write1, findLargest
ProcedureSyntaxAutoCheckerTest: shouldFailOnMissingYieldResultColumns, shouldFailOnExtraYieldResultColumns, shouldFailOnMissingResultTableRows, shouldFailOnExtraResultTableRows
ConfigKeyValidationTest: returnMultipleErrorsInConfigConstructionAtOnce
GraphProjectProcTest: failsOnBothEmptyProjection
LinkPredictionPipelineAddTrainerMethodProcsTest: failOnInvalidParameterValues
NodeClassificationPipelineAddTrainerMethodProcsTest: failOnInvalidParameterValues
CypherExporterTest: testDumpByGraphDbApi, testDumpByGraphForHuge

One test fails due to path separators differences (slash/backslash): GraphStoreExportProcTest: failsWhenTheExportDirectoryAlreadyExists

And some other tests fail due to reasons I don't yet understand:

UserLogProcTest: userLogOutputMessageOnMultipleTasks, userLogOutputOnlyMostRecentTasks
LoggingOutputStreamTest: shouldLog (this is probably a CR;LF issue as well, but less trivial to fix)

All the other tests pass successfully.

Is this a known issue? In case it's helpful, I can provide a PR that at least fixes the CR;LF issues. Here is a branch that fixes the CR;LF issues for me: https://github.com/yuvalr1/neo-gds/tree/windows-tests-crlf-fix

Mats-SX commented 2 years ago

Hello @yuvalr1 and thanks for reaching out to us!

I'm not surprised that the repository contains some Windows-unfriendly content. None of the developers working on the project frequently use Windows, so the development content is skewed towards Unix. We do have an ambition to do better and be accessible from any platform, so I appreciate very much your report.

If you would like to submit a pull request from your branch, that would be appreciated! We'll review it and help merge the changes. In order to accept your contribution, you need to sign the CLA. (You can submit the PR in advance though.)

yuvalr1 commented 2 years ago

Thanks @Mats-SX! I signed the CLA and submitted a pull request.

Mats-SX commented 2 years ago

Fixed by https://github.com/neo4j/graph-data-science/commit/4cde7e2ab83763363a80ed107ae709d46e246ce9