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

Remove previously deprecated --enable-test-discovery #7391

Merged
merged 7 commits into from Mar 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
78 changes: 30 additions & 48 deletions Sources/Build/BuildPlan/BuildPlan+Test.swift
Expand Up @@ -33,21 +33,19 @@ extension BuildPlan {
_ observabilityScope: ObservabilityScope
) throws -> [(product: ResolvedProduct, discoveryTargetBuildDescription: SwiftTargetBuildDescription?, entryPointTargetBuildDescription: SwiftTargetBuildDescription)] {
guard buildParameters.testingParameters.testProductStyle.requiresAdditionalDerivedTestTargets,
case .entryPointExecutable(let explicitlyEnabledDiscovery, let explicitlySpecifiedPath) =
case .entryPointExecutable(let explicitlySpecifiedPath) =
buildParameters.testingParameters.testProductStyle
else {
throw InternalError("makeTestManifestTargets should not be used for build plan which does not require additional derived test targets")
}

let isEntryPointPathSpecifiedExplicitly = explicitlySpecifiedPath != nil

var isDiscoveryEnabledRedundantly = explicitlyEnabledDiscovery && !isEntryPointPathSpecifiedExplicitly
var result: [(ResolvedProduct, SwiftTargetBuildDescription?, SwiftTargetBuildDescription)] = []
for testProduct in graph.allProducts where testProduct.type == .test {
guard let package = graph.package(for: testProduct) else {
throw InternalError("package not found for \(testProduct)")
}
isDiscoveryEnabledRedundantly = isDiscoveryEnabledRedundantly && nil == testProduct.testEntryPointTarget
// If a non-explicitly specified test entry point file exists, prefer that over test discovery.
// This is designed as an escape hatch when test discovery is not appropriate and for backwards
// compatibility for projects that have existing test entry point files (e.g. XCTMain.swift, LinuxMain.swift).
Expand All @@ -58,10 +56,7 @@ extension BuildPlan {
// `--experimental-test-entry-point-path <file>`. The latter is useful because it still performs test discovery and places the discovered
// tests into a separate target/module named "<PackageName>PackageDiscoveredTests". Then, that entry point file may import that module and
// obtain that list to pass it to the `XCTMain(...)` function and avoid needing to maintain a list of tests itself.
if testProduct.testEntryPointTarget != nil && explicitlyEnabledDiscovery && !isEntryPointPathSpecifiedExplicitly {
let testEntryPointName = testProduct.underlying.testEntryPointPath?.basename ?? SwiftTarget.defaultTestEntryPointName
observabilityScope.emit(warning: "'--enable-test-discovery' was specified so the '\(testEntryPointName)' entry point file for '\(testProduct.name)' will be ignored and an entry point will be generated automatically. To use test discovery with a custom entry point file, pass '--experimental-test-entry-point-path <file>'.")
} else if testProduct.testEntryPointTarget == nil, let testEntryPointPath = explicitlySpecifiedPath, !fileSystem.exists(testEntryPointPath) {
if testProduct.testEntryPointTarget == nil, let testEntryPointPath = explicitlySpecifiedPath, !fileSystem.exists(testEntryPointPath) {
observabilityScope.emit(error: "'--experimental-test-entry-point-path' was specified but the file '\(testEntryPointPath)' could not be found.")
}

Expand Down Expand Up @@ -160,43 +155,34 @@ extension BuildPlan {
}

if let entryPointResolvedTarget = testProduct.testEntryPointTarget {
if isEntryPointPathSpecifiedExplicitly || explicitlyEnabledDiscovery {
if isEntryPointPathSpecifiedExplicitly {
// Allow using the explicitly-specified test entry point target, but still perform test discovery and thus declare a dependency on the discovery targets.
let entryPointTarget = SwiftTarget(
name: entryPointResolvedTarget.underlying.name,
dependencies: entryPointResolvedTarget.underlying.dependencies + swiftTargetDependencies,
packageAccess: entryPointResolvedTarget.packageAccess,
testEntryPointSources: entryPointResolvedTarget.underlying.sources
)
let entryPointResolvedTarget = ResolvedTarget(
packageIdentity: testProduct.packageIdentity,
underlying: entryPointTarget,
dependencies: entryPointResolvedTarget.dependencies + resolvedTargetDependencies,
defaultLocalization: testProduct.defaultLocalization,
supportedPlatforms: testProduct.supportedPlatforms,
platformVersionProvider: testProduct.platformVersionProvider
)
let entryPointTargetBuildDescription = try SwiftTargetBuildDescription(
package: package,
target: entryPointResolvedTarget,
toolsVersion: toolsVersion,
buildParameters: buildParameters,
testTargetRole: .entryPoint(isSynthesized: false),
disableSandbox: disableSandbox,
fileSystem: fileSystem,
observabilityScope: observabilityScope
)

result.append((testProduct, discoveryTargets?.buildDescription, entryPointTargetBuildDescription))
} else {
// Ignore test entry point and synthesize one, declaring a dependency on the test discovery targets created above.
let entryPointTargetBuildDescription = try generateSynthesizedEntryPointTarget(
swiftTargetDependencies: swiftTargetDependencies,
resolvedTargetDependencies: resolvedTargetDependencies
)
result.append((testProduct, discoveryTargets?.buildDescription, entryPointTargetBuildDescription))
}
if isEntryPointPathSpecifiedExplicitly {
// Allow using the explicitly-specified test entry point target, but still perform test discovery and thus declare a dependency on the discovery targets.
let entryPointTarget = SwiftTarget(
name: entryPointResolvedTarget.underlying.name,
dependencies: entryPointResolvedTarget.underlying.dependencies + swiftTargetDependencies,
packageAccess: entryPointResolvedTarget.packageAccess,
testEntryPointSources: entryPointResolvedTarget.underlying.sources
)
let entryPointResolvedTarget = ResolvedTarget(
packageIdentity: testProduct.packageIdentity,
underlying: entryPointTarget,
dependencies: entryPointResolvedTarget.dependencies + resolvedTargetDependencies,
defaultLocalization: testProduct.defaultLocalization,
supportedPlatforms: testProduct.supportedPlatforms,
platformVersionProvider: testProduct.platformVersionProvider
)
let entryPointTargetBuildDescription = try SwiftTargetBuildDescription(
package: package,
target: entryPointResolvedTarget,
toolsVersion: toolsVersion,
buildParameters: buildParameters,
testTargetRole: .entryPoint(isSynthesized: false),
disableSandbox: disableSandbox,
fileSystem: fileSystem,
observabilityScope: observabilityScope
)

result.append((testProduct, discoveryTargets?.buildDescription, entryPointTargetBuildDescription))
} else {
// Use the test entry point as-is, without performing test discovery.
let entryPointTargetBuildDescription = try SwiftTargetBuildDescription(
Expand All @@ -221,10 +207,6 @@ extension BuildPlan {
}
}

if isDiscoveryEnabledRedundantly {
observabilityScope.emit(warning: "'--enable-test-discovery' option is deprecated; tests are automatically discovered on all platforms")
}

return result
}
}
Expand Down
8 changes: 1 addition & 7 deletions Sources/Commands/Utilities/TestingSupport.swift
Expand Up @@ -215,19 +215,13 @@ extension SwiftCommandState {
) throws -> BuildParameters {
var parameters = try self.productsBuildParameters

var explicitlyEnabledDiscovery = false
var explicitlySpecifiedPath: AbsolutePath?
if case let .entryPointExecutable(
explicitlyEnabledDiscoveryValue,
explicitlySpecifiedPathValue
) = parameters.testingParameters.testProductStyle {
explicitlyEnabledDiscovery = explicitlyEnabledDiscoveryValue
if case let .entryPointExecutable(explicitlySpecifiedPathValue) = parameters.testingParameters.testProductStyle {
explicitlySpecifiedPath = explicitlySpecifiedPathValue
}
parameters.testingParameters = .init(
configuration: parameters.configuration,
targetTriple: parameters.triple,
forceTestDiscovery: explicitlyEnabledDiscovery,
testEntryPointPath: explicitlySpecifiedPath,
library: library
)
Expand Down
5 changes: 0 additions & 5 deletions Sources/CoreCommands/Options.swift
Expand Up @@ -464,13 +464,8 @@ package struct BuildOptions: ParsableArguments {
#endif
}

/// Whether to enable test discovery on platforms without Objective-C runtime.
MaxDesiatov marked this conversation as resolved.
Show resolved Hide resolved
@Flag(help: .hidden)
package var enableTestDiscovery: Bool = false

/// Path of test entry point file to use, instead of synthesizing one or using `XCTMain.swift` in the package (if
/// present).
/// This implies `--enable-test-discovery`
@Option(
name: .customLong("experimental-test-entry-point-path"),
help: .hidden
Expand Down
11 changes: 0 additions & 11 deletions Sources/CoreCommands/SwiftCommandState.swift
Expand Up @@ -371,16 +371,6 @@ package final class SwiftCommandState {
observabilityScope.emit(.mutuallyExclusiveArgumentsError(arguments: ["--arch", "--triple"]))
}

// --enable-test-discovery should never be called on darwin based platforms
#if canImport(Darwin)
MaxDesiatov marked this conversation as resolved.
Show resolved Hide resolved
if options.build.enableTestDiscovery {
observabilityScope
.emit(
warning: "'--enable-test-discovery' option is deprecated; tests are automatically discovered on all platforms"
)
}
#endif

if options.caching.shouldDisableManifestCaching {
observabilityScope
.emit(
Expand Down Expand Up @@ -777,7 +767,6 @@ package final class SwiftCommandState {
testingParameters: .init(
configuration: options.build.configuration,
targetTriple: triple,
forceTestDiscovery: options.build.enableTestDiscovery, // backwards compatibility, remove with --enable-test-discovery
testEntryPointPath: options.build.testEntryPointPath
)
)
Expand Down
Expand Up @@ -28,15 +28,9 @@ extension BuildParameters {
/// `--experimental-test-entry-point-path <file>` option, that file is used, otherwise if an `XCTMain.swift`
/// (formerly `LinuxMain.swift`) file is located in the package, it is used.
///
/// - Parameter explicitlyEnabledDiscovery: Whether test discovery generation was forced by passing
/// `--enable-test-discovery`, overriding any custom test entry point file specified via other CLI options or located in
/// the package.
/// - Parameter explicitlySpecifiedPath: The path to the test entry point file, if one was specified explicitly via
/// `--experimental-test-entry-point-path <file>`.
case entryPointExecutable(
explicitlyEnabledDiscovery: Bool,
explicitlySpecifiedPath: AbsolutePath?
)
case entryPointExecutable(explicitlySpecifiedPath: AbsolutePath?)

/// Whether this test product style requires additional, derived test targets, i.e. there must be additional test targets, beyond those
/// listed explicitly in the package manifest, created in order to add additional behavior (such as entry point logic).
Expand All @@ -54,7 +48,7 @@ extension BuildParameters {
switch self {
case .loadableBundle:
return nil
case .entryPointExecutable(explicitlyEnabledDiscovery: _, explicitlySpecifiedPath: let entryPointPath):
case .entryPointExecutable(explicitlySpecifiedPath: let entryPointPath):
return entryPointPath
}
}
Expand All @@ -66,7 +60,6 @@ extension BuildParameters {

public enum CodingKeys: CodingKey {
case _case
case explicitlyEnabledDiscovery
case explicitlySpecifiedPath
}

Expand All @@ -75,9 +68,8 @@ extension BuildParameters {
switch self {
case .loadableBundle:
try container.encode(DiscriminatorKeys.loadableBundle, forKey: ._case)
case .entryPointExecutable(let explicitlyEnabledDiscovery, let explicitlySpecifiedPath):
case .entryPointExecutable(let explicitlySpecifiedPath):
try container.encode(DiscriminatorKeys.entryPointExecutable, forKey: ._case)
try container.encode(explicitlyEnabledDiscovery, forKey: .explicitlyEnabledDiscovery)
try container.encode(explicitlySpecifiedPath, forKey: .explicitlySpecifiedPath)
}
}
Expand Down Expand Up @@ -122,7 +114,6 @@ extension BuildParameters {
enableCodeCoverage: Bool = false,
enableTestability: Bool? = nil,
experimentalTestOutput: Bool = false,
forceTestDiscovery: Bool = false,
testEntryPointPath: AbsolutePath? = nil,
library: Library = .xctest
) {
Expand All @@ -137,7 +128,6 @@ extension BuildParameters {
// to disable testability in `swift test`, but that requires that the tests do not use the testable imports feature
self.enableTestability = enableTestability ?? (.debug == configuration)
self.testProductStyle = (targetTriple.isDarwin() && library == .xctest) ? .loadableBundle : .entryPointExecutable(
explicitlyEnabledDiscovery: forceTestDiscovery,
explicitlySpecifiedPath: testEntryPointPath
)
self.library = library
Expand Down
30 changes: 0 additions & 30 deletions Tests/CommandsTests/TestCommandTests.swift
Expand Up @@ -202,36 +202,6 @@ final class TestCommandTests: CommandsTestCase {
}
}

func testEnableTestDiscoveryDeprecation() throws {
let compilerDiagnosticFlags = ["-Xswiftc", "-Xfrontend", "-Xswiftc", "-Rmodule-interface-rebuild"]
#if canImport(Darwin)
// should emit when LinuxMain is present
try fixture(name: "Miscellaneous/TestDiscovery/Simple") { fixturePath in
let (_, stderr) = try SwiftPM.Test.execute(["--enable-test-discovery"] + compilerDiagnosticFlags, packagePath: fixturePath)
XCTAssertMatch(stderr, .contains("warning: '--enable-test-discovery' option is deprecated"))
}

// should emit when LinuxMain is not present
try fixture(name: "Miscellaneous/TestDiscovery/Simple") { fixturePath in
try localFileSystem.writeFileContents(fixturePath.appending(components: "Tests", SwiftTarget.defaultTestEntryPointName), bytes: "fatalError(\"boom\")")
let (_, stderr) = try SwiftPM.Test.execute(["--enable-test-discovery"] + compilerDiagnosticFlags, packagePath: fixturePath)
XCTAssertMatch(stderr, .contains("warning: '--enable-test-discovery' option is deprecated"))
}
#else
// should emit when LinuxMain is present
try fixture(name: "Miscellaneous/TestDiscovery/Simple") { fixturePath in
let (_, stderr) = try SwiftPM.Test.execute(["--enable-test-discovery"] + compilerDiagnosticFlags, packagePath: fixturePath)
XCTAssertMatch(stderr, .contains("warning: '--enable-test-discovery' option is deprecated"))
}
// should not emit when LinuxMain is present
try fixture(name: "Miscellaneous/TestDiscovery/Simple") { fixturePath in
try localFileSystem.writeFileContents(fixturePath.appending(components: "Tests", SwiftTarget.defaultTestEntryPointName), bytes: "fatalError(\"boom\")")
let (_, stderr) = try SwiftPM.Test.execute(["--enable-test-discovery"] + compilerDiagnosticFlags, packagePath: fixturePath)
XCTAssertNoMatch(stderr, .contains("warning: '--enable-test-discovery' option is deprecated"))
}
#endif
}

func testList() throws {
try fixture(name: "Miscellaneous/TestDiscovery/Simple") { fixturePath in
let (stdout, stderr) = try SwiftPM.Test.execute(["list"], packagePath: fixturePath)
Expand Down
15 changes: 0 additions & 15 deletions Tests/FunctionalTests/TestDiscoveryTests.swift
Expand Up @@ -88,21 +88,6 @@ class TestDiscoveryTests: XCTestCase {
}
}

func testEntryPointOverrideIgnored() throws {
#if os(macOS)
try XCTSkipIf(true)
#endif
try fixture(name: "Miscellaneous/TestDiscovery/Simple") { fixturePath in
let manifestPath = fixturePath.appending(components: "Tests", SwiftTarget.defaultTestEntryPointName)
try localFileSystem.writeFileContents(manifestPath, string: "fatalError(\"should not be called\")")
let (stdout, stderr) = try executeSwiftTest(fixturePath, extraArgs: ["--enable-test-discovery"])
// in "swift test" build output goes to stderr
XCTAssertMatch(stderr, .contains("Build complete!"))
// in "swift test" test output goes to stdout
XCTAssertNoMatch(stdout, .contains("Executed 1 test"))
}
}

func testTestExtensions() throws {
#if os(macOS)
try XCTSkipIf(true)
Expand Down