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

New Issue.Kind case for third party expectations. #490

Open
younata opened this issue Jun 14, 2024 · 4 comments · May be fixed by #513
Open

New Issue.Kind case for third party expectations. #490

younata opened this issue Jun 14, 2024 · 4 comments · May be fixed by #513
Labels
enhancement New feature or request public-api Affects public API tools integration Integration of swift-testing into tools/IDEs

Comments

@younata
Copy link

younata commented Jun 14, 2024

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.

Related to #474 & #481

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

@younata younata added the enhancement New feature or request label Jun 14, 2024
@grynspan grynspan added tools integration Integration of swift-testing into tools/IDEs public-api Affects public API labels Jun 15, 2024
@grynspan
Copy link
Contributor

grynspan commented Jun 19, 2024

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
Copy link
Author

younata commented Jun 28, 2024

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 here1.

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.

Footnotes

  1. 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
Copy link
Contributor

grynspan commented Jun 29, 2024

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.

grynspan added a commit that referenced this issue Jun 29, 2024
… test libraries.

This PR adds a new `Issue` kind, `.recordedByTool`, that takes a custom payload provided by a third-party tool or library (e.g. Nimble). This case can then be used to distinguish issues specific to tools while also providing sufficient infrastructural support to allow those tools to distinguish issues they created at later stages of the testing workflow. (If this sounds abstract, it is—the proposed API is meant to be used in a fairly arbitrary fashion by an open set of third-party tools and libraries.)

Resolves #490.
@younata
Copy link
Author

younata commented Jul 1, 2024

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request public-api Affects public API tools integration Integration of swift-testing into tools/IDEs
2 participants