Support for validating the content of recorded Issues

[dabrahams]

trafficstars

Description

Users shouldn't have to concoct their own versions of this facility. You're going to need it for your own tests anyway; it should be exposed.

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

[stmontgomery]

Thanks @dabrahams. swift-testing currently has a set of APIs named withKnownIssue which are meant to serve this purpose. See documentation at:

https://swiftpackageindex.com/apple/swift-testing/0.1.0/documentation/testing/expectthrows#recording-known-issues-in-tests

Can you check whether withKnownIssue meets your needs? If not, I'd love to know what functionality it's missing.

[dabrahams]

I think while those might do what I am asking for (I am not sure), their names certainly imply that these issues are supposed to get fixed at some point. The facility I'm talking about is expressly for testing that some testing facility works, and should reflect the intent that the proper operation of that facility under the circumstances is to report a test failure. The intention expressed by a piece of test code is important, and I would say withKnownIssue expresses the wrong thing to a reader.

[stmontgomery]

Any chance you could provide a small example of the usage scenario you have in mind? Just to help us understand your use case a bit better. Are you attempting to test a library which provides testing utilities of its own?

[dabrahams]

@stmontgomery Did you look at the README on the page I linked to? I think it answers all those questions and indeed describes some behavior you might want for withKnownIssue as well. Is there some information missing from the README that I can add?

You can see more robust examples of where this function is useful in these tests of the StandardLibraryProtocolChecks package.

[grynspan]

Do you have a specific interface in mind here, @dabrahams?

[dabrahams]

Not really; I haven't internalized your interface style.

[stmontgomery]

Thank you. I think we understand the use case better here, now. We think it could be useful to improve this area, but consider this fairly niche and not a priority for the project while it's still in an early stage of development. We will hold this Issue for further consideration, and in the mean time, if you would like to propose a specific SPI or API enhancement along these lines, we can consider it further. I'll also re-title this issue to try to clarify how it's different than a general-purpose "expected failure" API, which for most users is withKnownIssue.

For now, we do not plan to take action on this ourselves, but I do think withKnownIssue may suit your needs in the short term, regardless of its name.

[grynspan]

If we extend what we've done with #expect(throws:), it might be natural to have:

#expect {
  ...
} records: { issue in
  ...
}

As an even more general form of:

#expect {
  ...
} throws: { error in
  ...
}

I do worry that we're creating too many similar-but-subtly-different interfaces, but maybe I worry too much?

[SeanROlszewski]

Presumably, the second block is to codify their expectations against the issue?

#expect {
  thisThrowsAnErrorAndIWantThat()
} records: { issue in
  #expect(issue.value == "whatever")
}

[grynspan]

The second closure would return true if the issue matched what was expected to be recorded, and false for other issues. This is how #expect {} throws: {} handles errors. So you'd have the opportunity to validate the issue was what you intended to record.

Alternatively, we could maybe introduce something like:

withKnownIssue(intentional: true) { ... }

Which would behave like withKnownIssue() today, but would not record an XFAIL state for intentional issues.

[SeanROlszewski]

I think the variant with the second closure is both easier to understand and more flexible, since someone could author an arbitrary boolean expression to evaluate the issue, which is more useful than always saying, "This should produce an issue." We can also automatically fail the test if it didn't produce an issue.

[grynspan]

withKnownIssue() also takes an (optional) second closure for examining the issue and matching it in the same fashion. :)

[dabrahams]

withKnownIssue(intentional: true) { ... }

FWIW, linguistically this doesn't read like an expression of what I'm doing when I'm testing my testing code. At the level of this test, there is no “issue” unless the test fails.

Actually I'd go further than that; even for its originally intended use this name doesn't work well. “Issue” tends to imply somebody filed a bug report somewhere and that's not necessarily the case. Also, with is inappropriate here; it appears to just have been cargo culted from other names of things that run closures. Normally withXXX(foo) means “prepare XXX and pass it to foo.” Something very imperative like failUnlessATestFails(in:) would be a big improvement. If you want to build higher-level ideas on that like "known issue," you can, or you can leave that to users.

[grynspan]

I don't think that name would be particularly idiomatic Swift. ☹️ We already have withKnownIssue() (and the name, we think, is appropriate for its existing/intended uses) and I'm bikeshedding ways we can avoid expanding the API surface here for what is a relatively niche use case. I take it you'd prefer #expect {} records: {}?

[dabrahams]

Once upon the time I took a lead role in the invention of what “idiomatic Swift” means. Part of that was a break with a past that included the use of naming patterns based on trivial surface similarities rather than semantic content. withKnownIssue very much appears to be that; if that's now idiomatic Swift it's a bit sad. I don't know why you think the other name is non-idiomatic, but 🤷‍♂️

I take it you'd prefer #expect {} records: {}?

I'm sorry, I don't know what that means. I'm not familiar with Swift macros or how their APIs are described or what that incantation is supposed to do. I can't really offer an opinion without more information.

[grynspan]

Oh, it's the same as function invocation syntax here (just drop the pound character.) The first trailing closure is the body of code to which the expectation is applied; the second trailing closure would be invoked upon an issue being recorded to give the caller a chance to inspect it and say "this is actually what I expected would happen, not an arbitrary failure." For a usage example with the existing #expect {} throws: {} macro, see here.

I had offered it as a possible way to implement the requested enhancement earlier in this thread, as opposed to augmenting withKnownIssue().

[dabrahams]

Oof, multiple trailing closures; another innovation I'm not very familiar with. To me it's very strange to have sentence connectors mixed with long multiline blocks. IIUC, I suspect the throws clause would tend to be much shorter, so I would prefer something like #expect error: { } in: { } or #expect errorMatching: { } in: { }

That also seems better to me than order you suggested because you get a clue that the block to be tested is expected to throw before you read it and wonder why it looks erroneous.

[grynspan]

Unfortunately that isn't valid Swift syntax. Only the second (and onward) trailing closure gets a label. The first trailing closure is unlabelled.

[dabrahams]

Then please make the obvious adjustments #expectError {} in: {} or #expectErrorMatching: { } in: { }

[grynspan]

Thanks for the suggestions. The team will take a look at the API and see if there are areas where we can improve it.

[grynspan]

Tracked internally by rdar://137211023.

[grynspan]

This may be of interest to you, Dave: #780

[grynspan]

i.e. we could extrapolate from there to:

let issue = #expect(records: Issue.self) {
  ...
}
guard case let .expectationFailed(expectation) = issue?.kind else {
  // TBD: failure mode--really, `case let` ought to be an expression
}
#expect(expectation.whatever)