debug_assertion validations #1797
Conversation
pksunkara
changed the title
| - name: Release profile tests | ||
| script: | ||
| - cargo test --release --features yaml unstable |
There was a problem hiding this comment.
Why do we need this?
There was a problem hiding this comment.
This will skip cfg(debug_assertions) and helped me catch a few issues with our tests.
There was a problem hiding this comment.
But why do we need to skip them?
There was a problem hiding this comment.
These tests behave differently for normal execution and under release profile execution. I want to introduce testing both behaviour by doing:
#[test]
#[cfg_attr(debug_assertions, should_panic = "..."))]
But it's a step for later. This is setting up for it.
There was a problem hiding this comment.
It seems I confused you, sorry. My question is: why do you need to run tests in release in the first place? You see, if test are run only in debug mode, there's no "different behavior" problem. What does the release pass gives us on it's own and why can't we ditch it?
There was a problem hiding this comment.
I think those who release without proper testing deserve what they get. The desired
we are not entitled here to pass judgement on how people go about their dev process. Kindly refrain from making such comments in the future.
There was a problem hiding this comment.
No, not about debug and release versions having the same behaviour, it's about them having defined behaviour.
There was a problem hiding this comment.
But what is the defined behavior? I wouldn't try to define behavior in cases when user makes mistakes, like giving the same name for an arg and a group. If they do so but skip checks, they harvest what they sow (likely wrong behavior or indescriptive panics, like unwrapped on None); this is expected and we shouldn't encourage it.
In case the user did everything right, release and debug builds must behave identically, and this is the only reason I see to run release tests. Otherwise it's useless because if we are sure they are the same, just one run is enough. Anyway, this is not really important since we don't pay for the extra time wasted on CI; I'm just trying to decipher your logic 🙂
There was a problem hiding this comment.
Defined behaviour will be changing on case-to-case. Some of them just silently fail right now or show error messages which doesn't make sense. Some of them lead to panic. And this is a setup to start testing those too.
There was a problem hiding this comment.
Waste of time if you ask me: ways to make an error are countless. We can't hope to catch all of them; the select ones we do cover are served by debug builds.
A user:
- did everything right - clap works, sun shines, angels sing.
- made a mistake (some action that's at disagreement with our documentation) and checked with debug build - they get nice descriptive error.
- made a mistake but didn't make use of the debug check - they are on they own, no guarantees here.
I honestly don't see how release builds help anyone.
|
Replied to your comments. Go ahead and resolve conversations which you feel has satisfied your comments. The only actionable comments are |
I think it used to be a |
Precisely. This is why |
|
Ah, I missed that part. That would work. Could you create an issue please? Thanks. |
|
bors r+ |
Build succeeded |
Closes 5 issues. And provides the way we will be doing validations for programmer errors.
debug_assertionswill be enabled when no optimizations exist. So, when people are usingclapand use a test for it, this panicking happen. But if they build a release to distribute binary, this would not happen. In fact, the checks would not even occur making it more performant.