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

Tests: normalise the fixtures path for Windows #785

Merged
merged 1 commit into from
Jun 23, 2023
Merged

Conversation

compnerd
Copy link
Contributor

Ensure that we normalise the path when we substitute the path into the results to properly remove the basename of the fixture.

@jpsim
Copy link
Owner

jpsim commented Jun 15, 2023

Approach looks ok, I'll merge when you fix the remaining issues causing tests to fail.

@compnerd
Copy link
Contributor Author

@jpsim any idea why this would be failing?

@jpsim
Copy link
Owner

jpsim commented Jun 16, 2023

any idea why this would be failing?

It's adding a trailing / on macOS/Linux which wasn't there before.

This fixes the macOS/Linux tests:

// Strip out fixtures directory since it's dependent on the test machine's setup
let escapedFixturesDirectory = URL(fileURLWithPath: rootDirectory)
    .standardizedFileURL
    .withUnsafeFileSystemRepresentation { String(cString: $0!) }
    .replacingOccurrences(of: #"\"#, with: #"\\"#)
    .replacingOccurrences(of: "/", with: #"\/"#)
let escapedJSONString = jsonString.replacingOccurrences(of: escapedFixturesDirectory, with: "")

Is it ok to have the Windows paths skip adding the trailing path separator?

@compnerd
Copy link
Contributor Author

IIRC it was needed ... but I think that we can go with this for now, and come back around to when we can start running the tests? That will give us a better view of the failure.

@compnerd
Copy link
Contributor Author

-  "Subscript.swift" : {
+  "\/Subscript.swift" : {

This diff makes me think that the trailing suffix is important?

@jpsim
Copy link
Owner

jpsim commented Jun 16, 2023

We can just do a separate codepath for Windows for the time being:

    // Strip out fixtures directory since it's dependent on the test machine's setup
#if os(Windows)
    let escapedFixturesDirectory = URL(fileURLWithPath: rootDirectory)
        .standardizedFileURL
        .withUnsafeFileSystemRepresentation { String(cString: $0!) }
        .replacingOccurrences(of: #"\"#, with: #"\\"#)
        .replacingOccurrences(of: "/", with: #"\/"#)
#else
    let escapedFixturesDirectory = rootDirectory.replacingOccurrences(of: "/", with: #"\/"#)
#endif
    let escapedJSONString = jsonString.replacingOccurrences(of: escapedFixturesDirectory, with: "")

Adjust that for whatever you need on the Windows side.

Ensure that we normalise the path when we substitute the path into the
results to properly remove the basename of the fixture.
@jpsim jpsim merged commit b1f22c8 into jpsim:main Jun 23, 2023
13 checks passed
@compnerd compnerd deleted the fixedture branch June 24, 2023 13:56
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