-
Notifications
You must be signed in to change notification settings - Fork 711
testsuite/README: document fails
combinator
#7709
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks accurate.
The CI failure is due to #7707. |
cabal-testsuite/README.md
Outdated
To create a test that is supposed to fail, there is the `fails` combinator, e.g.: | ||
```haskell | ||
main = cabalTest $ | ||
fails $ cabal "bad-command" [ "bad", "args" ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the test is supposed to fail, but the command is expected to have a non-zero exit-code. The interesting thing with this combinator is that you can interleave succeeding commands and failing commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fendor: Changed this. Let me know whether you think it is more representative now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it shouldn't mention "tests that are supposed to fail" (= known failures, bugs) at all, since for those there's expectBroken
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since for those there's
expectBroken
Oh, I missed that. Certainly it's worth spelling out the difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it shouldn't mention "tests that are supposed to fail" (= known failures, bugs) at all, since for those there's
expectBroken
So rather:
To create a test for a command that is supposed to fail,
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fail is probably a strong word. I suggest being pedantic and say, "that is supposed to have a non-zero exit code".
Thank you for taking the time to update the test docs, btw ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fendor: Here is a new formulation.
Btw, I was not aiming at maximal precision, but to give posteriority a hint how to construct a failing test case, so that they do not have to research it from scratch themselves.
Aren't we all wasting our time a bit here in a super-strict PR procedure (with two approvals needed) for a small snippet of developer-only documentation?
I'd say my (modest) contribution on fails
is already better (even in its original form) than what was there, namely nothing; and if folks feel they want to even improve it in their own PRs, I can only say "YES!!, be my guest!", because I would like to see more documentation on how to write tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm definitely not complaining that I'm wasting my time: I've learned about the difference between fail
and expectBroken
(though I'd love to learn still a bit more about that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one thing i dislike about the PR workflow... it isn't easy to make a small change without tedious back-and-forth because there's a shared branch, and doing it separately requires the rather big overhead of another PR.
I have no solution to this, it's just a rant
78f85dd
to
08d2ca3
Compare
version 3 of that little snippet
08d2ca3
to
d07c435
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
testsuite/README: document
fails
combinator