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

Support a string argument to "ignore" describing the reason #102

Closed
ijc opened this issue Jul 19, 2022 · 5 comments · Fixed by #103
Closed

Support a string argument to "ignore" describing the reason #102

ijc opened this issue Jul 19, 2022 · 5 comments · Fixed by #103

Comments

@ijc
Copy link
Contributor

ijc commented Jul 19, 2022

Since Rust 1.61.0 the argument to an ignore attribute has been printed as part of the test output. So:

#[test]
#[ignore = "Some reason"]
fn something() {...}

Will result in:

$ cargo test 
...
test something ... ignored, Some reason

I believe the syntax has been valid for some releases prior to 1.61.0 (I'm afraid I don't know since when) but was ignored.

It would be great if test_case's ignore modifier (and inconclusive I suppose) could similarly take an option reason and propagate it so that cargo test will show it.

I'm not sure that #[test_case(INPUT => ignore = "Some reason" OUTPUT)] will fit the current syntax but perhaps #[test_case(INPUT => ignore("Some reason") OUTPUT)] could work?

@luke-biel
Copy link
Collaborator

Hmm, yeah, it's definitelly something missing. I remember ignore comment syntax was present since 1.3x or earlier, so it shouldn't be a breaking change, we have MSRV 1.49 anyway. It just wasn't thought of.

I like your second proposal, if you have resources and time PR is welcome.

@ijc
Copy link
Contributor Author

ijc commented Jul 20, 2022

I assumed we want the reason to be optional, for backward compat so that existing cases do not all need updating and can still use a bare inconclusive. Given that then using () to surround the reason argument was ambiguous -- since => inconclusive () should mean a test with output () but is treated as one with an empty reason and no output specified. Similar problems exist with => inconclusive ("this is the result") where this is the result ended up as the reason not the expected result...

I went with [] instead and will open a PR in a moment, {} would be an easy change if preferred. Other things (like <>) would need more custom handling.

ijc added a commit to ijc/test-case that referenced this issue Jul 20, 2022
Since Rust 1.61.0 `cargo test` has included the reason string in the output.

The new syntax is optional, so `inconclusive` is still accepted, a reason may
be given inside optional brackets `inconclusive["some reason"]`. Parentheses
are not used to avoid ambiguity with the output/result.

A custom `impl Debug` is used to avoid injecting an unsightly
`inconclusivewithreason_litstr_token` into the test name by default.

Fixes: frondeus#102
@frondeus
Copy link
Owner

What about => inconclusive = "this is reason" ("this is result")? @luke-biel would that be ambiguous or/and better?

@ijc
Copy link
Contributor Author

ijc commented Jul 21, 2022

I think that would be unambiguous? At least I don't think there are any output matchers which start with an = (nor an == FWIW` although I don't think that would be ambiguous even if there was).

I guess it's a question of taste/style and perhaps future direction, happy to defer to you both.

@luke-biel
Copy link
Collaborator

I think I'm leaning towards [] as proposed right now. I don't think that any of these is better, but
=> ignore = "reason" "result" may be a bit confusing.

ijc added a commit to ijc/test-case that referenced this issue Jul 21, 2022
Since Rust 1.61.0 `cargo test` has included the reason string in the output.

The new syntax is optional, so `inconclusive` is still accepted, a reason may
be given inside optional brackets `inconclusive["some reason"]`. Parentheses
are not used to avoid ambiguity with the output/result.

A custom `impl Debug` is used to avoid injecting an unsightly
`inconclusivewithreason_litstr_token` into the test name by default.

Fixes: frondeus#102
ijc added a commit to ijc/test-case that referenced this issue Jul 21, 2022
Since Rust 1.61.0 `cargo test` has included the reason string in the output.

The new syntax is optional, so `inconclusive` is still accepted, a reason may
be given inside optional brackets `inconclusive["some reason"]`. Parentheses
are not used to avoid ambiguity with the output/result.

A custom `impl Debug` is used to avoid injecting an unsightly
`inconclusivewithreason_litstr_token` into the test name by default.

Fixes: frondeus#102
luke-biel pushed a commit that referenced this issue Jul 21, 2022
* Remove unneeded turbofish

It is sufficient to declare the type as the `kw::ignore` case does.

* Support giving a reason to inconclusive/ignored cases

Since Rust 1.61.0 `cargo test` has included the reason string in the output.

The new syntax is optional, so `inconclusive` is still accepted, a reason may
be given inside optional brackets `inconclusive["some reason"]`. Parentheses
are not used to avoid ambiguity with the output/result.

A custom `impl Debug` is used to avoid injecting an unsightly
`inconclusivewithreason_litstr_token` into the test name by default.

Fixes: #102
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants