Skip to content

Tests: Convert RegistryClientTests to Swift Testing #8640

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

plemarquand
Copy link
Contributor

Continue the test conversion to Swift Testing

  • Tests/PackageRegistryTests/RegistryClientTests.swift

Continue the test conversion to Swift Testing

- Tests/PackageRegisttryTests/RegistryClientTests.swift
@plemarquand plemarquand force-pushed the registry-client-tests-to-swift-testing branch from 2a9db12 to f3ef031 Compare May 8, 2025 18:28
@plemarquand
Copy link
Contributor Author

@swift-ci test

bkhouri
bkhouri previously requested changes May 8, 2025
Copy link
Contributor

@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.

Thanks for converting tests to Swift Testing. It's appreciated getting assistance with these.

I noticed some test are calling testDiagnostics(...), which has a call to XCFail. We should use expectDiagnostics(...) instead to avoid a test using a mix of Swift Test and XCTest calls.

@@ -1497,18 +1405,12 @@ final class RegistryClientTests: XCTestCase {
customToolsVersion: .v5_3
)
let parsedToolsVersion = try ToolsVersionParser.parse(utf8String: manifest)
XCTAssertEqual(parsedToolsVersion, .v5_3)
#expect(parsedToolsVersion == .v5_3)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (non-blocking): this do block, and the one before, are very similar and can be converted to a parameterized tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

)
let parsedToolsVersion = try ToolsVersionParser.parse(utf8String: manifest)
XCTAssertEqual(parsedToolsVersion, .v5_3)
#expect(parsedToolsVersion == toolsVersion ?? .current)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (non-blocking): In order to be more explicit, the value of the expected passed tools version can be passed in as the test data, instead of using `toolsVersion ?? .current)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This exercises that "nil" is interpreted as .current, so was deliberate

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the test intent, but the test can look cleaner as a parameterized test.

e.g.

@Test(
    arguments: [
        (toolsVersion: ToolsVersion.v5_3, expectedParsedVersion: ToolsVersion.v5_3),
        (toolsVersion: nil, expectedParsedVersion: ToolsVersion.current),
    ]
)
func mytest(toolsVersion: ToolsVersion?, expectedParsedVersion: ToolsVersion) async throws {
    ...
    #expect(parsedToolsVersion == expectedParsedVersion)
}

@plemarquand
Copy link
Contributor Author

@swift-ci test

@plemarquand
Copy link
Contributor Author

@swift-ci test windows

@plemarquand
Copy link
Contributor Author

@swift-ci please test windows

@plemarquand plemarquand requested a review from bkhouri May 9, 2025 13:05
@bkhouri bkhouri dismissed their stale review May 14, 2025 22:52

the blocking comment was address.

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.

2 participants