Skip to content

Conversation

plemarquand
Copy link
Contributor

Now that #9032 has landed, take the work from #8995 to convert the InitTests to Swift Testing and break it out in to its own PR.

@bripeticca
Copy link
Contributor

@swift-ci please test windows

///
/// - Parameters:
/// - path: The absolute path to check for file existence.
/// - sourceLocation: The source location where the expectation is made.
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: thank you for adding documentation to these files! this makes it a lot more readable.

import Testing
import SPMBuildCore

/// Tests for the `InitPackage` functionality, which creates new Swift packages with different configurations.
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: another thank you for the comments/documentation here!

@plemarquand plemarquand force-pushed the init-tests-to-swift-testing branch from 4a43f2f to ce0cf11 Compare September 12, 2025 00:04
@plemarquand plemarquand force-pushed the init-tests-to-swift-testing branch from 196f5e1 to 3c6643d Compare September 22, 2025 13:56
@plemarquand
Copy link
Contributor Author

@swift-ci test

@plemarquand
Copy link
Contributor Author

@swift-ci test windows

@plemarquand plemarquand force-pushed the init-tests-to-swift-testing branch from 3c6643d to 5264211 Compare September 22, 2025 18:49
@plemarquand plemarquand force-pushed the init-tests-to-swift-testing branch from 5264211 to 9f2a73e Compare September 22, 2025 18:59
@plemarquand
Copy link
Contributor Author

@swift-ci test

@plemarquand
Copy link
Contributor Author

@swift-ci test windows

4 similar comments
@plemarquand
Copy link
Contributor Author

@swift-ci test windows

@plemarquand
Copy link
Contributor Author

@swift-ci test windows

@plemarquand
Copy link
Contributor Author

@swift-ci test windows

@plemarquand
Copy link
Contributor Author

@swift-ci test windows

@plemarquand
Copy link
Contributor Author

@swift-ci test

@plemarquand
Copy link
Contributor Author

@swift-ci test windows

@plemarquand
Copy link
Contributor Author

@swift-ci test

@plemarquand
Copy link
Contributor Author

@swift-ci test windows

1 similar comment
@plemarquand
Copy link
Contributor Author

@swift-ci test windows

@plemarquand plemarquand force-pushed the init-tests-to-swift-testing branch from 326df82 to 4a3f755 Compare October 22, 2025 19:34
@plemarquand
Copy link
Contributor Author

@swift-ci test

@plemarquand
Copy link
Contributor Author

@swift-ci test windows

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 migrating the Package Init Tests to Swift Testing. I have some blocking comments, hence the "requests changes". I'm happy to have a discussion if you feel the comments are non-blocking.


// Use shorter prefix on Windows to avoid MAX_PATH issues
#if os(Windows)
let prefix = "spm-\(abs(cleanedFunction.hashValue))"
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: if we want to use short prefix on Windows, why don't we set prefix = ""?

env: Environment? = nil,
sourceLocation: SourceLocation = #_sourceLocation,
) async {
for conf in configurations {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: instead of providing configurations as a function argument, consider passing as a test parameter so it can be clearly tracked as it's own test. See https://github.com/swiftlang/swift-package-manager/blob/main/Tests/CommandsTests/PackageCommandTests.swift#L100-L112 as a sample test that add a cross matrix of build system and build configurations. The name SupportedBuildSystemOnAllPlatforms is a bit misleading, but it currently is an array containing Native and Swift Build build systems.

let manifest = path.appending("Package.swift")
XCTAssertFileExists(manifest)
let manifestContents: String = try localFileSystem.readFileContents(manifest)
expectFileExists(at: manifest)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: consider changing this to a try #require(...), which may mean we need to create an associated helper if one does not currently exist. Using require makes more sense here since the following code requires it.


switch packageType {
case .library:
if buildSystem == .native {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: instead of using an if statement, consider using a switch buildSystem to ensure the tests are updated properly should entries to BuildSystemProvider.Kind be added or removed - this would ensure the tests are at least updated/looked at..

case .executable, .tool:
expectFileExists(at: expectedPath.appending(executableName(name)))
case .empty, .buildToolPlugin, .commandPlugin, .macro:
// These types don't have specific build products to verify or are verified separately
Copy link
Contributor

Choose a reason for hiding this comment

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

question: do we want to support tests calling this? this may provide a false positive. Consider calling Issue.record("Test implementation issue: \(packageType) don't have specific build products to verify. Replace this function call with a different verification steps") instead.


func testInitPackageNonc99Directory() async throws {
/// Tests creating a package in a directory with a non-C99 compliant name.
@Test(arguments: [BuildSystemProvider.Kind.native, .swiftbuild])
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: consider passing the Build configuration as a test argument.

Suggested change
@Test(arguments: [BuildSystemProvider.Kind.native, .swiftbuild])
@Test(
arguments: getBuildData(for: SupportedBuildSystemOnAllPlatforms),
)

func testInitPackageNonc99Directory() async throws {
/// Tests creating a package in a directory with a non-C99 compliant name.
@Test(arguments: [BuildSystemProvider.Kind.native, .swiftbuild])
func initPackageNonc99Directory(buildSystem: BuildSystemProvider.Kind) async throws {
Copy link
Contributor

Choose a reason for hiding this comment

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

Providing the build configuration via test arguments, as suggestd above, will update the argument type as follows:

Suggested change
func initPackageNonc99Directory(buildSystem: BuildSystemProvider.Kind) async throws {
func initPackageNonc99Directory(
data: BuildData,
) async throws {

await expectBuilds(packageRoot, buildSystem: buildSystem)

// Assert that the expected build products exist
let expectedPath = packageRoot.appending(components: try buildSystem.binPath(for: BuildConfiguration.debug))
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (blocking): Since the test currently build both debug and release build configuration, we should also assert on the expected behaviour for the release build configuration.

let expectedPath = packageRoot.appending(components: try buildSystem.binPath(for: BuildConfiguration.debug))

// Verify the module name is properly mangled
if buildSystem == .native {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: change this from an if to a swift so we get compilation time error should the build system kinds be updated.

I used the following when I wanted to support a subset of build systems in a test

switch buildSystem {
    case .native:
        #expect("native build expectations".count > 1)
    case .swiftbuild:
        #expect("swift build expectations".count > 1)
    case .xcode:
        Issue.record("Test expectations have not been implemented.")
}

buildSystem: .native,
)

await expectBuilds(packageRoot, buildSystem: buildSystem)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: consider adding the build configuration as a test data

@plemarquand
Copy link
Contributor Author

@swift-ci test

@plemarquand
Copy link
Contributor Author

@swift-ci test windows

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