Skip to content

Add support for correctly discovering async test methods on non-Apple platforms #3844

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 3 commits into from
Nov 10, 2021

Conversation

briancroom
Copy link
Contributor

This adopts a recent enhancement to swift-corelibs-xctest for supporting async test methods by ensuring the automatic test discovery machinery used on non-Apple platforms generates the correct test manifest code for the harness to run async tests.

Modifications:

I have adjusted the code-gen logic used during test discovery to notice async test methods, using the richer IndexStore data provided by swiftlang/swift-tools-support-core#258, and wrap such methods in the asyncTest helper from swift-corelibs-xctest so that they can execute correctly.

Result:

With this change, it is now possible to transparently run async XCTests on Linux via swift test.

Copy link
Contributor

@neonichu neonichu left a comment

Choose a reason for hiding this comment

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

Thanks, Brian!

@neonichu
Copy link
Contributor

neonichu commented Nov 9, 2021

swiftlang/swift-tools-support-core#258
@swift-ci please smoke test

@briancroom
Copy link
Contributor Author

Note that I haven't seen the new test pass on Linux yet. I tried using one of the "nightly" Docker images, but they're about a week out of date right now and don't have the swift-corelibs-xctest change in place yet. We'll see how CI goes.

Copy link
Contributor

@stmontgomery stmontgomery left a comment

Choose a reason for hiding this comment

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

Looks great! I left a comment on the TSC PR which may impact this, but the changes here all look good to me

class AsyncTests: XCTestCase {

func testAsync() async {
XCTAssertFalse(Thread.isMainThread)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we confident that an async, non-@MainActor func is guaranteed to never run on the main thread, even by coincidence? I bet that is true currently and perhaps always will be, although I feel like semantically there is no particular guarantee there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's fair. The assertion isn't really providing significant value anyway, so I'll just remove it.

@neonichu
Copy link
Contributor

neonichu commented Nov 9, 2021

swiftlang/swift-tools-support-core#258
@swift-ci please smoke test

@briancroom
Copy link
Contributor Author

swiftlang/swift-tools-support-core#258
@swift-ci please smoke test

@briancroom
Copy link
Contributor Author

@swift-ci please smoke test

@briancroom
Copy link
Contributor Author

It looks like the “Linux (self hosted)” CI job uses the latest pre-built trunk development toolchain, which unfortunately is a week out of date right now, and is missing the swift-corelibs-xctest changes needed for the integration test to succeed, so we’re effectively blocked on landing this until a newer nightly toolchain can get posted (cc @shahmishal, who I spoke to about this earlier.)

@neonichu
Copy link
Contributor

The self-hosted jobs do not gate merging.

@briancroom
Copy link
Contributor Author

The self-hosted jobs do not gate merging.

Ah ok, good to know. So you wouldn’t be concerned if this goes in before the toolchain updates to allow that to pass? (Assuming the other, required, Linux job does indeed pass now?)

@neonichu
Copy link
Contributor

So you wouldn’t be concerned if this goes in before the toolchain updates to allow that to pass?

Nope, the main reason for the self hosted jobs is to have quicker iteration time than the smoke tests which take hours, they're not really testing anything in addition to the smoke tests. So I think this is good to merge.

@briancroom
Copy link
Contributor Author

Excellent! The primary Linux job passed, and I confirmed in the logs that it indeed ran the new test. That gives me enough confidence to proceed with landing this. Thanks everybody! 🎉

@briancroom briancroom merged commit f7f7836 into main Nov 10, 2021
@tomerd
Copy link
Contributor

tomerd commented Nov 10, 2021

thanks @briancroom

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.

4 participants