Skip to content

Add Async Support to XCTest, Take 2 #331

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

Merged
merged 1 commit into from
Nov 4, 2021

Conversation

SeanROlszewski
Copy link

This PR contains the changes from #326, plus a change which conditionally compiles out the new async setUp/tearDown variants on Windows, since there's a compilation error in XCTest. We plan on removing this limitation once the underlying compilation error is reproduced and resolved in a CI environment.

@SeanROlszewski SeanROlszewski requested review from compnerd and stmontgomery and removed request for compnerd September 28, 2021 16:30
@briancroom
Copy link
Contributor

@swift-ci Please test

@kirilltitov
Copy link

What about tests discovery and LinuxMain autogeneration, by the way?

@MaxDesiatov
Copy link
Contributor

AFAIK test discovery is implemented in SwiftPM and this question may not be relevant to this PR. Additionally, it has been enabled by default since Swift 5.4, and --generate-linuxmain has been deprecated.

@kirilltitov
Copy link

Of course I know, I mean, SPM doesn't know anything about asyncTest wrapper, does it?

@joshuawright11
Copy link

Curious, would this not be available until Swift 5.6? Considering adding some async tests to a library of mine, but plan to wait until this is publicly available.

@kirilltitov
Copy link

@joshuawright11 I managed to run async tests in Linux using some code (in a polyfill form) from this PR. Rather dirty solution, but the alternative is nothing.

@SeanROlszewski
Copy link
Author

SeanROlszewski commented Oct 7, 2021

@kirilltitov Those are our thoughts as well. We recognized this intermediate solution is not ideal by any means. Once we implement the necessary code generation in SPM, then this will be more ergonomic.

@joshuawright11 that's my current understanding. We had originally wanted this to coincide with Swift 5.5 but once work was underway we ran into some unexpected complexity. This made us want to hold off on landing something too quickly in case we didn't get it quite right in time. Specifically, we ran into some issues around preserving the existing threading semantics of setUp and tearDown methods, among a few bugs where tests were unexpectedly executing on the wrong thread.

Which is all to say, it turns out concurrency and parallelism are Hard™ to add into an existing DSL. 🤓

We added some tests which prove the threading invariants were being violated, then resolved the issues over a few iterations, so this PR also introduces some interesting tests which will let us make future changes more reliably.

@joshuawright11
Copy link

Thanks for the update! Decided to go with something similar to what @kirilltitov suggested in the meantime. Looking forward to 5.6 🙂

@0xTim
Copy link
Member

0xTim commented Oct 26, 2021

Is there any reason this can't be added to a Linux patch release?

This reverts commit 6569394.

# Conflicts:
#	Sources/XCTest/Public/XCAbstractTest.swift
@SeanROlszewski SeanROlszewski force-pushed the chefski/add-async-support branch from 1a8871e to 256f54b Compare October 27, 2021 16:13
@SeanROlszewski
Copy link
Author

@swift-ci please test

@briancroom
Copy link
Contributor

Is there any reason this can't be added to a Linux patch release?

Hey @0xTim, we will definitely be considering this for a Linux patch release after it’s landed on main and sticks there 😄 We are still working with @compnerd on sorting out an issue with these changes impacting Windows. Additionally, work will still be needed around SwiftPM’s automatic test discovery code-gen to round out the complete feature.

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

Tested this locally on Windows after swiftlang/swift#39998:

Failed Tests (1):
  SwiftXCTestFunctionalTests :: Asynchronous/Use/main.swift


Testing Time: 12.70s
  Passed: 24
  Failed:  1

I have an idea of how to improve the Asynchronous.Use.main test so it passes most of the time on WIndows. Worst case, we can disable the one test on Windows. But thank you for the support on this, it allows people to continue to be able to use XCTest uniformly on all the platforms.

The workaround that I have in mind would flush the output stream on exit. Since the issue is more about synchronising the output, I'm less concerned about that. We can address that subsequently after the feature is merged. I don't think it is valuable to keep the feature waiting for the ideal state.

@SeanROlszewski SeanROlszewski merged commit 1b0e65c into main Nov 4, 2021
@tomerd
Copy link

tomerd commented Nov 4, 2021

🥳

@ktoso
Copy link

ktoso commented Nov 8, 2021

Wohoo!

stmontgomery added a commit to stmontgomery/swift-corelibs-xctest that referenced this pull request Sep 30, 2022
After async test support was added to this version of XCTest in swiftlang#331, the Swift async/await feature became available in older Apple OSes, and on macOS specifically it changed from requiring 12.0 to 10.15. So this lowers the availability of all macOS 12.0 requirements of async APIs to 10.15.
@compnerd compnerd deleted the chefski/add-async-support branch July 7, 2023 17:53
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.

10 participants