Add support for WASI platform #478
Conversation
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.
|
@swift-ci test |
|
@swift-ci test macos |
|
@MaxDesiatov macOS build for corelibs-xctest has been broken for a while unfortunately... |
|
@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. 🙇 |
There was a problem hiding this comment.
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?
CMakeLists.txt
Outdated
| set(USE_SWIFT_CONCURRENCY_WAITER_default ON) | ||
| endif() | ||
|
|
||
| option(USE_SWIFT_CONCURRENCY_WAITER "Use Swift Concurrency-based waiter implementation" "${USE_SWIFT_CONCURRENCY_WAITER_default}") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Cool. It's fine for that to come afterwards, but I would like to see it happen!
| @@ -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.") | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🙏
There was a problem hiding this comment.
Agreed, considering the fine-grained scoping of the locking provided by the queue for these, I think we should be ok in that regard.
|
|
||
| do { | ||
| if skip == nil { | ||
| try testClosure(self) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
I think we can keep the consistency by adding @MainActor on this method.
Yeah, that's a very unfortunate situation to have many branching here due to function-colors. I agree that we can improve it with
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
force-pushed
the
yt/xctest-wasm-upstream
branch
from
d36a85d
to
2a46822
Compare
|
@swift-ci test |
|
@swift-ci test |
|
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. |
|
@swift-ci test windows |
|
|
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? |
|
This change might have broken some tests especially if there are many test cases. (I'm sorry if I was wrong.) |
|
@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. |
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,
XCTMainis async runs the given test suites without blocking the thread by bypassing some synchronous public APIs likeXCTest.performandXCTest.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.