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

Feature Request: Automatically exclude the final line of a #[should_panic] test #279

Open
fuerstenau opened this issue Dec 2, 2018 · 6 comments

Comments

@fuerstenau
Copy link

In Rust, it is possible to mark tests as #[should_panic], meaning that (surprise!) the test should panic. However, when this intended behaviour takes its course, it will prevent the test function from returning. This is counted by kcov as a failure to hit the final line, unless that line is marked with LCOV_EXCL_LINE. As specified in another issue, this presumably also applies to functions marked as unreachable!.

Desirable behaviour

Lines that should obviously be excluded from coverage are automatically excluded.

Current behaviour

We have to specify the obvious and automatically-detectable manually.

Urgency

Rather low.

@SimonKagstrom
Copy link
Owner

It should be possible to do this via passing --exclude-line='#[should_panic]' as an option to kcov, but I suppose [#should_panic] could be a default exclusion as well. 32eda27 implements that particular feature, although I'm not completely sure I understood the proper behavior.

As for unreachable!, it's perhaps slightly more likely to be a valid comment in a non-Rust language, so I'm not as sure about that one :-)

@fuerstenau
Copy link
Author

I'm not completely sure I understood the proper behavior.

I'll try to provide a better explanation then. The problem is unfortunately in the last line of the test. Example:

#[test] // Already not considered to be a relevant line by kcov
#[should_panic] // Already not considered to be a relevant line by kcov
fn panics() { // This line is hit
    panic!(); // This line is hit
} // This line is unreachable by design and currently needs to be excluded by hand

We will need at least a parser for the context-free grammar of matching braces to tackle this, a simple regex-based exclusion won't do for that one. (I'm sorry. I was aware of that but failed to explicitly specify this in my feature request.)

As for unreachable!, it's perhaps slightly more likely to be a valid comment in a non-Rust language, so I'm not as sure about that one :-)

Follow-up feature request: Language-dependent exclusion patterns?

@SimonKagstrom
Copy link
Owner

OK, then I misunderstood, thanks for the example!

Then the task is slightly bigger :-). kcov currently doesn't know anything about the language being covered (except for Python and Bash, but those work in a completely different way), so it would be some work just adding that. I guess looking at the filename would be a good enough hint in 99% of the cases though.

Do you know why Rust generates code for the last line? With C++, I believe these things are typically generated by exception handling, and perhaps something similar is valid for Rust? Otherwise I think Rust should be able to determine that everything after panic is dead code and eliminate the remainder of the function.

@fuerstenau
Copy link
Author

I would have assumed these processes to be similar to those of C++ but I do not actually have any clue about this. But even if we had dead code elimination for this, it would not solve the problem: For those cases where the compiler can detect that this will panic it would hardly be necessary to actually run the test. The entire thing could be optimized to simply return “Test succeeded.” without executing any of the code.

@fuerstenau
Copy link
Author

kcov currently doesn't know anything about the language being covered (except for Python and Bash, but those work in a completely different way), so it would be some work just adding that. I guess looking at the filename would be a good enough hint in 99% of the cases though.

A simple configuration file might be a more robust way to handle this.

@SimonKagstrom
Copy link
Owner

I think this is pretty much what PR #424 would implement

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

No branches or pull requests

2 participants