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

Make reconciler tests build and run on macOS #229

Merged
merged 3 commits into from
Jul 30, 2020
Merged

Conversation

MaxDesiatov
Copy link
Collaborator

We can't run our basic reconciler tests in a WASI environment yet because XCTestExpectation is not available on WASI as it relies on the presence of Dispatch. We can run these tests on macOS though, and even on Linux in the future when Swift 5.3 is available for Linux on GitHub Actions.

My current OpenCombine fork doesn't build on macOS and it was much easier to add a new CombineShim module that uses native Combine there.

@MaxDesiatov MaxDesiatov added continuous integration Changes related to the continuous integration process test suite Changes to the framework's test suite labels Jul 30, 2020
@carson-katri
Copy link
Member

carson-katri commented Jul 30, 2020

I wasn’t able to build the combine fork on Arch Linux either. Maybe use OpenCombine/OpenCombine instead of the fork for Linux? Unless that’s only a problem on Arch, which I don’t know for sure.

It’s the same error I get on macOS.

@MaxDesiatov
Copy link
Collaborator Author

No, I don't think my fork can be built on non-WASI platforms, I had a few things from COpenCombineHelpers torn out manually rushing to make it work on WASI 😅

@MaxDesiatov MaxDesiatov marked this pull request as ready for review July 30, 2020 15:25
@MaxDesiatov MaxDesiatov requested review from carson-katri and j-f1 July 30, 2020 15:25
dependencies: [.product(
name: "OpenCombine",
package: "OpenCombine",
condition: .when(platforms: [.wasi, .linux])
Copy link
Member

Choose a reason for hiding this comment

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

Could you swap out the OpenCombine url in .package on line 34 when on Linux?

Copy link
Collaborator Author

@MaxDesiatov MaxDesiatov Jul 30, 2020

Choose a reason for hiding this comment

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

I don't think so, at least after looking at the proposal. I'll try to submit my changes to the upstream OpenCombine repo to make it work on all platforms including WASI, I just have another old open PR there with the zip implementation that I need to finish first...

Copy link
Member

@j-f1 j-f1 left a comment

Choose a reason for hiding this comment

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

Tests pass after linking the wasm version into ~/.swiftenv/versions and running swift test

@MaxDesiatov
Copy link
Collaborator Author

Yeah, that builds and runs on macOS though unless you pass the --triple wasm32-unknown-wasi option to it. This is what carton test does automatically, but it won't build because of XCTestExpectation unavailable on WASI.

@MaxDesiatov MaxDesiatov merged commit 2c539d9 into main Jul 30, 2020
@MaxDesiatov MaxDesiatov deleted the revive-test-renderer branch July 30, 2020 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
continuous integration Changes related to the continuous integration process test suite Changes to the framework's test suite
Development

Successfully merging this pull request may close these issues.

3 participants