fix(coverage): use only string byte indexes and 0-indexed line numbers #13190
Conversation
| @@ -81,6 +81,7 @@ shell-escape = "=0.1.5" | |||
| sourcemap = "=6.0.1" | |||
| tempfile = "=3.2.0" | |||
| text-size = "=1.1.0" | |||
| text_lines = "=0.4.1" | |||
There was a problem hiding this comment.
This is my repo, which is used in deno_ast and already a dependency.
| let mut line_counts = Vec::with_capacity(text_lines.lines_count()); | ||
| for line_index in 0..text_lines.lines_count() { | ||
| let line_start_offset = text_lines.line_start(line_index); | ||
| let line_end_offset = text_lines.line_end(line_index); |
There was a problem hiding this comment.
It might be good to improve text_lines to return an iterator of line information, but this is an O(1) lookup so not critical... just would be nicer.
| && (span.hi.0 as usize) >= line_end_offset | ||
| }) || script_source[line_start_offset..line_end_offset] | ||
| .trim() | ||
| .is_empty(); |
There was a problem hiding this comment.
This fixes the blank line issue. I think we need to do something a bit better for determining which lines to ignore in the future though (ex. lines with only a close brace or similar token).
cli/tools/coverage.rs
Outdated
| let url = Url::parse(&script_coverage.url).unwrap(); | ||
| let file_path = url.to_file_path().unwrap(); |
There was a problem hiding this comment.
Is it guaranteed that these unwraps won't fail? Why not make script_coverage.url a ModuleSpecifier?
There was a problem hiding this comment.
I just kept the code as-is here for the most part. I don't want to change script_coverage.url to a ModuleSpecifier for this PR (maybe a followup)... looks like it might be a filename. I will slightly improve this though.
| // TODO(caspervonb): collect uncovered ranges on the lines so that we can highlight specific | ||
| // parts of a line in color (word diff style) instead of the entire line. |
There was a problem hiding this comment.
Is this comment still relevant?
cli/tools/coverage.rs
Outdated
| let overlaps = std::cmp::max(line_end_offset, range.end_offset) | ||
| - std::cmp::min(line_start_offset, range.start_offset) | ||
| < (line_end_offset - line_start_offset) | ||
| + (range.end_offset - range.start_offset); |
There was a problem hiding this comment.
It's really hard to read this :D maybe split into two separate variable bindings?
There was a problem hiding this comment.
This is code from before. I'm not sure why it's using lengths to do this comparison. I changed it to compare offsets and it should be the same (I think).
cli/tools/coverage.rs
Outdated
| impl CoverageReporter for LcovCoverageReporter { | ||
| fn report(&mut self, coverage_report: &CoverageReport, _file_text: &str) { | ||
| println!("SF:{}", coverage_report.file_path); |
There was a problem hiding this comment.
This is a very nice cleanup
Verified, that everything works correctly. |
The coverage code was mixing character indexes and byte indexes along with 0-indexed line numbers with 1-indexed. This PR fixes that along with ignoring blank lines. There is still some work to do on coverage.
Closes #11327