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.

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

Convert some tests BasicsTests to Swift Testing #8093

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bkhouri
Copy link
Contributor

@bkhouri bkhouri commented Nov 1, 2024

Convert some BasicsTests from XCTest to Swift Testing to make use of parallelism and, in some cases, test parameterization

Motivation:

The XCTest run, by default, sequentially. Convert most of the WorkspaceTests from XCTests to Swift Testing to make use of parallelism, and in some cases, parameterized test cases.

Not all Test Suites in BasicsTests have been converted as some use helpers in swift-tools-core-support, which don't have a matching swift testing helper.

Modifications:

Convert XCTests to Swift Testing

Result:

Ran the equivalent of

for _ in $(seq 0 100); 
do
    swift test --enable-swift-testing --disable-xctest
done

and ensured there were no test-related failures.

Blocked by #8137
Requires swiftlang/swift#78300

public func isRunninginCI(file: StaticString = #filePath, line: UInt = #line) -> Bool {
return (ProcessInfo.processInfo.environment["SWIFTCI_USE_LOCAL_DEPS"] != nil || ProcessInfo.processInfo.environment["CI"] != nil)
}

public func XCTSkipIfCI(file: StaticString = #filePath, line: UInt = #line) throws {
Copy link
Contributor

Choose a reason for hiding this comment

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

The idiomatic way to do this with SwiftTesting would be a trait, something like this:

extension Trait where Self == Testing.ConditionTrait {
    static var skipInCI: Self {
        disabled("the test is being run on CI", ProcessInfo.processInfo.environment["SWIFTCI_USE_LOCAL_DEPS"] != nil || ProcessInfo.processInfo.environment["CI"] != nil)
    }
}

@bkhouri bkhouri changed the title [DRAFT] Convert some tests BasicsTests to Swift Testing Convert some tests BasicsTests to Swift Testing Nov 7, 2024
@bkhouri bkhouri marked this pull request as ready for review November 7, 2024 13:13
@bkhouri
Copy link
Contributor Author

bkhouri commented Nov 7, 2024

Ready for review, though it may be blocked as not all pipeline builds run with Swift 6.0!

@bkhouri
Copy link
Contributor Author

bkhouri commented Nov 8, 2024

Ready for review, though it may be blocked as not all pipeline builds run with Swift 6.0!

Looks like the Selft-hosted macOS pipeline is now running the nightly build
https://ci.swift.org/job/swift-package-manager-with-xcode-self-hosted-PR-osx/4511/execution/node/49/log/

)
struct AsyncProcessTests {
@Test
func basics() throws {
do {
let process = AsyncProcess(args: "echo", "hello")
try process.launch()
let result = try process.waitUntilExit()
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is presumably actually blocking the thread, which is dangerous in Swift Testing because tests run on the shared thread pool. Consider switching over to an asynchronous suspension.


// Check that there's no error if we try to create the directory again.
try! makeDirectories(dirPath)
#expect(throws: Never.self) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just throw the error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, I want to explicitly indicate a test intentions is to not have an exception raised. Otherwise, future test authors might be include to catch and handle the error!

Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably never catch thrown errors in tests unless the test is itself testing that an error should be thrown. In general, allow the error to propagate out so that the testing library (XCTest or Swift Testing) can gather optimal metadata for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand, but how do we handle the case where we explicitly want to validate that an error did not occur?

@bkhouri bkhouri force-pushed the t/main/convert_from_xctests_to_swift-testing branch from 8ad2798 to c3e455d Compare November 12, 2024 19:21
Copy link
Contributor Author

@bkhouri bkhouri left a comment

Choose a reason for hiding this comment

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

I updated the PR, but I have a few more comments to address. This update will allow me to mark some comments are resolved, ensuring I don't miss a comment.

Tests/BasicsTests/DictionaryTest.swift Outdated Show resolved Hide resolved
Tests/BasicsTests/HTTPClientTests.swift Outdated Show resolved Hide resolved
Tests/BasicsTests/HTTPClientTests.swift Outdated Show resolved Hide resolved
Tests/BasicsTests/HTTPClientTests.swift Outdated Show resolved Hide resolved
Tests/BasicsTests/HTTPClientTests.swift Outdated Show resolved Hide resolved
Tests/BasicsTests/NetrcTests.swift Outdated Show resolved Hide resolved
Tests/BasicsTests/NetrcTests.swift Outdated Show resolved Hide resolved
Tests/BasicsTests/NetrcTests.swift Outdated Show resolved Hide resolved
Tests/BasicsTests/TripleTests.swift Outdated Show resolved Hide resolved
Tests/BasicsTests/TripleTests.swift Outdated Show resolved Hide resolved
@bkhouri bkhouri force-pushed the t/main/convert_from_xctests_to_swift-testing branch 2 times, most recently from 6f9d5a0 to abfba54 Compare November 13, 2024 04:14
@bkhouri bkhouri marked this pull request as draft November 14, 2024 17:55
@bkhouri bkhouri force-pushed the t/main/convert_from_xctests_to_swift-testing branch 3 times, most recently from b29877c to 67bad81 Compare November 22, 2024 01:44
@bkhouri
Copy link
Contributor Author

bkhouri commented Nov 22, 2024

This PR is blocked by #8137

@bkhouri bkhouri force-pushed the t/main/convert_from_xctests_to_swift-testing branch from 67bad81 to 9bb616d Compare December 3, 2024 18:02
@bkhouri
Copy link
Contributor Author

bkhouri commented Dec 19, 2024

@swift-ci please test

@bkhouri bkhouri marked this pull request as ready for review December 19, 2024 01:25
@bkhouri bkhouri added the test suite improvements to SwiftPM test suite label Dec 20, 2024
@bkhouri
Copy link
Contributor Author

bkhouri commented Jan 15, 2025

@swift-ci please test

@bkhouri bkhouri force-pushed the t/main/convert_from_xctests_to_swift-testing branch from 9bb616d to 7eebb04 Compare January 15, 2025 16:59
@bkhouri
Copy link
Contributor Author

bkhouri commented Jan 15, 2025

@swift-ci please test

@bkhouri bkhouri force-pushed the t/main/convert_from_xctests_to_swift-testing branch from 7eebb04 to 4593699 Compare January 15, 2025 19:04
@bkhouri
Copy link
Contributor Author

bkhouri commented Jan 15, 2025

@swift-ci please test

bkhouri added a commit that referenced this pull request Jan 16, 2025
Add a canary swift testing tests to ensure we do not regress.

This test will be removed sometime after #8092, #8093 or #8100 are
merged
Convert some BasicsTests from XCTest to Swift Testing to make use of
parallelism and, in some cases, test parameterization.

Not all Test Suites in BasicsTests have been converted as some use helpers
in swift-tools-core-support, which don't have a matching swift testing
helper.
@bkhouri bkhouri force-pushed the t/main/convert_from_xctests_to_swift-testing branch from 4593699 to d252933 Compare January 17, 2025 06:09
@bkhouri
Copy link
Contributor Author

bkhouri commented Jan 17, 2025

@swift-ci please test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test suite improvements to SwiftPM test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants