Skip to content

Allow using 'withKnownIssue' as a test or suite trait #1132

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

Open
AnthonyLatsis opened this issue May 28, 2025 · 7 comments
Open

Allow using 'withKnownIssue' as a test or suite trait #1132

AnthonyLatsis opened this issue May 28, 2025 · 7 comments
Assignees
Labels
enhancement New feature or request issue-handling Related to Issue handling within the testing library public-api Affects public API traits Issues and PRs related to the trait subsystem or built-in traits triaged This issue has undergone initial triage

Comments

@AnthonyLatsis
Copy link

AnthonyLatsis commented May 28, 2025

Motivation

withKnownIssue provides granularity, but does not offer a great user experience when the intent is to express an expected failure of a test function because it requires you to modify the entire body, resulting in (much) larger changes/diffs than necessary.

Proposed solution

E.g. a new defaulted parameter for the Test macros. Could simply be a boolean; a more prudent design, I believe, would be to have it accept an enum along the lines of

enum ArgumentsWithKnownIssue<C: Collection> {
  case none, all
  case arguments(C)
}

Alternatives considered

No response

Additional information

No response

@AnthonyLatsis AnthonyLatsis added the enhancement New feature or request label May 28, 2025
@grynspan
Copy link
Contributor

I imagine we'd just implement this as a trait, not introduce a new argument.

@stmontgomery Do you recall offhand why we didn't do this originally? Were we just waiting for scoping?

@stmontgomery stmontgomery added public-api Affects public API issue-handling Related to Issue handling within the testing library traits Issues and PRs related to the trait subsystem or built-in traits labels May 28, 2025
@stmontgomery
Copy link
Contributor

Thanks for filing @AnthonyLatsis! This has indeed come up before, but nobody had yet filed an issue to track the idea.

I agree it would be best structured as a trait. I vaguely recall one reason we didn't originally implement withKnownIssue as a trait was because of difficulty coming up with a name for the static factory method that felt idiomatic. There may also have been a compiler bug preventing the use of closure literals in attached macro expressions at the time, although that has since been resolved.

@stmontgomery stmontgomery added the triaged This issue has undergone initial triage label May 28, 2025
@stmontgomery stmontgomery self-assigned this May 28, 2025
@stmontgomery stmontgomery changed the title Support for XFAILing a @Test without wrapping the entire body in a closure Allow using 'withKnownIssue' as a test or suite trait May 28, 2025
@grynspan
Copy link
Contributor

My gut says we'd be okay limiting this to a blanket .xfail or .xfail("...") and ask developers to just use the function version for the more complex cases. The ergonomics of closures in traits aren't great.

But that's probably a question for the eventual review thread.

@AnthonyLatsis
Copy link
Author

There may also have been a compiler bug preventing the use of closure literals in attached macro expressions at the time

Was the idea for the trait’s closure to accept a generic input? I think the trait variant would work best by optionally accepting a list of inputs that are expected to fail, whereas the purpose of withKnownIssue as I see it is to enable wrapping specific expectations about a particular input along a test function’s flow.

@grynspan
Copy link
Contributor

It is not possible to infer type information for a trait's arguments/etc. based on the type of the function to which the @Test attribute is applied. Hence, it is not possible to build that sort of trait today. That doesn't mean we couldn't in the future with sufficient/hypothetical compiler and/or language changes, but it's where we are today.

@AnthonyLatsis
Copy link
Author

It is not possible to infer type information for a trait's arguments/etc. based on the type of the function to which the @Test attribute is applied. Hence, it is not possible to build that sort of trait today

The same stands for the @Test macros themselves, but that didn’t stop us from having them accept unconstrained collections. Or am I missing the point?

@grynspan
Copy link
Contributor

The same stands for the @Test macros themselves, but that didn’t stop us from having them accept unconstrained collections. Or am I missing the point?

That's a fair question. The difference there is that we can do type-checking on the arguments to @Test at compile time, although the diagnostics you get when type-checking fails aren't great because they happen after macro expansion. Still, you can't write this:

@Test(arguments: 0 ... 10)
func foo(arg: String) { ... }

Because the type checker will notice that 0 ... 10 is not a collection of strings.

Traits don't participate in macro expansion in the same way, so there's no way to statically enforce that the arguments to a function-as-trait are of the correct type. Consider:

@Test(.xfail([1, 2, 3]), arguments: ["a", "b", "c"])
func foo(arg: String) { ... }

There is no static way to intersect [1, 2, 3] with ["a", "b", "c"]. This is ignoring that this would only work with argument types that are Equatable or Identifiable or some such (which is the easy part of the problem to solve because at least that part can be expressed in the signature of func xfail(...).)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request issue-handling Related to Issue handling within the testing library public-api Affects public API traits Issues and PRs related to the trait subsystem or built-in traits triaged This issue has undergone initial triage
Projects
None yet
Development

No branches or pull requests

3 participants