Skip to content

Introduce a severity level for issues, and a 'warning' severity #931

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

Merged
merged 32 commits into from
Feb 11, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
ff6cf12
Introduce a severity level for issues, and a 'warning' severity
stmontgomery Jan 27, 2025
e81ce3d
Improve wording
stmontgomery Jan 27, 2025
8415b90
Add missing documentation to Snapshot property
stmontgomery Jan 28, 2025
11dec4b
Describe the cases in increasing order instead of decreasing
stmontgomery Jan 28, 2025
6715362
Add severity to the ABIv0 JSON, as private initially
stmontgomery Jan 28, 2025
7cdfeb2
We don't need to pass the count of warning issues in the symbol
stmontgomery Jan 28, 2025
ca838a3
Simplify issue count tracking with a dictionary
stmontgomery Jan 28, 2025
04f115f
Simplify compactMap with key path
stmontgomery Jan 28, 2025
a51b385
Replace "a warning issue" with "a warning"
stmontgomery Jan 28, 2025
fba703f
Introduce new Configuration SPI controlling whether warning issues sh…
stmontgomery Jan 29, 2025
cefc62d
Attempt to work around compiler crash
stmontgomery Jan 30, 2025
f224697
PR feedback
stmontgomery Jan 30, 2025
e9c84c3
Fix severity property in Issue not being assigned in initializer, and…
stmontgomery Feb 4, 2025
50a6143
Improve Issue's Custom(Debug)StringConvertible implementations
stmontgomery Feb 4, 2025
b8f81cf
Drive-by fix for private function underscore prefix
stmontgomery Feb 4, 2025
b74187c
Add tests of the v0 entry point, with and without warning issues enab…
stmontgomery Feb 4, 2025
02996d7
Make the fixture @Test function added in the previous commit `.hidden…
stmontgomery Feb 4, 2025
ce1f4b8
Update usages of now-deprecated Configuration property
stmontgomery Feb 4, 2025
5b5bfca
Fixed extra space in output formatting
stmontgomery Feb 4, 2025
c7ff8a2
Adjust and add new console output tests
stmontgomery Feb 4, 2025
51c5327
Generalize the infrastructure for enabling experimental features
stmontgomery Feb 4, 2025
39f8ac6
Make 'includeHiddenTests' property internal since it's only needed by…
stmontgomery Feb 5, 2025
a101445
Add custom decoding logic to gracefully decode Issue.Snapshot instanc…
stmontgomery Feb 5, 2025
d7c8bdc
Revert "Generalize the infrastructure for enabling experimental featu…
stmontgomery Feb 5, 2025
b64d7a3
Avoid needing to serialize the new internal properties on __CommandLi…
stmontgomery Feb 5, 2025
8902a96
Various PR feedback
stmontgomery Feb 7, 2025
1e6e866
Don't use warning symbol for test and run ended events, only for issu…
stmontgomery Feb 7, 2025
95d83ff
Move pending ABI changes writeup to a separate file
stmontgomery Feb 7, 2025
c23a4d6
Adjust symbols and ABI documentation
stmontgomery Feb 7, 2025
9fa497e
Adjust event symbols
stmontgomery Feb 8, 2025
57f2221
PR feedback
stmontgomery Feb 10, 2025
4811f39
Improve style in tests
stmontgomery Feb 11, 2025
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
6 changes: 3 additions & 3 deletions Sources/Testing/ABI/EntryPoints/ABIEntryPoint.swift
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ extension ABIv0 {
/// callback.
public static var entryPoint: EntryPoint {
return { configurationJSON, recordHandler in
try await Testing.entryPoint(
try await _entryPoint(
configurationJSON: configurationJSON,
recordHandler: recordHandler
) == EXIT_SUCCESS
Expand Down Expand Up @@ -87,7 +87,7 @@ typealias ABIEntryPoint_v0 = @Sendable (
@usableFromInline func copyABIEntryPoint_v0() -> UnsafeMutableRawPointer {
let result = UnsafeMutablePointer<ABIEntryPoint_v0>.allocate(capacity: 1)
result.initialize { configurationJSON, recordHandler in
try await entryPoint(
try await _entryPoint(
configurationJSON: configurationJSON,
eventStreamVersionIfNil: -1,
recordHandler: recordHandler
Expand All @@ -104,7 +104,7 @@ typealias ABIEntryPoint_v0 = @Sendable (
///
/// This function will be removed (with its logic incorporated into
/// ``ABIv0/entryPoint-swift.type.property``) in a future update.
private func entryPoint(
private func _entryPoint(
configurationJSON: UnsafeRawBufferPointer?,
eventStreamVersionIfNil: Int? = nil,
recordHandler: @escaping @Sendable (_ recordJSON: UnsafeRawBufferPointer) -> Void
Expand Down
37 changes: 36 additions & 1 deletion Sources/Testing/ABI/EntryPoints/EntryPoint.swift
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ private import _TestingInternals
/// writes events to the standard error stream in addition to passing them
/// to this function.
///
/// - Returns: An exit code representing the result of running tests.
///
/// External callers cannot call this function directly. The can use
/// ``ABIv0/entryPoint-swift.type.property`` to get a reference to an ABI-stable
/// version of this function.
Expand All @@ -40,7 +42,7 @@ func entryPoint(passing args: __CommandLineArguments_v0?, eventHandler: Event.Ha

// Set up the event handler.
configuration.eventHandler = { [oldEventHandler = configuration.eventHandler] event, context in
if case let .issueRecorded(issue) = event.kind, !issue.isKnown {
if case let .issueRecorded(issue) = event.kind, !issue.isKnown, issue.severity >= .error {
exitCode.withLock { exitCode in
exitCode = EXIT_FAILURE
}
Expand Down Expand Up @@ -270,6 +272,13 @@ public struct __CommandLineArguments_v0: Sendable {
/// The value(s) of the `--skip` argument.
public var skip: [String]?

/// Whether or not to include tests with the `.hidden` trait when constructing
/// a test filter based on these arguments.
///
/// This property is intended for use in testing the testing library itself.
/// It is not parsed as a command-line argument.
var includeHiddenTests: Bool?

/// The value of the `--repetitions` argument.
public var repetitions: Int?

Expand All @@ -278,6 +287,13 @@ public struct __CommandLineArguments_v0: Sendable {

/// The value of the `--experimental-attachments-path` argument.
public var experimentalAttachmentsPath: String?

/// Whether or not the experimental warning issue severity feature should be
/// enabled.
///
/// This property is intended for use in testing the testing library itself.
/// It is not parsed as a command-line argument.
var isWarningIssueRecordedEventEnabled: Bool?
}

extension __CommandLineArguments_v0: Codable {
Expand Down Expand Up @@ -517,6 +533,9 @@ public func configurationForEntryPoint(from args: __CommandLineArguments_v0) thr
filters.append(try testFilter(forRegularExpressions: args.skip, label: "--skip", membership: .excluding))

configuration.testFilter = filters.reduce(.unfiltered) { $0.combining(with: $1) }
if args.includeHiddenTests == true {
configuration.testFilter.includeHiddenTests = true
}

// Set up the iteration policy for the test run.
var repetitionPolicy: Configuration.RepetitionPolicy = .once
Expand Down Expand Up @@ -547,6 +566,22 @@ public func configurationForEntryPoint(from args: __CommandLineArguments_v0) thr
configuration.exitTestHandler = ExitTest.handlerForEntryPoint()
#endif

// Warning issues (experimental).
if args.isWarningIssueRecordedEventEnabled == true {
configuration.eventHandlingOptions.isWarningIssueRecordedEventEnabled = true
} else {
switch args.eventStreamVersion {
Copy link
Contributor

Choose a reason for hiding this comment

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

For now, until we've proposed the change list for ABIv1, I think we should just leave this switch out. It'll be easy to add back in v1 (actually, there's a whole different way we can do it that may be more scalable, I'll discuss with you in DMs.)

I say this because somebody passing 1 here is either from the future or is providing what is currently invalid input.

Copy link
Contributor

Choose a reason for hiding this comment

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

See also my comment here: https://github.com/swiftlang/swift-testing/pull/931/files#r1944900283

I realize I'm being confusing, sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replied there, and in the mean time we need this so the tests can opt-in.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't the tests passing isWarningIssueRecordedEventEnabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This switch case doesn't actually enable the feature for ABIv1, it ensures that it remains disabled for ABIv0. We definitely want that safeguard, even if we decide not to enable this feature in ABIv1.

Copy link
Contributor

Choose a reason for hiding this comment

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

As of right now, it's dead code since the default value is always false though.

case .some(...0):
// If the event stream version was explicitly specified to a value < 1,
// disable the warning issue event to maintain legacy behavior.
configuration.eventHandlingOptions.isWarningIssueRecordedEventEnabled = false
default:
// Otherwise the requested event stream version is ≥ 1, so don't change
// the warning issue event setting.
break
}
}

return configuration
}

Expand Down
19 changes: 19 additions & 0 deletions Sources/Testing/ABI/v0/Encoded/ABIv0.EncodedIssue.swift
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,19 @@ extension ABIv0 {
/// assists in converting values to JSON; clients that consume this JSON are
/// expected to write their own decoders.
struct EncodedIssue: Sendable {
/// An enumeration representing the level of severity of a recorded issue.
///
/// For descriptions of individual cases, see ``Issue/Severity-swift.enum``.
enum Severity: String, Sendable {
case warning
case error
}

/// The severity of this issue.
///
/// - Warning: Severity is not yet part of the JSON schema.
var _severity: Severity

/// Whether or not this issue is known to occur.
var isKnown: Bool

Expand All @@ -33,6 +46,11 @@ extension ABIv0 {
var _error: EncodedError?

init(encoding issue: borrowing Issue, in eventContext: borrowing Event.Context) {
_severity = switch issue.severity {
case .warning: .warning
case .error: .error
}

isKnown = issue.isKnown
sourceLocation = issue.sourceLocation
if let backtrace = issue.sourceContext.backtrace {
Expand All @@ -48,3 +66,4 @@ extension ABIv0 {
// MARK: - Codable

extension ABIv0.EncodedIssue: Codable {}
extension ABIv0.EncodedIssue.Severity: Codable {}
3 changes: 3 additions & 0 deletions Sources/Testing/ABI/v0/Encoded/ABIv0.EncodedMessage.swift
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ extension ABIv0 {
case `default`
case skip
case pass
case passWithWarnings = "_passWithWarnings"
case passWithKnownIssue
case fail
case difference
Expand All @@ -44,6 +45,8 @@ extension ABIv0 {
} else {
.pass
}
case .passWithWarnings:
.passWithWarnings
case .fail:
.fail
case .difference:
Expand Down
5 changes: 1 addition & 4 deletions Sources/Testing/Events/Event.swift
Original file line number Diff line number Diff line change
Expand Up @@ -290,10 +290,7 @@ extension Event {
if let configuration = configuration ?? Configuration.current {
// The caller specified a configuration, or the current task has an
// associated configuration. Post to either configuration's event handler.
switch kind {
case .expectationChecked where !configuration.deliverExpectationCheckedEvents:
break
default:
if configuration.eventHandlingOptions.shouldHandleEvent(self) {
configuration.handleEvent(self, in: context)
}
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,8 @@ extension Event.Symbol {
return "\(_ansiEscapeCodePrefix)90m\(symbolCharacter)\(_resetANSIEscapeCode)"
}
return "\(_ansiEscapeCodePrefix)92m\(symbolCharacter)\(_resetANSIEscapeCode)"
case .passWithWarnings:
return "\(_ansiEscapeCodePrefix)93m\(symbolCharacter)\(_resetANSIEscapeCode)"
case .fail:
return "\(_ansiEscapeCodePrefix)91m\(symbolCharacter)\(_resetANSIEscapeCode)"
case .warning:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,9 @@ extension Event {
/// The instant at which the test started.
var startInstant: Test.Clock.Instant

/// The number of issues recorded for the test.
var issueCount = 0
/// The number of issues recorded for the test, grouped by their
/// level of severity.
var issueCount: [Issue.Severity: Int] = [:]

/// The number of known issues recorded for the test.
var knownIssueCount = 0
Expand Down Expand Up @@ -114,27 +115,36 @@ extension Event.HumanReadableOutputRecorder {
/// - graph: The graph to walk while counting issues.
///
/// - Returns: A tuple containing the number of issues recorded in `graph`.
private func _issueCounts(in graph: Graph<String, Event.HumanReadableOutputRecorder._Context.TestData?>?) -> (issueCount: Int, knownIssueCount: Int, totalIssueCount: Int, description: String) {
private func _issueCounts(in graph: Graph<String, Event.HumanReadableOutputRecorder._Context.TestData?>?) -> (errorIssueCount: Int, warningIssueCount: Int, knownIssueCount: Int, totalIssueCount: Int, description: String) {
guard let graph else {
return (0, 0, 0, "")
return (0, 0, 0, 0, "")
}
let issueCount = graph.compactMap(\.value?.issueCount).reduce(into: 0, +=)
let errorIssueCount = graph.compactMap(\.value?.issueCount[.error]).reduce(into: 0, +=)
let warningIssueCount = graph.compactMap(\.value?.issueCount[.warning]).reduce(into: 0, +=)
let knownIssueCount = graph.compactMap(\.value?.knownIssueCount).reduce(into: 0, +=)
let totalIssueCount = issueCount + knownIssueCount
let totalIssueCount = errorIssueCount + warningIssueCount + knownIssueCount

// Construct a string describing the issue counts.
let description = switch (issueCount > 0, knownIssueCount > 0) {
case (true, true):
let description = switch (errorIssueCount > 0, warningIssueCount > 0, knownIssueCount > 0) {
case (true, true, true):
" with \(totalIssueCount.counting("issue")) (including \(warningIssueCount.counting("warning")) and \(knownIssueCount.counting("known issue")))"
case (true, false, true):
" with \(totalIssueCount.counting("issue")) (including \(knownIssueCount.counting("known issue")))"
case (false, true):
case (false, true, true):
" with \(warningIssueCount.counting("warning")) and \(knownIssueCount.counting("known issue"))"
case (false, false, true):
" with \(knownIssueCount.counting("known issue"))"
case (true, false):
case (true, true, false):
" with \(totalIssueCount.counting("issue")) (including \(warningIssueCount.counting("warning")))"
case (true, false, false):
" with \(totalIssueCount.counting("issue"))"
case(false, false):
case(false, true, false):
" with \(warningIssueCount.counting("warning"))"
case(false, false, false):
""
}

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

Expand Down Expand Up @@ -267,7 +277,8 @@ extension Event.HumanReadableOutputRecorder {
if issue.isKnown {
testData.knownIssueCount += 1
} else {
testData.issueCount += 1
let issueCount = testData.issueCount[issue.severity] ?? 0
testData.issueCount[issue.severity] = issueCount + 1
}
context.testData[id] = testData

Expand Down Expand Up @@ -355,15 +366,15 @@ extension Event.HumanReadableOutputRecorder {
let testData = testDataGraph?.value ?? .init(startInstant: instant)
let issues = _issueCounts(in: testDataGraph)
let duration = testData.startInstant.descriptionOfDuration(to: instant)
return if issues.issueCount > 0 {
return if issues.errorIssueCount > 0 {
CollectionOfOne(
Message(
symbol: .fail,
stringValue: "\(_capitalizedTitle(for: test)) \(testName) failed after \(duration)\(issues.description)."
)
) + _formattedComments(for: test)
} else {
[
[
Message(
symbol: .pass(knownIssueCount: issues.knownIssueCount),
stringValue: "\(_capitalizedTitle(for: test)) \(testName) passed after \(duration)\(issues.description)."
Expand Down Expand Up @@ -400,13 +411,19 @@ extension Event.HumanReadableOutputRecorder {
""
}
let symbol: Event.Symbol
let known: String
let subject: String
if issue.isKnown {
symbol = .pass(knownIssueCount: 1)
known = " known"
subject = "a known issue"
} else {
symbol = .fail
known = "n"
switch issue.severity {
case .warning:
symbol = .passWithWarnings
subject = "a warning"
case .error:
symbol = .fail
subject = "an issue"
}
}

var additionalMessages = [Message]()
Expand Down Expand Up @@ -435,13 +452,13 @@ extension Event.HumanReadableOutputRecorder {
let primaryMessage: Message = if parameterCount == 0 {
Message(
symbol: symbol,
stringValue: "\(_capitalizedTitle(for: test)) \(testName) recorded a\(known) issue\(atSourceLocation): \(issue.kind)",
stringValue: "\(_capitalizedTitle(for: test)) \(testName) recorded \(subject)\(atSourceLocation): \(issue.kind)",
conciseStringValue: String(describing: issue.kind)
)
} else {
Message(
symbol: symbol,
stringValue: "\(_capitalizedTitle(for: test)) \(testName) recorded a\(known) issue with \(parameterCount.counting("argument")) \(labeledArguments)\(atSourceLocation): \(issue.kind)",
stringValue: "\(_capitalizedTitle(for: test)) \(testName) recorded \(subject) with \(parameterCount.counting("argument")) \(labeledArguments)\(atSourceLocation): \(issue.kind)",
conciseStringValue: String(describing: issue.kind)
)
}
Expand Down Expand Up @@ -498,7 +515,7 @@ extension Event.HumanReadableOutputRecorder {
let runStartInstant = context.runStartInstant ?? instant
let duration = runStartInstant.descriptionOfDuration(to: instant)

return if issues.issueCount > 0 {
return if issues.errorIssueCount > 0 {
[
Message(
symbol: .fail,
Expand Down
16 changes: 14 additions & 2 deletions Sources/Testing/Events/Recorder/Event.Symbol.swift
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,14 @@ extension Event {
/// The symbol to use when a test passes.
///
/// - Parameters:
/// - knownIssueCount: The number of known issues encountered by the end
/// of the test.
/// - knownIssueCount: The number of known issues recorded for the test.
/// The default value is `0`.
case pass(knownIssueCount: Int = 0)

/// The symbol to use when a test passes with one or more warnings.
@_spi(Experimental)
case passWithWarnings

/// The symbol to use when a test fails.
case fail

Expand Down Expand Up @@ -62,6 +66,8 @@ extension Event.Symbol {
} else {
("\u{10105B}", "checkmark.diamond.fill")
}
case .passWithWarnings:
("\u{100123}", "questionmark.diamond.fill")
case .fail:
("\u{100884}", "xmark.diamond.fill")
case .difference:
Expand Down Expand Up @@ -122,6 +128,9 @@ extension Event.Symbol {
// Unicode: HEAVY CHECK MARK
return "\u{2714}"
}
case .passWithWarnings:
// Unicode: QUESTION MARK
return "\u{003F}"
case .fail:
// Unicode: HEAVY BALLOT X
return "\u{2718}"
Expand Down Expand Up @@ -157,6 +166,9 @@ extension Event.Symbol {
// Unicode: SQUARE ROOT
return "\u{221A}"
}
case .passWithWarnings:
// Unicode: QUESTION MARK
return "\u{003F}"
case .fail:
// Unicode: MULTIPLICATION SIGN
return "\u{00D7}"
Expand Down
Loading