Add support for WASI platform #478

kateinoigakukun · 2024-03-10T22:46:02Z

WASI does not have thread spawning method yet, but the existing implementation blocks threads to wait async test cases synchronously. This commit introduced a new waiter method for running async test cases in single-threaded WASI environments, enabled by USE_SWIFT_CONCURRENCY_WAITER flag.

With the new waiter, XCTMain is async runs the given test suites without blocking the thread by bypassing some synchronous public APIs like XCTest.perform and XCTest.run. This ignores those APIs even if they are overridden by user-defined subclasses, so it's not 100% compatible with the existing XCTest APIs. This is a trade-off to support async test execution in single-threaded environments, but it should be fine because the APIs are seldom overridden by user code.

WASI does not have thread spawning method yet, but the existing implementation blocks threads to wait async test cases synchronously. This commit introduced a new waiter method for running async test cases in single-threaded WASI environments, enabled by USE_SWIFT_CONCURRENCY_WAITER flag. With the new waiter, `XCTMain` is async runs the given test suites without blocking the thread by bypassing some synchronous public APIs like `XCTest.perform` and `XCTest.run`. This ignores those APIs even if they are overridden by user-defined subclasses, so it's not 100% compatible with the existing XCTest APIs. This is a trade-off to support async test execution in single-threaded environments, but it should be fine because the APIs are seldom overridden by user code.

kateinoigakukun · 2024-03-10T22:52:22Z

@swift-ci test

MaxDesiatov · 2024-03-11T10:32:25Z

@swift-ci test macos

kateinoigakukun · 2024-03-11T10:49:59Z

@MaxDesiatov macOS build for corelibs-xctest has been broken for a while unfortunately...

kateinoigakukun · 2024-03-12T05:38:48Z

@stmontgomery If we can get this merged before the next branch cut, it would be appreciated because this patch is the last difference from the fork. Most of the changes are guarded by a macro and should not affect other platforms. 🙇

briancroom

Thank you for the PR and all the work on this, @kateinoigakukun! (And @MaxDesiatov as well, it sounds like.) I've left a number of questions and comments, some pretty important.

I wish there were a way to achieve this with less branching, but without reasync support in the compiler, I doubt we can do much better.

Besides the details of the code changes in the PR, I'm interested in learning more about the plans for ongoing support and maintenance of this compilation mode and platform. In particular, what kind of CI and testing is in place, or expected to come in the near future?

Sources/XCTest/Public/XCTestSuite.swift

briancroom · 2024-03-12T16:20:01Z

CMakeLists.txt

+  set(USE_SWIFT_CONCURRENCY_WAITER_default ON)
+endif()
+
+option(USE_SWIFT_CONCURRENCY_WAITER "Use Swift Concurrency-based waiter implementation" "${USE_SWIFT_CONCURRENCY_WAITER_default}")


Does this option work on other platforms besides WASM/WASI? I wonder if that would be a path to improving test coverage for this new code, by running the existing lit-based tests on Darwin or Linux, with this flag turned on?

Yes, it works on other platforms, but the test suite doesn't work as is now because the XCTMain is async with the configuration. So we need to adjust those tests, but it's not too hard. Do you want to include those adjustments in this PR?

Cool. It's fine for that to come afterwards, but I would like to see it happen!

briancroom · 2024-03-12T16:24:59Z

Sources/XCTest/Public/Asynchronous/XCTWaiter.swift

@@ -187,9 +189,16 @@ open class XCTWaiter {
    ///   these environments. To ensure compatibility of tests between
    ///   swift-corelibs-xctest and Apple XCTest, it is not recommended to pass
    ///   explicit values for `file` and `line`.
+    #if USE_SWIFT_CONCURRENCY_WAITER
+    @available(*, unavailable, message: "Expectation-based waiting is not available when using the Swift concurrency waiter.")


I'm a little bit confused about the messaging of the USE_SWIFT_CONCURRENCY_WAITER option, the continued availability of the XCTWaiter class, and these unavailability messages about "expectation-based waiting".

Is there anything left that XCTWaiter can do in this mode? Or should the entire class be removed / made unavailable?

I'm a little bit confused about the messaging of the USE_SWIFT_CONCURRENCY_WAITER option

Sorry for your confusion, the option conceptually means that no thread blocking should happen with this mode.

and these unavailability messages about "expectation-based waiting".

I used "expectation" word because XCTestExpectation-related APIs block thread. But "Blocking-wait is unavailable" might be more descriptive?

the continued availability of the XCTWaiter class
Is there anything left that XCTWaiter can do in this mode? Or should the entire class be removed / made unavailable?

There are no functionalities XCTWaiter can do with this mode, but I left the class itself available to avoid spreading #if around all XCTWaiter.subsystemQueue uses.

Do you think it would be workable to adjust the scope and semantics of this conditional a little bit, perhaps calling it DISABLE_XCTWAITER or something along those lines, and have it guard the entirety of the XCTWaiter and XCTestExpectation classes, along with the everything in XCTestCase+Asynchronous.swift, and the expectation-related state tracking in XCTestCase.swift.

I do see that usage of the XCTWaiter.subsystemQueue has leaked out to non-waiter code in a few places (ThrownErrorWrapper and TeardownBlocksState). Those are using it for just basic locking purposes, and quite independently from the actual waiter subsystem. Perhaps we introduce a XCTestCase.subsystemQueue to use for those instead? Or even a different locking solution altogether? This would be a general-purpose change, not conditionalized by the new USE_SWIFT_CONCURRENCY_WAITER/DISABLE_XCTWAITER option.

That totally makes sense to me. I managed to guard out XCTWaiter and XCTestExpectation entirely, and it simplified a lot. A bonus here was I could remove DispatchShim :)

If we introduce a second queue, we should be very wary of deadlocks, data races, etc. where the current code assumes a single global lock.

Right, I used the new queue in TeardownBlocksState and ThrownErrorWrapper. As far as I checked, they are well independent from the other, and they won't acquire the other lock while acquiring itself lock. But I need more eyes here 🙏

Agreed, considering the fine-grained scoping of the locking provided by the queue for these, I think we should be ok in that regard.

Sources/XCTest/Public/XCAbstractTest.swift

Sources/XCTest/Public/XCTestCase.swift

briancroom · 2024-03-12T16:40:14Z

Sources/XCTest/Public/XCTestCase.swift

+
+        do {
+            if skip == nil {
+                try testClosure(self)


It's very important for behavioral consistency in this mode that when calling out to non-async user-provided code (test methods, and setUp and tearDown overrides) that those synchronous functions run on the main actor. I'm not certain, but I don't think that invariant is maintained by the current code. Can you comment on that?

(This was something that we struggled with a bit, and initially got wrong when we were working on support for async test methods a while back in #326)

I think we can keep the consistency by adding @MainActor on this method.

kateinoigakukun · 2024-03-12T18:08:35Z

I wish there were a way to achieve this with less branching, but without reasync support in the compiler, I doubt we can do much better.

Yeah, that's a very unfortunate situation to have many branching here due to function-colors. I agree that we can improve it with reasync support in the future.

Besides the details of the code changes in the PR, I'm interested in learning more about the plans for ongoing support and maintenance of this compilation mode and platform. In particular, what kind of CI and testing is in place, or expected to come in the near future?

Yes, at least I'm going to include XCTest and Foundation build in the WebAssembly build CI after this will get merged: https://ci.swift.org/job/oss-swift-pr-test-crosscompile-wasm-ubuntu-20_04/

Also, as you mentioned, we can build and run tests with this mode on other platforms.

Also this revealed teardown blocks were not being run in the mode, so fix that as well.

To keep consistency with the regular mode.

To make the semantics of the option clearer.

The use of DispatchQueue in XCTest is now very limited, and it's only used in a single place in XCTestCase.swift.

kateinoigakukun · 2024-03-12T21:36:36Z

@swift-ci test

Sources/XCTest/Private/XCTestCase.TearDownBlocksState.swift

kateinoigakukun · 2024-03-13T03:57:46Z

@swift-ci test

briancroom · 2024-03-13T04:18:13Z

Let’s take a good look at CI results, but the diff looks ok to me now! Thanks for discussing and the quick turnaround on the revisions.

MaxDesiatov · 2024-03-13T08:53:24Z

@swift-ci test windows

kateinoigakukun · 2024-03-13T09:37:49Z

Windows CI has passed but just not reported on GitHub UI: https://ci-external.swift.org/job/swift-corelibs-xctest-PR-windows/90/
macOS CI test failure is a known issue. At least, the build is not broken.

kateinoigakukun · 2024-03-13T18:18:02Z

Thanks a lot for your review @briancroom! I assume we need @shahmishal's help to get this merged by temporarily bypassing PR status check, right?

kkebo · 2024-03-25T16:37:21Z

This change might have broken some tests especially if there are many test cases. (I'm sorry if I was wrong.)

kateinoigakukun · 2024-03-25T16:55:29Z

@KKK669 Yes, the implementation is changed from the version used in the downstream, so some of large async test cases might not work. It will be mitigated after tail-call support.

kateinoigakukun requested a review from MaxDesiatov March 10, 2024 22:46

kateinoigakukun mentioned this pull request Mar 10, 2024

Fix XCTest entrypoint for WASI by making it async swiftlang/swift-package-manager#7400

Merged

MaxDesiatov requested review from stmontgomery and grynspan March 11, 2024 07:56

MaxDesiatov approved these changes Mar 11, 2024

View reviewed changes

MaxDesiatov added the WebAssembly Wasm and WASI support label Mar 11, 2024

briancroom self-requested a review March 12, 2024 16:13

briancroom requested changes Mar 12, 2024

View reviewed changes

kateinoigakukun added 6 commits March 12, 2024 18:09

Guard out awaitUsingExpectation for SWIFT_CONCURRENCY_WAITER mode

ae2cb67

Also this revealed teardown blocks were not being run in the mode, so fix that as well.

Guard out SWIFT_CONCURRENCY_WAITER specific member field

e428651

Run test suite, setUp, and tearDown in the main actor

99a5c70

To keep consistency with the regular mode.

Rename USE_SWIFT_CONCURRENCY_WAITER to DISABLE_XCTWAITER

c63a84f

To make the semantics of the option clearer.

Make entire XCTWaiter and XCTestExpectation unavailable for WASI

9decbc4

Remove DispatchShims.swift and replace it with a conditional typealias

2a46822

The use of DispatchQueue in XCTest is now very limited, and it's only used in a single place in XCTestCase.swift.

kateinoigakukun force-pushed the yt/xctest-wasm-upstream branch from d36a85d to 2a46822 Compare March 12, 2024 20:52

briancroom reviewed Mar 12, 2024

View reviewed changes

Sources/XCTest/Private/XCTestCase.TearDownBlocksState.swift Show resolved Hide resolved

Keep running tearDown blocks on the main actor consistently

da82bb9

briancroom approved these changes Mar 13, 2024

View reviewed changes

MaxDesiatov changed the title ~~WebAssembly Support~~ Add support for WASI platform Mar 13, 2024

shahmishal merged commit e0c3868 into swiftlang:main Mar 13, 2024

Add support for WASI platform #478

Add support for WASI platform #478

Uh oh!

Conversation

kateinoigakukun commented Mar 10, 2024

Uh oh!

kateinoigakukun commented Mar 10, 2024

Uh oh!

MaxDesiatov commented Mar 11, 2024

Uh oh!

kateinoigakukun commented Mar 11, 2024

Uh oh!

kateinoigakukun commented Mar 12, 2024

Uh oh!

briancroom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kateinoigakukun commented Mar 12, 2024

Uh oh!

kateinoigakukun commented Mar 12, 2024

Uh oh!

Uh oh!

kateinoigakukun commented Mar 13, 2024

Uh oh!

briancroom commented Mar 13, 2024

Uh oh!

MaxDesiatov commented Mar 13, 2024

Uh oh!

kateinoigakukun commented Mar 13, 2024

Uh oh!

kateinoigakukun commented Mar 13, 2024

Uh oh!

kkebo commented Mar 25, 2024

Uh oh!

kateinoigakukun commented Mar 25, 2024

Uh oh!

Uh oh!