Show the number of test cases that ran at the end of a test #972

hrayatnia · 2025-02-23T22:10:56Z

Motivation:

To have the number of test cases at the end of the test in for parameterized tests, so the logs don’t easily get buried.

Modifications:

Log the number of test cases for parameterized tests at the end of a test.

Result:

If the user runs the parameterized tests, an extra piece of information (with * test cases) would get bound to the end log.

Resolves #943.
Resolves rdar://144250498.

Checklist:

Code and documentation should follow the style of the Style Guide.
If public symbols are renamed or modified, DocC references should be updated.

… for clarity

grynspan

Thanks for opening this PR! It would be a good idea to familiarize yourself with the existing code in Event.HumanReadableOutputRecorder.swift and get a feel for how it works and the existing mechanisms we use for gathering data during test runs.

grynspan · 2025-02-24T14:08:57Z

Sources/Testing/Events/Recorder/Event.HumanReadableOutputRecorder.swift

-          return " with \(numberOfArguments) " + (numberOfArguments > 1 ? "arguments" : "argument")
+  private func _includeNumberOfTestCases(for test: Test, verbose: Int) -> String {
+    if verbose == 2 && !test.isSuite { // very verbose
+        let testCasesCount =  test.testCases?.count(where: { _ in true }) ?? 0


Calling testCases here will produce a second sequence of test cases, which makes getting the number of test cases an O(n) operation instead of O(1). What we would normally do is keep a running count of test cases in the TestData structure, incrementing it for each .testCaseStarted event.

Thank you, it helps me understand the way you guys think 🙏.

Honestly, it was my first idea; however, I thought it is the same due to the fact that O(n) gets distributed among each test run and also adds a negligible amount of miscalculation to the compute time (because it happens after the start instant as well as it adds 8 bytes of additional data to the test data). For those reasons, I went with the second solution.

I fixed the issue.

grynspan · 2025-02-24T14:09:16Z

Sources/Testing/Events/Recorder/Event.HumanReadableOutputRecorder.swift

-          let numberOfArguments = _numberOfArguments(for: test.testCases)
-          return " with \(numberOfArguments) " + (numberOfArguments > 1 ? "arguments" : "argument")
+  private func _includeNumberOfTestCases(for test: Test, verbose: Int) -> String {
+    if verbose == 2 && !test.isSuite { // very verbose


You probably mean >= 2, right?

And if you want to count test cases, you should use test.isParameterized, not !test.isSuite, since it doesn't make sense to count test cases for a non-parameterized test function either.

grynspan · 2025-02-24T14:10:24Z

Sources/Testing/Events/Recorder/Event.HumanReadableOutputRecorder.swift

+  private func _includeNumberOfTestCases(for test: Test, verbose: Int) -> String {
+    if verbose == 2 && !test.isSuite { // very verbose
+        let testCasesCount =  test.testCases?.count(where: { _ in true }) ?? 0
+        return " with \(testCasesCount) " + (testCasesCount > 1 ? "test cases" : "test case")


We have a helper function for pluralizing English nouns (used elsewhere in this file.)

thanks, applied. 🙏

grynspan · 2025-02-24T14:12:12Z

Sources/Testing/Events/Recorder/Event.HumanReadableOutputRecorder.swift

  ///   - verbose: If the level is very verbose, a detailed description is returned.
  ///
-  /// - Returns: A string describing the number of arguments in the test cases, or an empty string if it's not very verbose level.
+  /// - Returns: A string describing the number of argument(s) in the test cases, or an empty string if it's not very verbose level.


Documentation comments (but not code) should wrap at 80 columns. This helps give readers a sense of how long the documentation will be when rendered with DocC.

Applied, sorry for not double-checking it.

For more details see the Style Guide.

…eedback - use counting instead of ternary operator - fixing char limit per row - fixing verbosity condition check - check if test is parameterized

… of after test ends

grynspan · 2025-02-24T16:49:21Z

Sources/Testing/Events/Recorder/Event.HumanReadableOutputRecorder.swift

+
+      case .testCaseStarted:
+        guard verbosity >= 2 else { break }
+        let id: [String] = if let test {


We don't check verbosity in this first switch intentionally so that the data we collect is always comprehensive. The second switch below checks verbosity before producing any messages.

test will never be nil for this event, and .testStarted will always be recorded before .testCaseStarted, so you can just write:

case .testCaseStarted: let test = test! context.testData[test.id.keyPathRepresentation]?.testCasesCount += 1

grynspan · 2025-02-24T16:50:05Z

Sources/Testing/Events/Recorder/Event.HumanReadableOutputRecorder.swift

+  /// - Returns: A string describing the number of test cases in the test,
+  ///   or an empty string if it's not very verbose level.
+  ///
+  private func _includeNumberOfTestCasesIfNeeded(


You probably don't need this helper function and can just construct the string in place in the .testEnded handler below.

grynspan · 2025-02-24T16:51:46Z

Sources/Testing/Events/Recorder/Event.HumanReadableOutputRecorder.swift

@@ -366,18 +404,21 @@ extension Event.HumanReadableOutputRecorder {
      let testData = testDataGraph?.value ?? .init(startInstant: instant)
      let issues = _issueCounts(in: testDataGraph)
      let duration = testData.startInstant.descriptionOfDuration(to: instant)
+      let testCasesCountMessage = _includeNumberOfTestCasesIfNeeded(test.isParameterized,


Here, you can probably just write:

let testCasesCount = if verbosity >= 2 && test.isParameterized { " with \(testData.testCasesCount.counting("test case"))" } else { "" }

("Message" has a domain-specific meaning in this file, which is why I renamed this variable in my example above.)

… creation

hrayatnia · 2025-02-24T20:53:13Z

@grynspan Thank you for being patience with me. I've applied all the requested changes—please review and resolve if everything looks good. Also, I'll make an effort to participate more in code reviews and go through other pull requests to minimize trivial questions in the future.

@stmontgomery Thank you for your support as well. 🙏

grynspan · 2025-02-24T21:52:07Z

Sources/Testing/Events/Recorder/Event.HumanReadableOutputRecorder.swift

@@ -146,6 +149,7 @@ extension Event.HumanReadableOutputRecorder {

    return (errorIssueCount, warningIssueCount, knownIssueCount, totalIssueCount,  description)
  }
+


Nitpick: stray newline? :)

grynspan · 2025-02-24T21:52:25Z

Tests/TestingTests/EventRecorderTests.swift

@@ -566,3 +610,4 @@ struct EventRecorderTests {
    }
  }
 }
+


Nitpick: stray newline?

grynspan · 2025-02-24T21:52:37Z

@swift-ci test

grynspan · 2025-02-24T22:20:15Z

@swift-ci test

grynspan · 2025-02-25T13:00:55Z

Sources/Testing/Events/Recorder/Event.HumanReadableOutputRecorder.swift

@@ -366,18 +373,23 @@ extension Event.HumanReadableOutputRecorder {
      let testData = testDataGraph?.value ?? .init(startInstant: instant)
      let issues = _issueCounts(in: testDataGraph)
      let duration = testData.startInstant.descriptionOfDuration(to: instant)
+      let testCasesCount = if verbosity >= 2 && test.isParameterized {


@stmontgomery What do you think about the required verbosity here?

I think we should always include this, and not conditionalize it on verbosity. It's only a few extra words, not adding any additional lines, and I think it's a generally useful enhancement. The current summary text always includes a count of the issues recorded regardless of verbosity, and this isn't all that different.

(We should continue to check test.isParameterized, though.)

stmontgomery · 2025-02-25T16:56:46Z

Tests/TestingTests/EventRecorderTests.swift

+      ("f()", #".* Test f\(\) failed after .*"# ,  0),
+      ("f()", #".* Test f\(\) failed after .*"# ,  2),
+      ("d(_:)", #".* Test d\(_:\) with .+ test cases passed after.*"# , 2),
+      ("PredictablyFailingTests", #".* Suite PredictablyFailingTests failed after .*"# , 1),
+      ("PredictablyFailingTests", #".* Suite PredictablyFailingTests failed after .*"# , 2),


nit: whitespace

Suggested change

("f()", #".* Test f\(\) failed after .*"# , 0),

("f()", #".* Test f\(\) failed after .*"# , 2),

("d(_:)", #".* Test d\(_:\) with .+ test cases passed after.*"# , 2),

("PredictablyFailingTests", #".* Suite PredictablyFailingTests failed after .*"# , 1),

("PredictablyFailingTests", #".* Suite PredictablyFailingTests failed after .*"# , 2),

("f()", #".* Test f\(\) failed after .*"#, 0),

("f()", #".* Test f\(\) failed after .*"#, 2),

("d(_:)", #".* Test d\(_:\) with .+ test cases passed after.*"#, 2),

("PredictablyFailingTests", #".* Suite PredictablyFailingTests failed after .*"#, 1),

("PredictablyFailingTests", #".* Suite PredictablyFailingTests failed after .*"#, 2),

stmontgomery · 2025-02-25T19:12:27Z

Sources/Testing/Events/Recorder/Event.HumanReadableOutputRecorder.swift

@@ -62,6 +62,9 @@ extension Event {

        /// The number of known issues recorded for the test.
        var knownIssueCount = 0
+
+        /// The number of test cases for the test.
+        var testCasesCount: Int = 0


nit: can omit the explicit type since this isn't a public property, to match prevailing style

Suggested change

var testCasesCount: Int = 0

var testCasesCount = 0

stmontgomery · 2025-02-25T19:13:57Z

@swift-ci please test

stmontgomery · 2025-02-25T19:16:08Z

Tests/TestingTests/EventRecorderTests.swift

+
+  @available(_regexAPI, *)
+  @Test(
+    "number of arguments based on verbosity level at the end of test run",


The display name of this test can be rephrased now to not mention verbosity. I'd also prefer if we could capitalize the first word to match prevailing style.

stmontgomery

Couple more style comments but besides that I think this is ready to land. Thanks @hrayatnia!

…ter clarity

stmontgomery · 2025-02-25T20:23:47Z

@swift-ci please test

[943] minor change: - show number of args passed to at the end of a test

14184b1

hrayatnia changed the title ~~[943] minor change: - show number of args passed to at the end of a test~~ #943 minor change: - show number of args passed to at the end of a test Feb 23, 2025

hrayatnia changed the title ~~#943 minor change: - show number of args passed to at the end of a test~~ [943] minor change: - show number of args passed to at the end of a test Feb 23, 2025

hrayatnia added 4 commits February 24, 2025 11:35

minor change: - update test cases & excluding suites

a1f7b4b

Rename _includeNumberOfTestCases to _includeNumberOfTestCasesIfNeeded…

928f67b

… for clarity

update the documentation

2a796ec

fix typo in the method name _includeNumberOfTestCasesIfNeeded

16a997e

grynspan requested changes Feb 24, 2025

View reviewed changes

hrayatnia added 2 commits February 24, 2025 15:53

Refactor code to use existing methods and align with document style f…

ea309da

…eedback - use counting instead of ternary operator - fixing char limit per row - fixing verbosity condition check - check if test is parameterized

Update test case count increment to occur during the test run instead…

4592e06

… of after test ends

grynspan reviewed Feb 24, 2025

View reviewed changes

grynspan changed the title ~~[943] minor change: - show number of args passed to at the end of a test~~ Show the number of test cases that ran at the end of a test Feb 24, 2025

grynspan assigned hrayatnia Feb 24, 2025

grynspan added enhancement New feature or request tools integration 🛠️ Integration of swift-testing into tools/IDEs command-line experience ⌨️ enhancements to the command line interface labels Feb 24, 2025

grynspan added this to the Swift 6.x milestone Feb 24, 2025

refactor: remove redundant helper methods and unnecessary test object…

4cf3306

… creation

hrayatnia marked this pull request as ready for review February 24, 2025 20:53

hrayatnia requested review from briancroom, SeanROlszewski and suzannaratcliff as code owners February 24, 2025 20:53

hrayatnia requested a review from grynspan February 24, 2025 20:59

grynspan reviewed Feb 24, 2025

View reviewed changes

Cleanup: Remove unnecessary newline

46d4910

grynspan reviewed Feb 25, 2025

View reviewed changes

stmontgomery reviewed Feb 25, 2025

View reviewed changes

hrayatnia added 3 commits February 25, 2025 18:03

nitpick: remove redundant whitespace in numberOfTestsCasesAtTestEnd

701506c

Remove verbosity limit on testCasesCount and update related unit tests

9616b98

add a minor arguments to numberOfTestsCasesAtTestEnd test

8f61da7

hrayatnia requested review from stmontgomery and grynspan February 25, 2025 18:47

stmontgomery reviewed Feb 25, 2025

View reviewed changes

stmontgomery approved these changes Feb 25, 2025

View reviewed changes

grynspan approved these changes Feb 25, 2025

View reviewed changes

hrayatnia added 2 commits February 25, 2025 20:37

Remove testCasesCount type definition

5c52756

Refactor the test title for numberOfTestCasesAtTestEnd test for bet…

a5bac97

…ter clarity

stmontgomery merged commit f9e1701 into swiftlang:main Feb 25, 2025
3 checks passed

This was referenced Mar 12, 2025

Log test case ended events, including issue counts, when running in verbose mode #1021

Closed

Represent non-encodable test argument values in Test.Case.ID #1000

Merged

		@@ -146,6 +149,7 @@ extension Event.HumanReadableOutputRecorder {

		return (errorIssueCount, warningIssueCount, knownIssueCount, totalIssueCount, description)
		}

@@ @@ -566,3 +610,4 @@ struct EventRecorderTests { @@
                   }
                 }
               }

Show the number of test cases that ran at the end of a test #972

Show the number of test cases that ran at the end of a test #972

Uh oh!

Conversation

hrayatnia commented Feb 23, 2025 • edited by grynspan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation:

Modifications:

Result:

Checklist:

Uh oh!

grynspan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hrayatnia commented Feb 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grynspan commented Feb 24, 2025

Uh oh!

grynspan commented Feb 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stmontgomery commented Feb 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stmontgomery left a comment

Choose a reason for hiding this comment

Uh oh!

stmontgomery commented Feb 25, 2025

Uh oh!

Uh oh!

Uh oh!

hrayatnia commented Feb 23, 2025 •

edited by grynspan

Loading