Add support for “known failures” to transcripts #5394
Conversation
Shows that unisonweb#5337 is _not_ fixed.
This is basically the opposite of `:failure` – it indicates that a successful result is incorrect. The correct result may either be an error (which will be caught by the transcript runner) or a different result then what is currently expected (which won’t be caught by the runner, but the appearance of `:incorrect` conveys to the programmer that the output.md diff may be an improvement from the previous state).
Shows that unisonweb#5178 is _not_ fixed.
sellout
force-pushed
the
transcript-known-failures
branch
from
9126ca4 to
56a2f5c
Compare
|
I wanna think a little more about what the label names should be, because I think |
|
I agree. It might be worth doing #5214 before this. |
|
How about:
And then also if it's idempotent, we want to preserve any unsuccessful input or unprocessed. |
These aren’t run by check.sh, so I missed them before pushing.
|
Ok, I’ve switched to |
| @@ -4,7 +4,7 @@ When an expected error is not encountered in a `unison :hide:all:error` block | |||
| then the transcript parser should print the stanza | |||
| and surface a helpful message. | |||
|
|
|||
| ``` unison :hide:all:error | |||
| ``` unison :hide:all :error | |||
There was a problem hiding this comment.
Just curious, why does :error get a space but :all doesn't?
There was a problem hiding this comment.
Because :hide:all is really just one tag. It’s never been parsable as :hide :all. I just changed the printer to put spaces between, but didn’t modify the parsers.
Might be worth renaming to :hide-all (perhaps allowing :hide:all still for backward compat).
There was a problem hiding this comment.
I just added a commit with this change, I can back it out if you prefer.
sellout
force-pushed
the
transcript-known-failures
branch
from
5e13ab4 to
523256c
Compare
The former was confusing because `:hide` and `:all` are not actually separate tags.
sellout
force-pushed
the
transcript-known-failures
branch
from
523256c to
49a0cae
Compare
| "The stanza above previously had an incorrect successful result, but now fails with" | ||
| <> "\n" | ||
| <> Pretty.border 2 msg | ||
| <> "\n" | ||
| <> "if this is the expected result, remove `:bug`, otherwise remove `:error`." |
There was a problem hiding this comment.
I think having four terms is too many, I wonder if we can do it with no more than two.
Yes to "is the error the error we're actually expecting?" that's where I was heading with the "open a new ticket if needed".
I'd be satisfied with this, wdyt:
| "The stanza above previously had an incorrect successful result, but now fails with" | |
| <> "\n" | |
| <> Pretty.border 2 msg | |
| <> "\n" | |
| <> "if this is the expected result, remove `:bug`, otherwise remove `:error`." | |
| "The stanza above marked with `:error :bug` is now failing with" | |
| <> "\n" | |
| <> Pretty.border 2 msg | |
| <> "\n" | |
| <> "so you can remove `:bug`, close any appropriate Github issues by using the commit message or PR description, and/or open a new issue if the error message is still not correct." |
There was a problem hiding this comment.
Oh, yeah – mentioning closing the related issue is important! I’ll do the same on the the other :bug case. And yeah, this message looks good to me.
I think there’s still four terms, but different axes, and ones that match our tags better: passing/failing (:error) and expected/unexpected (:bug). So yeah, I like that direction.
There was a problem hiding this comment.
Ok, I changed the phrasing a bit again, so see what you think in the latest commits.
I also realized that the only tests added were for actual outstanding bugs, so I added an additional transcript that will continue testing the “positive” cases after those outstanding bugs are fixed, as well as two error tests for fixed bugs (which show the messages we’ve been fiddling with).
These exposed a few issues with the output formatting, so those are also addressed here.
Overview
This allows issues that haven’t yet been resolved to be reflected in the transcripts. This makes it easier to close tickets as soon as issues have been fixed, and avoids having to manually re-try transcripts from a ticket to see if the situation is the same.
Fixes #5350.
Implementation notes
This adds two new transcript tags
:failureand:incorrect. They behave similarly to:errorand no tag, respectively, except the error messages are different when the results don’t match.:failurerepresents a stanza that is intended to succeed, but currently fails (whereas:erroris intended to fail):incorrectrepresents a stanza that is intended to error, but currently doesn’t.Test coverage
This adds three new transcripts for open tickets exercising the two new tags.
Loose ends
There are lots of open tickets that should also get transcripts added.