feat(coverage): add coverage ignore comments #26590
Conversation
|
Hey @bartlomieju, I see you added this to the 2.1.0 milestone! 😊 Can I help with anything else related to this feature in the meantime e.g. writing up docs? |
|
Hey @bcheidemann, thanks for the PR. I intend to land it next week when the merge window for v2.1 opens, would be helpful if you could rebase the PR. In terms of other work, if you could update https://docs.deno.com/runtime/reference/cli/coverage/ or https://docs.deno.com/runtime/reference/cli/test/ with information about how to use ignore coverage comments that would be super helpful 🙏 |
8e51747 to
5d0fe4c
Compare
| // If the coverage ignore start directive has no corresponding close directive | ||
| // then close it at the end of the program. | ||
| if let Some(mut range) = current_range.take() { |
There was a problem hiding this comment.
I'm not sure this is desirable - won't that mark a big chunk of program to be ignored? Maybe instead of doing that we should remove this directive altogether and cover everything?
| &program, | ||
| ); | ||
|
|
||
| assert_eq!(line_directives.len(), 2); |
There was a problem hiding this comment.
Would it make sense to actually assert the ranges of directives are correct?
| // deno-coverage-ignore-start | ||
| function foo(): any {} | ||
|
|
||
| function bar(): any { | ||
| // deno-coverage-ignore-start | ||
| foo(); | ||
| // deno-coverage-ignore-stop | ||
| } | ||
| // deno-coverage-ignore-stop |
There was a problem hiding this comment.
I'm not sure about this one... It feels to me like a user error and we should complain (error?) if user does "nested" directives. WDYT?
There was a problem hiding this comment.
Yeah I think that's a reasonable critique. I'm happy to make this a warning like with the unterminated start comment case, or we could make both an error. I don't have strong opinions on whether this should be an error or a warning, but I do think both cases should be handled the same way.
There was a problem hiding this comment.
I think warning will be fine here 👍
| #[test] | ||
| fn test_parse_next_ignore_comments() { | ||
| let source_code = r#" | ||
| // deno-coverage-ignore-next |
There was a problem hiding this comment.
Bikeshed: for deno fmt we do // deno-fmt-ignore and for lint we do // deno-lint-ignore - maybe we should drop the -next suffix?
| ); | ||
|
|
||
| parse_and_then( | ||
| "// deno-coverage-ignore-file foo -- reason for ignoring", |
There was a problem hiding this comment.
Great, could you add a couple tests for other directives to ensure that they can accept the "reason for ignoring"?
cli/tools/coverage/mod.rs
Outdated
| let parsed_source = parse_program( | ||
| options.script_module_specifier, | ||
| options.script_media_type, | ||
| &options.script_original_source, | ||
| ) | ||
| .expect("invalid source code"); |
There was a problem hiding this comment.
We shouldn't use expect here - we're gonna find all sorts of strange or broken files that are included by mistake - we should error out or print a warning in this case instead of panicking.
|
Hey @bartlomieju @dsherret, thanks for your feedback! Due to some deadlines this week, I likely will not have time to action your feedback until the weekend. Apologies for the delay! |
|
@bartlomieju @dsherret I believe that's all the comments addressed 👍 Let me know if you'd like any further changes made |
|
Hey @bartlomieju @dsherret, I see the 2.2.0 milestone was removed 😞 Is there anything I can do to help land this PR in 2.3.0? |
|
@dsherret Thanks! I've pushed some tests for the warnings as requested. |
|
Thanks again! Going to merge now (minor release merge window is open) |
Closes #16626
Adds the following coverage directives: