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

Move integration tests to use swift-testing #8396

Merged

Conversation

kcieplak
Copy link
Contributor

  • Move all integration tests to use swift-testing
  • Add some test skipping traits for OS types, Xcode etc.
  • Create a common test module
    • Move the helpers from test source to common test module.
    • TSCBasic requires a minimum os of 10.3, force produce to 13.0
    • Update the IntegrationTests sub package to use the common test module.
    • Remove XCTest macros from within the StringChecker for #expect usage.
  • Run swift-format on all files

@kcieplak
Copy link
Contributor Author

@swift-ci test

public func notExists(_ path: AbsolutePath, followSymlink: Bool = false) -> Bool {
return !exists(path, followSymlink: followSymlink)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

missing trailing newline

Suggested change
}
}

@@ -177,7 +184,8 @@ func _sh(

environment.merge(env, uniquingKeysWith: { $1 })

let result = try Process.popen(arguments: arguments.map { $0.description }, environment: environment)
let result = try Process.popen(
arguments: arguments.map { $0.description }, environment: environment)
Copy link
Contributor

@MaxDesiatov MaxDesiatov Mar 20, 2025

Choose a reason for hiding this comment

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

Was this formatting change made by SwiftFormat or swift-format? Per CONTRIBUTING.md we're using SwiftFormat here, which IIUC would format it differently.

Suggested change
arguments: arguments.map { $0.description }, environment: environment)
arguments: arguments.map { $0.description }, environment: environment
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like the vscode swift extension picked swift-format. I will re-run with SwiftFormat.

Comment on lines 22 to 32
public func hostOperatingSystem() -> OperatingSystem {
#if os(macOS)
return .macOS
#elseif os(Linux)
return .linux
#elseif os(Windows)
return .windows
#else
return .unknown
#endif
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting seems off here. Additionally, why not use a stored property instead of function calls if this is a compile-time constant?

Suggested change
public func hostOperatingSystem() -> OperatingSystem {
#if os(macOS)
return .macOS
#elseif os(Linux)
return .linux
#elseif os(Windows)
return .windows
#else
return .unknown
#endif
}
#if os(macOS)
static let hostOperatingSystem = OperatingSystem.macOS
#elseif os(Linux)
static let hostOperatingSystem = OperatingSystem.linux
#elseif os(Windows)
static let hostOperatingSystem = OperatingSystem.windows
#else
static let hostOperatingSystem = OperatingSystem.unknown
#endif

#endif
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing final newline

Suggested change
}
}

ProcessInfo.processInfo.environment["SWIFTCI_IS_SELF_HOSTED"] != nil
}
}
// Skip test if the built by XCode.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: typo fix

Suggested change
// Skip test if the built by XCode.
// Skip test if built by Xcode.

Comment on lines 16 to 34
// Skip test if the host operating system does not match the running OS.
public static func requireHostOS(_ os: OperatingSystem, when condition: Bool = true) -> Self {
enabled("This test requires a \(os) host OS.", { ProcessInfo.processInfo.hostOperatingSystem() == os && condition })
}
// Skip test if the host operating system matches the running OS.
public static func skipHostOS(_ os: OperatingSystem, _ comment: Comment? = nil) -> Self {
disabled(comment ?? "This test cannot run on a \(os) host OS.", { ProcessInfo.processInfo.hostOperatingSystem() == os })
}
// Skip test unconditionally
public static func skip(_ comment: Comment? = nil) -> Self {
disabled(comment ?? "Unconditional skip, a comment should be added for the reason", { true } )
}
// Skip test will be skipped if the environment is self hosted.
public static func skipSwiftCISelfHosted(_ comment: Comment? = nil) -> Self {
disabled(comment ?? "SwiftCI is self hosted") {
ProcessInfo.processInfo.environment["SWIFTCI_IS_SELF_HOSTED"] != nil
}
}
// Skip test if the built by XCode.
Copy link
Contributor

@MaxDesiatov MaxDesiatov Mar 20, 2025

Choose a reason for hiding this comment

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

nit: formatting-wise we separate declarations with a single newline.

Suggested change
// Skip test if the host operating system does not match the running OS.
public static func requireHostOS(_ os: OperatingSystem, when condition: Bool = true) -> Self {
enabled("This test requires a \(os) host OS.", { ProcessInfo.processInfo.hostOperatingSystem() == os && condition })
}
// Skip test if the host operating system matches the running OS.
public static func skipHostOS(_ os: OperatingSystem, _ comment: Comment? = nil) -> Self {
disabled(comment ?? "This test cannot run on a \(os) host OS.", { ProcessInfo.processInfo.hostOperatingSystem() == os })
}
// Skip test unconditionally
public static func skip(_ comment: Comment? = nil) -> Self {
disabled(comment ?? "Unconditional skip, a comment should be added for the reason", { true } )
}
// Skip test will be skipped if the environment is self hosted.
public static func skipSwiftCISelfHosted(_ comment: Comment? = nil) -> Self {
disabled(comment ?? "SwiftCI is self hosted") {
ProcessInfo.processInfo.environment["SWIFTCI_IS_SELF_HOSTED"] != nil
}
}
// Skip test if the built by XCode.
// Skip test if the host operating system does not match the running OS.
public static func requireHostOS(_ os: OperatingSystem, when condition: Bool = true) -> Self {
enabled("This test requires a \(os) host OS.", { ProcessInfo.processInfo.hostOperatingSystem() == os && condition })
}
// Skip test if the host operating system matches the running OS.
public static func skipHostOS(_ os: OperatingSystem, _ comment: Comment? = nil) -> Self {
disabled(comment ?? "This test cannot run on a \(os) host OS.", { ProcessInfo.processInfo.hostOperatingSystem() == os })
}
// Skip test unconditionally.
public static func skip(_ comment: Comment? = nil) -> Self {
disabled(comment ?? "Unconditional skip, a comment should be added for the reason", { true } )
}
// Skip test will be skipped if the environment is self hosted.
public static func skipSwiftCISelfHosted(_ comment: Comment? = nil) -> Self {
disabled(comment ?? "SwiftCI is self hosted") {
ProcessInfo.processInfo.environment["SWIFTCI_IS_SELF_HOSTED"] != nil
}
}
// Skip test if built by Xcode.

@kcieplak kcieplak force-pushed the topics/move-integration-tests-to-swift-testing branch 2 times, most recently from f21875d to 15849f1 Compare March 20, 2025 19:42
* Move all tests to use swift-testing
* Add some test skipping traits for OS types, Xcode etc.
* Create a common test module
    * Move the helpers from test source to common test module.
    * TSCBasic requires a minimum os of 10.3, force produce to 13.0
    * Update the IntegrationTests sub package to use the
      common test module.
@kcieplak kcieplak force-pushed the topics/move-integration-tests-to-swift-testing branch from 15849f1 to d173abe Compare March 20, 2025 19:45
@kcieplak
Copy link
Contributor Author

@swift-ci test

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.

Looks good, though I still see hints of XCTest and some test are no longer executed on linux host, while the XCTest version skipped the test only on Amazon Linux 2.

All comments that require code changes have been marked with (blocking),

}

@Test(
nil,
Copy link
Contributor

Choose a reason for hiding this comment

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

thought (non-blocking): I believe the first argument can be omitted, at least I have omitted them in a previous PR - which are still open.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason when I first converted the tests, compilation was failing as the @test was expecting String? as the first argument. I removed the nil, and is seems to work now. Not sure what changed.

}

// Skip test unconditionally
public static func skip(_ comment: Comment? = nil) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: can we use .disabled(...) instead of creating an extra indirection?

#endif

// Test SwiftBuildSystem
@Test(nil, .skipHostOS(.linux, "Amazon Linux has platform issues"))
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): this will skip the test on all linux host, but the issue is only on Amazon Linux 2. Can we update the skipHostOS trait that optionally accepts an "enum" which we can use skip against a specific distribution?

if the optional is not set, skip on all host. if the optional is set, skip only on that distribution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just copy the code from https://github.com/swiftlang/swift-build/blob/cd632ba33e264dcac4bcf3d09f83e404c3408f09/Sources/SWBTestSupport/SkippedTestSupport.swift#L140; the sole issue with Amazon Linux 2 is the lack of posix_spawn_file_actions_addchdir, so let's be explicit about that. Technically the same issue would impact OpenBSD.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, yeah not sure why I missed that the code was checking not just for #if os(linux), but probing the distro as well.

I will borrow more from the swift-build test support ;)

#if !os(Windows)
// SWBINTTODO: Windows fails to link this library package due to a "lld-link: error: subsystem must be defined" error. See https://github.com/swiftlang/swift-build/issues/310
@Test(
nil, .skipHostOS(.linux, "Amazon Linux has platform issues"),
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): this will skip the test on all linux host, but the issue is only on Amazon Linux 2. Can we update the skipHostOS trait that optionally accepts an "enum" which we can use skip against a specific distribution?

if the optional is not set, skip on all host. if the optional is set, skip only on that distribution.

import TSCBasic
import TSCTestSupport
import XCTest
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 we converted to swift testing, can we remove this import and replace all XCTest API with the associated Swift Testing call?

See https://developer.apple.com/documentation/testing/migratingfromxctest#Record-issues

* Add test conditional Trait for detecting thread safe
  working directories.  Issue present on Amazon linux
* Get rid of 'nil' parameter to @test macro.
* Remove all XCTest calls from Helpers.swift
* Add known issues to the binaryTargets() test, which
  was previously unconditionally skipped.
* Remove unused XCTest imports
@kcieplak kcieplak force-pushed the topics/move-integration-tests-to-swift-testing branch from 06ea324 to 1e8efb9 Compare March 21, 2025 13:01
@kcieplak
Copy link
Contributor Author

@swift-ci test

@kcieplak kcieplak requested a review from bkhouri March 21, 2025 14:02
@kcieplak
Copy link
Contributor Author

@swift-ci test windows

@kcieplak
Copy link
Contributor Author

@swift-ci test macos

#if !os(Windows)
// SWBINTTODO: Windows fails to link this library package due to a "lld-link: error: subsystem must be defined" error. See https://github.com/swiftlang/swift-build/issues/310
@Test(
.skipHostOS(
Copy link
Contributor

Choose a reason for hiding this comment

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

thought (non-blocking): I really wish Swift Testing has a .all(...) and .any(...) trait to make this more explicit as we don't know if this is currently an OR or AND condition.

Here, we need to skip the test if any of the conditions are true.

I created swiftlang/swift-testing#1034 to have an option to be more explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From https://developer.apple.com/documentation/testing/enablinganddisabling

@Test(
  "Ice cream is cold",
  .enabled(if: Season.current == .summer),
  .disabled("We ran out of sprinkles"),
  .bug(id: "12345")
)
func isCold() async throws { ... }

If a test has multiple conditions applied to it, they must all pass for it to run. Otherwise, the test notes the first condition to fail as the reason the test is skipped.

The traits are .enable or .disable depending on the extension of the trait. In this case it will have:

skipHostOS
enabled() with a condition of true if os != windows

requireThreadSafeWorkingDirectory
disabled() with a condition of true if the os does not support thread safe working directories

I think this logic is correct for this test-case.

@kcieplak
Copy link
Contributor Author

Jenkins instance was rebooted during macOS legs causing failures

@kcieplak
Copy link
Contributor Author

@swift-ci test macos

@bkhouri bkhouri merged commit 6c179de into swiftlang:main Mar 24, 2025
5 checks passed
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