Run CFamilyTargetTests with Swift Build backend #8772
Conversation
owenv
requested review from
jakepetroules,
dschaefer2,
shawnhyam,
bripeticca,
plemarquand,
bkhouri,
cmcgee1024 and
daveyc123
as code owners
|
@swift-ci test |
|
@swift-ci test |
|
@swift-ci test Windows |
There was a problem hiding this comment.
Thanks you for augmenting the tests to run with the SwiftBuild backend, and to convert the test to Swift Testing in the process.
The reason for requesting change is the reduction of test coverage with the conversion to Swift Testing by only building in the debug configuration, while the XCTest was building in both debug and release configuration.
| XCTAssertDirectoryContainsFile(dir: debugPath, filename: "Foo.c.o") | ||
| @Suite(.serialized) | ||
| struct CFamilyTargetTestCase { | ||
| @Test(arguments: [BuildSystemProvider.Kind.native, .swiftbuild]) |
There was a problem hiding this comment.
issue (blocking): The XCTAssertBuilds (used on line 40 of the left-hand file) builds, by default, against both release and debug Build configuration. Can we augment this test to add the build arguments as a parameter.
ie:
@Test(arguments: [BuildSystemProvider.Kind.native, .swiftbuild], BuildConfiguration.allCases)
func testCLibraryWithSpaces(
buildSystem: BuildSystemProvider.Kind,
configuration: BuildConfiguration
) async throws {
// ...
try await executSwiftBuild(..., configuration: configuration,...),
// ...
}
NOTE: This comment applies to all other occurrences where XCTAssertBuild was called in a test.
| XCTAssertDirectoryContainsFile(dir: debugPath, filename: "Sea.c.o") | ||
| XCTAssertDirectoryContainsFile(dir: debugPath, filename: "Foo.c.o") | ||
| try await executeSwiftBuild(packageRoot, buildSystem: buildSystem) | ||
| if buildSystem == .native { |
There was a problem hiding this comment.
question: do we expect the swiftbuild build system to "support' the subsequent "expectations"? If so, can we raise a GitHub issue and put wrap this with a withKnownIssue { ... } when: { ... }
This comment applies to all tests where there is a similar conditional.
| let errStr = "\(err)" | ||
| let missingIncludeDirStr = "\(ModuleError.invalidPublicHeadersDirectory("Cfactorial"))" | ||
| XCTAssert(errStr.contains(missingIncludeDirStr)) | ||
| var buildError: (any Error)? = nil |
There was a problem hiding this comment.
If we know the exact error message. this can be replaced with the following, or similar.
try await #expect(throws: ModuleError.invalidPublicHeadersDirectory("Cfactorial")) {
try await executeSwiftBuild(fixturePath, buildSystem: buildSystem)
}
or
let error = await #expect(throws: SwiftPMError.self ) {
try await executeSwiftBuild(fixturePath, buildSystem: buildSystem
}
// THEN I expect a failure
guard case SwiftPMError.executionFailure(_, let stdout, let stderr) = try #require(error) else {
Issue.record("Incorrect error was raised.")
return
}
let missingIncludeDirStr = "\(ModuleError.invalidPublicHeadersDirectory("Cfactorial"))"
#expect(stderr.contains(missingIncludeDirStr))
| let debugPath = fixturePath.appending(components: ".build", try UserToolchain.default.targetTriple.platformBuildPathComponent, "debug") | ||
| XCTAssertDirectoryContainsFile(dir: debugPath, filename: "Bar.c.o") | ||
| XCTAssertDirectoryContainsFile(dir: debugPath, filename: "Foo.c.o") | ||
| @Suite(.serialized) |
There was a problem hiding this comment.
suggestion: can we tag this as a large test?
e.g.: .tag(Tag.TestSize.large)
| struct CFamilyTargetTestCase { | ||
| @Test(arguments: [BuildSystemProvider.Kind.native, .swiftbuild]) | ||
| func testCLibraryWithSpaces(buildSystem: BuildSystemProvider.Kind) async throws { | ||
| try await withKnownIssue("https://github.com/swiftlang/swift-build/issues/333") { |
There was a problem hiding this comment.
suggestion (non-blocking): can we add the .bug trait to the test and reference the GitHub issue?
| func testCLibraryWithSpaces(buildSystem: BuildSystemProvider.Kind) async throws { | ||
| try await withKnownIssue("https://github.com/swiftlang/swift-build/issues/333") { | ||
| try await fixture(name: "CFamilyTargets/CLibraryWithSpaces") { fixturePath in | ||
| try await executeSwiftBuild(fixturePath, buildSystem: buildSystem) |
There was a problem hiding this comment.
suggestion (non-blocking): since this is calling swift-build executable, can we add the Tag.Feature.Command.Build tag? We would need to add the following test trait: .tags(Tag.Feature.Command.Build)
This comment applies to all tests that invoke the swift-build executable.
| // Run swift-test on package. | ||
| await XCTAssertSwiftTest(fixturePath) | ||
| try await executeSwiftTest(fixturePath, buildSystem: buildSystem) |
There was a problem hiding this comment.
suggestion (non-blocking): since this is calling swift-test executable, can we add the Tag.Feature.Command.Test tag? We would need to add the following test trait: .tags(Tag.Feature.Command.Test)
This comment applies to all tests that invoke the swift-test executable.
|
Seeing if @daveinglis's patches have fixed the failures here before I address the other feedback |
|
@swift-ci test |
1 similar comment
|
@swift-ci test |
While I'm here, port them to Swift testing. When using Swift Build, I skip the checks for specific .o files in the intermediates directory- I think that layout is best tested at the Swift Build layer