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.

Sign up for GitHub

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

Jump to bottom

[REVIEW] Add Async Support to XCTest #326

Merged
merged 19 commits into from
Sep 23, 2021
Merged

[REVIEW] Add Async Support to XCTest #326

merged 19 commits into from
Sep 23, 2021

Conversation

SeanROlszewski
Copy link
Contributor

@SeanROlszewski SeanROlszewski commented Aug 4, 2021 •
edited
Loading

This is a work-in-progress pull request that @stmontgomery and I have been working on which adds first-class async/await support to the cross-platform XCTest.

Please withhold comments until this PR changes from [WIP] to [REVIEW].

17/08/21 Update: This PR has now been opened for review and is being actively iterated on.

@SeanROlszewski
Copy link
Contributor Author

@swift-ci Please test

@stmontgomery stmontgomery self-requested a review August 5, 2021 00:00
@SeanROlszewski SeanROlszewski marked this pull request as draft August 10, 2021 18:45
stmontgomery
stmontgomery reviewed Aug 10, 2021
View reviewed changes
Sources/XCTest/Public/XCTestCase.swift Outdated Show resolved Hide resolved
Sources/XCTest/Public/XCTestCase.swift Outdated Show resolved Hide resolved
Sources/XCTest/Public/XCTestCase.swift Outdated Show resolved Hide resolved
Sources/XCTest/Public/XCTestCase.swift Outdated Show resolved Hide resolved
Sources/XCTest/Public/XCTestCase.swift Outdated Show resolved Hide resolved
SeanROlszewski
SeanROlszewski commented Aug 10, 2021
View reviewed changes
.gitignore Outdated Show resolved Hide resolved
@SeanROlszewski
Copy link
Contributor Author

  • Rename ClosureType -> XCTestMethodConvention
  • Potentially make an XCTFI equivalent

@SeanROlszewski
Copy link
Contributor Author

@swift-ci please test 😇

briancroom
briancroom reviewed Aug 17, 2021
View reviewed changes
Copy link
Contributor

@briancroom briancroom left a comment

Choose a reason for hiding this comment

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

Great progress here! Most of my comments are on the theme of tidying up what we’ve got.

Sources/XCTest/Public/XCTestMain.swift Outdated Show resolved Hide resolved
Sources/XCTest/Public/XCTestMain.swift Outdated Show resolved Hide resolved
Sources/XCTest/Public/XCTestMain.swift Outdated Show resolved Hide resolved
Sources/XCTest/Public/XCTestCase.swift Outdated Show resolved Hide resolved
Sources/XCTest/Public/Factory_Methods.swift Outdated Show resolved Hide resolved
Sources/XCTest/Public/XCTestCase.swift Outdated Show resolved Hide resolved
Sources/XCTest/Public/XCTestCase.swift Outdated Show resolved Hide resolved
Sources/XCTest/Public/XCTestCase.swift Outdated Show resolved Hide resolved
Sources/XCTest/Public/XCTestCase.swift Outdated Show resolved Hide resolved
@SeanROlszewski SeanROlszewski changed the title [WIP] Add Async Support to XCTest [REVIEW] Add Async Support to XCTest Aug 17, 2021
@SeanROlszewski SeanROlszewski marked this pull request as ready for review August 17, 2021 18:08
@SeanROlszewski
Copy link
Contributor Author

@swift-ci please test ❤️

@millenomi
Copy link

This work is now blocking landing swiftlang/swift-corelibs-foundation#3036

@millenomi
Copy link

(because it has asynchronous tests.)

briancroom
briancroom reviewed Sep 10, 2021
View reviewed changes
Copy link
Contributor

@briancroom briancroom left a comment

Choose a reason for hiding this comment

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

Thanks for all the iteration on this, I think it's looking good! Let's see what further work might be needed to get CI into good shape, particularly around the macOS deployment target?

Sources/XCTest/Public/XCTestCase.swift
@@ -194,26 +195,24 @@ open class XCTestCase: XCTest {
/// Teardown method called after the invocation of every test method in the
/// class.
open class func tearDown() {}

private var teardownBlocks: [() -> Void] = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice to have better encapsulation of this now!

Sources/XCTest/Public/XCTestCase.swift
@@ -225,33 +224,61 @@ open class XCTestCase: XCTest {
}
}

setUp()
}
do {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is our behavior here matching Xcode XCTest, in terms of what additional steps of the test we still run and what we report if an async setUp override:

  • throws XCTSkip?
  • throws some other error?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe it should be handling these correctly, due to the following two guards above in handleErrorDuringSetUp(_:):

if error.xct_shouldRecordAsTestFailure { and
if error.xct_shouldSkipTestInvocation {

@briancroom
Copy link
Contributor

@swift-ci please test

@briancroom
Copy link
Contributor

The Linux build errored with:

/home/buildnode/jenkins/workspace/swift-corelibs-xctest-PR-Linux/swift-corelibs-xctest/Sources/XCTest/Public/XCTestCase.swift:38:39: error: cannot find 'TeardownBlocksState' in scope
11:10:59     private let teardownBlocksState = TeardownBlocksState()

I had forgotten that for platforms building with CMake, we need to add a reference to any new source files to the CMakeLists.txt file in the root of the repository.

@briancroom
Copy link
Contributor

@swift-ci please test

@stmontgomery
Copy link
Contributor

@swift-ci please test

@SeanROlszewski
Add local lit config to temporarily disable test on the macOS in CI
ff0974c 
* This is due to the latest macOS SDK being unavailable at the moment.
@SeanROlszewski
Copy link
Contributor Author

@swift-ci please test ty <3

@SeanROlszewski
Copy link
Contributor Author

@swift-ci please test ty <3

@SeanROlszewski
Copy link
Contributor Author

@swift-ci please test thank yoooouu <3

@SeanROlszewski
Copy link
Contributor Author

@swift-ci please test

1 similar comment
@briancroom
Copy link
Contributor

@swift-ci please test

@briancroom
Copy link
Contributor

@swift-ci please test

@SeanROlszewski SeanROlszewski merged commit 38f9fa1 into main Sep 23, 2021
@compnerd
Copy link
Member

This seems to have regressed the Windows builds :-(
https://dev.azure.com/compnerd/3133d6ab-80a8-4996-ac4f-03df25cd3224/_apis/build/builds/54170/logs/148

@compnerd compnerd mentioned this pull request Sep 26, 2021
Merged
@finagolfin finagolfin mentioned this pull request Sep 27, 2021
Closed
@SeanROlszewski SeanROlszewski mentioned this pull request Sep 28, 2021
Merged
@joshuawright11 joshuawright11 mentioned this pull request Sep 28, 2021
Merged
@adam-fowler adam-fowler mentioned this pull request Sep 24, 2021
Closed
@briancroom briancroom mentioned this pull request Mar 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@briancroom briancroom briancroom left review comments

@stmontgomery stmontgomery stmontgomery approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@SeanROlszewski @millenomi @briancroom @stmontgomery @compnerd