New Issue.Kind case for third party expectations.

younata commented 3 months ago

Description

Just discussed in a lab.

To better facilitate integrate with third party assertion tools, there should be a dedicated Issue.Kind case for them. Because the current expectationFailed takes in a type that is still being iterated. This new case should take in a new, kind of lightweight version of Expectation so that we don't have to open up Expectation publicly.

One terrible name suggestion: thirdPartyExpectationFailed. Let's not use that, though.

Expected behavior

No response

Actual behavior

No response

Steps to reproduce

No response

swift-testing version/commit hash

No response

Swift & OS version (output of `swift --version && uname -a`)

No response

grynspan commented 2 months ago

Strawman:

/// A protocol describing context provided by a third-party tool when an issue occurs.
///
/// Conforming types can include whatever additional data is appropriate in their context types.
/// A tool may define more than one type conforming to this protocol.
///
/// - Note: Name to be bikeshedded.
protocol Issue.ToolContext: Sendable, Codable {
  /// A human-readable description of the tool (such as its name.)
  var toolDescription: String { get }
}

enum Issue.Kind {
  // ...

  /// A third-party tool recorded an issue.
  ///
  /// - Parameters:
  ///   - description: A brief human-readable description of the issue.
  ///   - context: Additional context for the issue including a description of the tool that recorded it.
  ///
  /// - Note: Name to be bikeshedded.
  case recordedByTool(_ description: String, context: any ToolContext)
}

younata commented 2 months ago

A ToolContext protocol would require additional refactoring to implement, unless we decide to simply drop that context when encoding/decoding Issue.Kind.Snapshot. Additionally, it prevents borrowing the Issue in ABIv0.EncodedIssue.swift here[^borrowing-footnote].

I've simplified this for Nimble's own usecase, to something that simply lets me provide a string. Though, there might be reasons I haven't thought of (I've only spent 30 minutes today getting this to work) to expand this. Unless there's more specific metadata for Swift Testing to consume, taking in and outputting an arbitrary string seems to be the most flexible approach.

enum Issue.Kind {
  /// A third-party tool recorded an issue.
  ///
  /// - Parameters:
  ///   - failureReason: A brief human-readable description of the issue.
  ///
  /// - Note: Name to be bikeshedded.
  case recordedByTool(_ failureReason: String)
}

And again, I still don't have a better suggestion for a name, though this is a better name than what I had.

[^borrowing-footnote]: My understanding of Swift's borrowing/consuming mechanism is still incomplete, but I'm under the impression that they only really do anything when used with a non-copyable type. Since Issue isn't a non-copyable type, using the borrowing keyword with it (as ABIv0.EncodedIssue.swift does) is meaningless, correct?

grynspan commented 2 months ago

A ToolContext protocol would require additional refactoring to implement,

I've actually got a draft PR ready to go here that shows how we can do this using a separate context type. I don't think the refactoring on your end would be major—you just need a smol type to contain whatever additional metadata you'd like to propagate; if there isn't any, it just needs to include Nimble's name:

struct NimbleToolContext: ToolContext {
  var toolName: String { "Nimble" }
}

unless we decide to simply drop that context when encoding/decoding Issue.Kind.Snapshot.

Keep in mind we're actually going to remove Issue.Kind.Snapshot in the future as we're pitching a replacement mechanism that uses JSON as its ABI rather than Swift. The replacement code doesn't generally care about decoding, only encoding, and the encoder can work with an existential.

For tools that directly interface with Swift, they'll of course have access to the Swift value and can conditionally cast it back to their context type if they need to dig into the value on the Swift side.

I've simplified this for Nimble's own usecase, to something that simply lets me provide a string. Though, there might be reasons I haven't thought of (I've only spent 30 minutes today getting this to work) to expand this. Unless there's more specific metadata for Swift Testing to consume, taking in and outputting an arbitrary string seems to be the most flexible approach.

My thinking here is that, while Nimble maybe doesn't need any additional context beyond a description, other tools might want to provide more complex data of arbitrary nature, and we don't want to limit the API design so much that they can't provide it or need to resort to using the string as a payload vector.

Note that Issue already has a comment property which acts as the description, so the tool context associated value is only needed for additional info, not for the basic human-readable string. The interface in my draft PR looks like:

extension Issue {
  /// Record an issue on behalf of a tool or library.
  ///
  /// - Parameters:
  ///   - comment: A comment describing the expectation.
  ///   - toolContext: Any tool-specific context about the issue including the
  ///     name of the tool that recorded it.
  ///   - sourceLocation: The source location to which the issue should be
  ///     attributed.
  ///
  /// - Returns: The issue that was recorded.
  ///
  /// Test authors do not generally need to use this function. Rather, a tool
  /// or library based on the testing library can use it to record a
  /// domain-specific issue and to propagatre additional information about that
  /// issue to other layers of the testing library's infrastructure.
  @discardableResult public static func record(
    _ comment: Comment? = nil,
    context toolContext: some Issue.Kind.ToolContext,
    sourceLocation: SourceLocation = #_sourceLocation
  ) -> Self
}

So you'd write something like:

Issue.record("The fleeble flobbled too flibbiously!", NimbleToolContext())

Does that all make sense?

My understanding of Swift's borrowing/consuming mechanism is still incomplete, but I'm under the impression that they only really do anything when used with a non-copyable type.

It's a hint to both the compiler and the developer that the snapshot and ABIv0 types aren't taking ownership of the issue, which can help with codegen by reducing retain/release traffic. However it is just a hint, and the compiler is free to ignore it for a copyable type.

younata commented 2 months ago

Keep in mind we're actually going to remove Issue.Kind.Snapshot in the future as we're pitching a replacement mechanism that uses JSON as its ABI rather than Swift. The replacement code doesn't generally care about decoding, only encoding, and the encoder can work with an existential.

Oh. That's a critical piece of information that makes this approach doable.

Does that all make sense?

Yes! This does make sense to me, thank you!

It's a hint to both the compiler and the developer that the snapshot and ABIv0 types aren't taking ownership of the issue, which can help with codegen by reducing retain/release traffic. However it is just a hint, and the compiler is free to ignore it for a copyable type.

Fascinating! Thanks for that clarification!

swiftlang / swift-testing