Skip to content

Commit

Permalink
Skip evaluating parameterized test arguments for disabled tests (#507)
Browse files Browse the repository at this point in the history
Similar to how parameterized test arguments are left unevaluated for
tests that aren't included in a `Plan`, also leave them unevaluated for
tests in a `Plan` that won't run, e.g. because of having the
`.disabled()` trait attached.

### Motivation:

Since the changes in #366, parameterized test arguments are evaluated
lazily, only for tests which are included in the `Runner.Plan` that is
being prepared for execution. A code comment included in that change
further suggested that arguments are only evaluated for tests which will
actually run as part of the plan, as opposed to being skipped. However
the actual code didn't reflect that behavior, by mistake. That means
wasted work is being performed when setting up a `Runner.Plan`,
evaluating the arguments of tests which won't end up actually running.

### Modifications:

Only perform the step of evaluating test arguments/cases for tests which
are marked with a `.run` action in the `Runner.Plan`.

### Result:

Less throwaway work is performed when setting up a `Runner.Plan` for
greater efficiency.

### Checklist:

- [x] Code and documentation should follow the style of the [Style
Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md).
- [x] If public symbols are renamed or modified, DocC references should
be updated.
  • Loading branch information
briancroom authored Jun 27, 2024
1 parent 2cdfccf commit aacacd7
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 7 deletions.
14 changes: 8 additions & 6 deletions Sources/Testing/Running/Runner.Plan.swift
Original file line number Diff line number Diff line change
Expand Up @@ -253,12 +253,14 @@ extension Runner.Plan {
// can be evaluated lazily only once it is determined that the test will
// run, to avoid unnecessary work. But now is the appropriate time to
// evaluate them.
do {
try await test.evaluateTestCases()
} catch {
let sourceContext = SourceContext(backtrace: Backtrace(forFirstThrowOf: error))
let issue = Issue(kind: .errorCaught(error), comments: [], sourceContext: sourceContext)
action = .recordIssue(issue)
if case .run = action {
do {
try await test.evaluateTestCases()
} catch {
let sourceContext = SourceContext(backtrace: Backtrace(forFirstThrowOf: error))
let issue = Issue(kind: .errorCaught(error), comments: [], sourceContext: sourceContext)
action = .recordIssue(issue)
}
}

actionGraph.updateValue(action, at: keyPath)
Expand Down
14 changes: 14 additions & 0 deletions Tests/TestingTests/PlanTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,20 @@ struct PlanTests {
#expect(step.test.id.nameComponents.count == 1, "Test is not top-level: \(step.test)")
}
}

@Test("Test cases of a disabled test are not evaluated")
func disabledTestCases() async throws {
var configuration = Configuration()
configuration.setEventHandler { event, context in
guard case .testSkipped = event.kind else {
return
}
let testSnapshot = try #require(context.test.map({ Test.Snapshot(snapshotting: $0 )}))
#expect(testSnapshot.testCases?.isEmpty ?? false)
}

await runTestFunction(named: "disabled(x:)", in: ParameterizedTests.self, configuration: configuration)
}
}

// MARK: - Fixtures
Expand Down
4 changes: 3 additions & 1 deletion Tests/TestingTests/Test.Case.ArgumentTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ struct Test_Case_ArgumentTests {
// MARK: - Fixture tests

@Suite(.hidden)
private struct ParameterizedTests {
struct ParameterizedTests {
@Test(.hidden, arguments: ["value"])
func oneParameter(x: String) {}

Expand All @@ -170,4 +170,6 @@ private struct ParameterizedTests {

@Test(.hidden, arguments: ["value": 123])
func oneDictionaryElementTupleParameter(x: (key: String, value: Int)) {}

@Test(.disabled(), arguments: [1, 2, 3]) func disabled(x: Int) {}
}

0 comments on commit aacacd7

Please sign in to comment.