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

add flag to disable building with testablity when running tests #4119

Merged
merged 3 commits into from
Feb 11, 2022
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import XCTest
import TestableExe1
import TestableExe2
@testable import TestableExe1
@testable import TestableExe2
// import TestableExe3
import class Foundation.Bundle

Expand All @@ -9,7 +9,7 @@ final class TestableExeTests: XCTestCase {
// This is an example of a functional test case.
// Use XCTAssert and related functions to verify your tests produce the correct
// results.

print(GetGreeting1())
XCTAssertEqual(GetGreeting1(), "Hello, world")
print(GetGreeting2())
Expand Down
14 changes: 9 additions & 5 deletions Sources/Build/BuildPlan.swift
Original file line number Diff line number Diff line change
Expand Up @@ -589,7 +589,7 @@ public final class SwiftTargetBuildDescription {
public let isTestTarget: Bool

/// True if this is the test discovery target.
public let testDiscoveryTarget: Bool
public let isTestDiscoveryTarget: Bool

/// True if this module needs to be parsed as a library based on the target type and the configuration
/// of the source code (for example because it has a single source file whose name isn't "main.swift").
Expand Down Expand Up @@ -632,7 +632,7 @@ public final class SwiftTargetBuildDescription {
buildToolPluginInvocationResults: [BuildToolPluginInvocationResult] = [],
prebuildCommandResults: [PrebuildCommandResult] = [],
isTestTarget: Bool? = nil,
testDiscoveryTarget: Bool = false,
isTestDiscoveryTarget: Bool = false,
fileSystem: FileSystem
) throws {
assert(target.underlyingTarget is SwiftTarget, "underlying target type mismatch \(target)")
Expand All @@ -641,7 +641,7 @@ public final class SwiftTargetBuildDescription {
self.buildParameters = buildParameters
// Unless mentioned explicitly, use the target type to determine if this is a test target.
self.isTestTarget = isTestTarget ?? (target.type == .test)
self.testDiscoveryTarget = testDiscoveryTarget
self.isTestDiscoveryTarget = isTestDiscoveryTarget
self.fileSystem = fileSystem
self.tempsPath = buildParameters.buildPath.appending(component: target.c99name + ".build")
self.derivedSources = Sources(paths: [], root: tempsPath.appending(component: "DerivedSources"))
Expand Down Expand Up @@ -1095,7 +1095,11 @@ public final class SwiftTargetBuildDescription {

/// Testing arguments according to the build configuration.
private var testingArguments: [String] {
if buildParameters.enableTestability {
if self.isTestTarget {
// test targets must be built with -enable-testing
// since its required for test discovery (the non objective-c reflection kind)
return ["-enable-testing"]
} else if buildParameters.enableTestability {
return ["-enable-testing"]
} else {
return []
Expand Down Expand Up @@ -1564,7 +1568,7 @@ public class BuildPlan {
toolsVersion: toolsVersion,
buildParameters: buildParameters,
isTestTarget: true,
testDiscoveryTarget: true,
isTestDiscoveryTarget: true,
fileSystem: fileSystem
)

Expand Down
2 changes: 1 addition & 1 deletion Sources/Build/LLBuildManifestBuilder.swift
Original file line number Diff line number Diff line change
Expand Up @@ -823,7 +823,7 @@ extension LLBuildManifestBuilder {
for target in plan.targets {
guard case .swift(let target) = target,
target.isTestTarget,
target.testDiscoveryTarget else { continue }
target.isTestDiscoveryTarget else { continue }

let testDiscoveryTarget = target

Expand Down
28 changes: 21 additions & 7 deletions Sources/Commands/SwiftTestTool.swift
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,10 @@ struct TestToolOptions: ParsableArguments {
@Option(help: "Test the specified product.")
var testProduct: String?

/// Generate LinuxMain entries and exit.
@Flag(name: .customLong("testable-imports"), inversion: .prefixedEnableDisable, help: "Enable or disable testable imports. Enabled by default.")
var enableTestableImports: Bool = true
Copy link
Contributor Author

@tomerd tomerd Feb 10, 2022

Choose a reason for hiding this comment

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

the flag is --disable-testable-imports. ideas for a different name welcome. the user facing feature is testable imports so this is what I went with.


/// Returns the test case specifier if overridden in the env.
private func testCaseSkipOverride() -> TestCaseSpecifier? {
guard let override = ProcessEnv.vars["_SWIFTPM_SKIP_TESTS_LIST"] else {
Expand Down Expand Up @@ -238,7 +242,7 @@ public struct SwiftTestTool: SwiftCommand {
guard let rootManifest = rootManifests.values.first else {
throw StringError("invalid manifests at \(root.packages)")
}
let buildParameters = try swiftTool.buildParametersForTest()
let buildParameters = try swiftTool.buildParametersForTest(options: self.options)
print(codeCovAsJSONPath(buildParameters: buildParameters, packageName: rootManifest.displayName))

case .generateLinuxMain:
Expand All @@ -259,7 +263,7 @@ public struct SwiftTestTool: SwiftCommand {
case .runSerial:
let toolchain = try swiftTool.getToolchain()
let testProducts = try buildTestsIfNeeded(swiftTool: swiftTool)
let buildParameters = try swiftTool.buildParametersForTest()
let buildParameters = try swiftTool.buildParametersForTest(options: self.options)

// Clean out the code coverage directory that may contain stale
// profraw files from a previous run of the code coverage tool.
Expand Down Expand Up @@ -327,7 +331,7 @@ public struct SwiftTestTool: SwiftCommand {
let tests = try testSuites
.filteredTests(specifier: options.testCaseSpecifier)
.skippedTests(specifier: options.testCaseSkip)
let buildParameters = try swiftTool.buildParametersForTest()
let buildParameters = try swiftTool.buildParametersForTest(options: self.options)

// If there were no matches, emit a warning and exit.
if tests.isEmpty {
Expand Down Expand Up @@ -383,7 +387,7 @@ public struct SwiftTestTool: SwiftCommand {
// Merge all the profraw files to produce a single profdata file.
try mergeCodeCovRawDataFiles(swiftTool: swiftTool)

let buildParameters = try swiftTool.buildParametersForTest()
let buildParameters = try swiftTool.buildParametersForTest(options: self.options)
for product in testProducts {
// Export the codecov data as JSON.
let jsonPath = codeCovAsJSONPath(
Expand All @@ -399,7 +403,7 @@ public struct SwiftTestTool: SwiftCommand {
let llvmProf = try swiftTool.getToolchain().getLLVMProf()

// Get the profraw files.
let buildParameters = try swiftTool.buildParametersForTest()
let buildParameters = try swiftTool.buildParametersForTest(options: self.options)
let codeCovFiles = try localFileSystem.getDirectoryContents(buildParameters.codeCovPath)

// Construct arguments for invoking the llvm-prof tool.
Expand All @@ -423,7 +427,7 @@ public struct SwiftTestTool: SwiftCommand {
private func exportCodeCovAsJSON(to path: AbsolutePath, testBinary: AbsolutePath, swiftTool: SwiftTool) throws {
// Export using the llvm-cov tool.
let llvmCov = try swiftTool.getToolchain().getLLVMCov()
let buildParameters = try swiftTool.buildParametersForTest()
let buildParameters = try swiftTool.buildParametersForTest(options: self.options)
let args = [
llvmCov.pathString,
"export",
Expand All @@ -443,7 +447,8 @@ public struct SwiftTestTool: SwiftCommand {
///
/// - Returns: The paths to the build test products.
private func buildTestsIfNeeded(swiftTool: SwiftTool) throws -> [BuiltTestProduct] {
let buildSystem = try swiftTool.createBuildSystem(buildParameters: swiftTool.buildParametersForTest())
let buildParameters = try swiftTool.buildParametersForTest(options: self.options)
let buildSystem = try swiftTool.createBuildSystem(buildParameters: buildParameters)

if options.shouldBuildTests {
let subset = options.testProduct.map(BuildSubset.product) ?? .allIncludingTests
Expand Down Expand Up @@ -1019,8 +1024,17 @@ final class XUnitGenerator {
}
}

extension SwiftTool {
func buildParametersForTest(options: TestToolOptions) throws -> BuildParameters {
try self.buildParametersForTest(
enableTestability: options.enableTestableImports
)
}
}

private extension Basics.Diagnostic {
static var noMatchingTests: Self {
.warning("No matching test cases were run")
}
}

9 changes: 6 additions & 3 deletions Sources/Commands/TestingSupport.swift
Original file line number Diff line number Diff line change
Expand Up @@ -134,10 +134,13 @@ enum TestingSupport {
}

extension SwiftTool {
func buildParametersForTest() throws -> BuildParameters {
func buildParametersForTest(
enableTestability: Bool? = nil
) throws -> BuildParameters {
var parameters = try self.buildParameters()
// for test commands, alway enable building with testability enabled
parameters.enableTestability = true
// for test commands, we normally enable building with testability
// but we let users override this with a flag
parameters.enableTestability = enableTestability ?? true
return parameters
}
}
18 changes: 9 additions & 9 deletions Sources/LLBuildManifest/BuildManifest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public struct BuildManifest {
inputs: [Node],
outputs: [Node]
) {
assert(commands[name] == nil, "aleady had a command named '\(name)'")
assert(commands[name] == nil, "already had a command named '\(name)'")
let tool = PhonyTool(inputs: inputs, outputs: outputs)
commands[name] = Command(name: name, tool: tool)
}
Expand All @@ -61,7 +61,7 @@ public struct BuildManifest {
inputs: [Node],
outputs: [Node]
) {
assert(commands[name] == nil, "aleady had a command named '\(name)'")
assert(commands[name] == nil, "already had a command named '\(name)'")
let tool = TestDiscoveryTool(inputs: inputs, outputs: outputs)
commands[name] = Command(name: name, tool: tool)
}
Expand All @@ -71,7 +71,7 @@ public struct BuildManifest {
inputs: [Node],
outputs: [Node]
) {
assert(commands[name] == nil, "aleady had a command named '\(name)'")
assert(commands[name] == nil, "already had a command named '\(name)'")
let tool = CopyTool(inputs: inputs, outputs: outputs)
commands[name] = Command(name: name, tool: tool)
}
Expand All @@ -81,7 +81,7 @@ public struct BuildManifest {
inputs: [Node],
outputs: [Node]
) {
assert(commands[name] == nil, "aleady had a command named '\(name)'")
assert(commands[name] == nil, "already had a command named '\(name)'")
let tool = PackageStructureTool(inputs: inputs, outputs: outputs)
commands[name] = Command(name: name, tool: tool)
}
Expand All @@ -91,7 +91,7 @@ public struct BuildManifest {
inputs: [Node],
outputs: [Node]
) {
assert(commands[name] == nil, "aleady had a command named '\(name)'")
assert(commands[name] == nil, "already had a command named '\(name)'")
let tool = ArchiveTool(inputs: inputs, outputs: outputs)
commands[name] = Command(name: name, tool: tool)
}
Expand All @@ -106,7 +106,7 @@ public struct BuildManifest {
workingDirectory: String? = nil,
allowMissingInputs: Bool = false
) {
assert(commands[name] == nil, "aleady had a command named '\(name)'")
assert(commands[name] == nil, "already had a command named '\(name)'")
let tool = ShellTool(
description: description,
inputs: inputs,
Expand All @@ -127,7 +127,7 @@ public struct BuildManifest {
outputs: [Node],
arguments: [String]
) {
assert(commands[name] == nil, "aleady had a command named '\(name)'")
assert(commands[name] == nil, "already had a command named '\(name)'")
let tool = SwiftFrontendTool(
moduleName: moduleName,
description: description,
Expand All @@ -146,7 +146,7 @@ public struct BuildManifest {
arguments: [String],
dependencies: String? = nil
) {
assert(commands[name] == nil, "aleady had a command named '\(name)'")
assert(commands[name] == nil, "already had a command named '\(name)'")
let tool = ClangTool(
description: description,
inputs: inputs,
Expand All @@ -173,7 +173,7 @@ public struct BuildManifest {
isLibrary: Bool,
wholeModuleOptimization: Bool
) {
assert(commands[name] == nil, "aleady had a command named '\(name)'")
assert(commands[name] == nil, "already had a command named '\(name)'")
let tool = SwiftCompilerTool(
inputs: inputs,
outputs: outputs,
Expand Down
6 changes: 6 additions & 0 deletions Sources/SPMBuildCore/BuildParameters.swift
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,12 @@ public struct BuildParameters: Encodable {
self.isXcodeBuildSystemEnabled = isXcodeBuildSystemEnabled
self.printManifestGraphviz = printManifestGraphviz
// decide on testability based on debug/release config
// the goals of this being based on the build configuration is
// that `swift build` followed by a `swift test` will need to do minimal rebuilding
// given that the default configuration for `swift build` is debug
// and that `swift test` normally requires building with testable enabled.
// when building and testing in release mode, one can use the '--disable-testable-imports' flag
// to disable testability in `swift test`, but that requires that the tests do not use the testable imports feature
self.enableTestability = enableTestability ?? (.debug == configuration)
// decide if to enable the use of test manifests based on platform. this is likely to change in the future
self.testDiscoveryStrategy = triple.isDarwin() ? .objectiveC : .manifest(generate: forceTestDiscovery)
Expand Down
33 changes: 28 additions & 5 deletions Tests/CommandsTests/TestToolTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,15 @@
See http://swift.org/CONTRIBUTORS.txt for Swift project authors
*/

import XCTest

import SPMTestSupport
import Commands
import SPMTestSupport
import TSCBasic
import XCTest

final class TestToolTests: CommandsTestCase {

private func execute(_ args: [String]) throws -> (stdout: String, stderr: String) {
return try SwiftPMProduct.SwiftTest.execute(args)
private func execute(_ args: [String], packagePath: AbsolutePath? = nil) throws -> (stdout: String, stderr: String) {
return try SwiftPMProduct.SwiftTest.execute(args, packagePath: packagePath)
}

func testUsage() throws {
Expand Down Expand Up @@ -58,4 +58,27 @@ final class TestToolTests: CommandsTestCase {
}
#endif
}

func testEnableDisableTestability() {
fixture(name: "Miscellaneous/TestableExe") { path in
// default should run with testability
do {
let result = try execute(["--vv"], packagePath: path)
XCTAssertMatch(result.stdout, .contains("-enable-testing"))
}

// disabled
do {
_ = try execute(["--disable-testable-imports", "--vv"], packagePath: path)
} catch SwiftPMProductError.executionFailure(_, let stdout, _) {
XCTAssertMatch(stdout, .contains("was not compiled for testing"))
}

// enabled
do {
let result = try execute(["--enable-testable-imports", "--vv"], packagePath: path)
XCTAssertMatch(result.stdout, .contains("-enable-testing"))
}
}
}
}