feat(gwr-code-coverage): Add a tool to compare two coverage reports

[pthedinger]

No description provided.

[samchesney]

The scope in the commit message also needs to be updated from gwr-tools to gwr-code-coverage, as GitHub will not use the updated PR title when merging.

In testing this I've found a problem with the line diff process. Here's what I did:

1. rm -rf target/llvm-cov
2. git status to confirm there were no changes.
3. cargo run --bin terminus --release -- run -r gwr-code-coverage/recipes/coverage.yaml
4. mv target/llvm-cov/latest target/llvm-cov/initial
5. echo "pub fn bar() {\n print!(\"bee\");\n}\n" > gwr-code-coverage/src/foo.rs
6. Add a use statement for foo in the gwr-code-coverage lib:

diff --git a/gwr-code-coverage/src/lib.rs b/gwr-code-coverage/src/lib.rs
index 43d5759..cb670a2 100644
--- a/gwr-code-coverage/src/lib.rs
+++ b/gwr-code-coverage/src/lib.rs
@@ -7,6 +7,7 @@ mod dp_table;
 mod lcs;
 mod markdown;
 mod source_files;
+mod foo;
 
 pub use coverage::{CoverageReport, coverage_did_not_decrease};
 pub use markdown::{DEFAULT_CONTEXT_LINES, RenderOptions, render_markdown_with_options};

8. cargo run --bin terminus --release -- run -r gwr-code-coverage/recipes/coverage.yaml
9. cargo run --bin diff-coverage --release -- target/llvm-cov/initial/details.json target/llvm-cov/latest/details.json > target/llvm-cov/diff.md which produced the following output:

# Coverage Delta

## Coverage Files

| Report | JSON file | Prefix removed |
| --- | --- | --- |
| Before | `target/llvm-cov/initial/details.json` | `/Volumes/scratch/repos/gwr-wt/pr-175` |
| After | `target/llvm-cov/latest/details.json` | `/Volumes/scratch/repos/gwr-wt/pr-175` |

## Totals

| Metric | Before | After | Delta |
| --- | ---: | ---: | ---: |
| Lines | 12813/20075 (63.83%) | 12813/20078 (63.82%) | 0 / +3 (-0.01%) |
| Functions | 1397/2140 (65.28%) | 1397/2141 (65.25%) | 0 / +1 (-0.03%) |
| Regions | 18207/29234 (62.28%) | 18207/29237 (62.27%) | 0 / +3 (-0.01%) |

## Files: showing 1/147 changed files

| File | Lines Before | Lines After | Lines Delta | Functions Before | Functions After | Functions Delta | Regions Before | Regions After | Regions Delta |
| --- | ---: | ---: | ---: | ---: | ---: | ---: | ---: | ---: | ---: |
| `gwr-code-coverage/src/foo.rs` | _ | 0/3 (0.00%) | _ | _ | 0/1 (0.00%) | _ | _ | 0/3 (0.00%) | _ |

## Line Coverage Changes

Rows are shown as `before-line -> after-line [before-hits -> after-hits] | source`; `_` means the line or hit count is absent.

### `gwr-code-coverage/src/lib.rs` changes

```diff
@@ -42,64 +42,64 @@
  42 -> 42 [ 0 -> 0 ] |         Error::Json(error)
  43 -> 43 [ _ -> 0 ] |     }
  44 -> 44 [ _ -> _ ] | }
- 45 -> 45 [12 -> _ ] | 
  46 -> 46 [12 -> 12] | fn infer_prefix<'a>(filenames: impl Iterator<Item = &'a str>) -> String {
  47 -> 47 [12 -> 12] |     let mut common: Option<String> = None;
  48 -> 48 [12 -> 12] |     for filename in filenames {
  49 -> 49 [10 -> 12] |         if !filename.starts_with('/') {
  50 -> 50 [ 2 -> 10] |             return String::new();
+ 51 -> 51 [ _ -> 2 ] |         }
- 52 -> 52 [ 2 -> _ ] | 
  53 -> 53 [ 2 -> 2 ] |         let dirname = filename.rsplit_once('/').map_or("", |(dirname, _)| dirname);
+ 54 -> 54 [ 0 -> 2 ] |         common = Some(match common {
- 55 -> 55 [ 2 -> 0 ] |             Some(common) => common_directory_prefix(common.as_str(), dirname),
+ 56 -> 56 [ _ -> 2 ] |             None => dirname.to_string(),
  57 -> 57 [ _ -> _ ] |         });
  58 -> 58 [ _ -> _ ] |     }
- 59 -> 59 [ 2 -> _ ] | 
  60 -> 60 [12 -> 2 ] |     common.as_deref().map_or_else(String::new, normalize_prefix)
+ 61 -> 61 [ _ -> 12] | }
  62 -> 62 [ 0 -> _ ] | 
  63 -> 63 [ 0 -> 0 ] | fn common_directory_prefix(left: &str, right: &str) -> String {
  64 -> 64 [ 0 -> 0 ] |     left.split('/')
@@ -68,74 +68,74 @@
  68 -> 68 [ 0 -> 0 ] |         .collect::<Vec<_>>()
  69 -> 69 [ 0 -> 0 ] |         .join("/")
  70 -> 70 [ _ -> 0 ] | }
- 71 -> 71 [20 -> _ ] | 
  72 -> 72 [20 -> 20] | fn normalize_prefix(prefix: &str) -> String {
  73 -> 73 [20 -> 20] |     prefix.trim_end_matches('/').to_string()
+ 74 -> 74 [ _ -> 20] | }
```

Which isn't the output I would except from adding mod foo;.

[pthedinger]

With reference to your observations (#175 (review)) I was able to reproduce the same behaviour. However, the issue is that the diff tool is comparing two reports with different coverage for files where the lines have changed, but there is only one source file. So - where the line numbers should be different between the two, they are not. You can see in this block for example:

- 45 -> 45 [12 -> _ ] | 
  46 -> 46 [12 -> 12] | fn infer_prefix<'a>(filenames: impl Iterator<Item = &'a str>) -> String {
  47 -> 47 [12 -> 12] |     let mut common: Option<String> = None;
  48 -> 48 [12 -> 12] |     for filename in filenames {
  49 -> 49 [10 -> 12] |         if !filename.starts_with('/') {
  50 -> 50 [ 2 -> 10] |             return String::new();
+ 51 -> 51 [ _ -> 2 ] |         }

That the tool believes the lines numbers are the same between the versions and that as a result the coverage has moved. This is because the coverage report in one case does not match the source which has changed.

Does that make sense. In effect - this is to be expected.

[samchesney]

I've tried following the same steps as before (more than once to make sure I hadn't started from a dirty state etc.), but got an unexpected diff result:

# Coverage Delta

## Coverage Files

| Report | JSON file | Prefix removed |
| --- | --- | --- |
| Before | `target/llvm-cov/initial/details.json` | `/Volumes/scratch/repos/gwr-wt/pr-175/target/llvm-cov/initial/source-snapshot` |
| After | `target/llvm-cov/latest/details.json` | `/Volumes/scratch/repos/gwr-wt/pr-175/target/llvm-cov/latest/source-snapshot` |

## Totals

| Metric | Before | After | Delta |
| --- | ---: | ---: | ---: |
| Lines | 13209/20490 (64.47%) | 13209/20493 (64.46%) | 0 / +3 (-0.01%) |
| Functions | 1444/2187 (66.03%) | 1444/2188 (66.00%) | 0 / +1 (-0.03%) |
| Regions | 18742/29804 (62.88%) | 18742/29807 (62.88%) | 0 / +3 (-0.01%) |

## Files: showing 1/149 changed files

| File | Lines Before | Lines After | Lines Delta | Functions Before | Functions After | Functions Delta | Regions Before | Regions After | Regions Delta |
| --- | ---: | ---: | ---: | ---: | ---: | ---: | ---: | ---: | ---: |
| `gwr-code-coverage/src/foo.rs` | _ | 0/3 (0.00%) | _ | _ | 0/1 (0.00%) | _ | _ | 0/3 (0.00%) | _ |

## Line Coverage Changes

Rows are shown as `before-line -> after-line [before-hits -> after-hits] | source`; `_` means the line or hit count is absent.
No line-level coverage changes.

Which doesn't show a diff for the new foo lines which have zero coverage, but does show that they have no coverage and therefore reduce the coverage overall.

I'm not sure the last section of the diff output is completely obvious:

## Line Coverage Changes

Rows are shown as `before-line -> after-line [before-hits -> after-hits] | source`; `_` means the line or hit count is absent.
No line-level coverage changes.

The No line-level coverage changes. should have a line break before it so it does not get rendered as part of the "legend" paragraph.

I think it should also spell out that new lines will not be shown if that's intended, as it wouldn't be unreasonable to expect them to be shown as [_, 0] lines, for example.

Next I tried what I assumed would be a no-op:

1. rm -rf target
2. cargo run --bin terminus --release -- run -r gwr-code-coverage/recipes/coverage.yaml
3. mv target/llvm-cov/latest target/llvm-cov/initial
4. cargo run --bin terminus --release -- run -r gwr-code-coverage/recipes/coverage.yaml
5. cargo run --bin diff-coverage --release -- target/llvm-cov/initial/details.json target/llvm-cov/latest/details.json > target/llvm-cov/diff.md which produced the following output:

# Coverage Delta

## Coverage Files

| Report | JSON file | Prefix removed |
| --- | --- | --- |
| Before | `target/llvm-cov/initial/details.json` | `/Volumes/scratch/repos/gwr-wt/pr-175/target/llvm-cov/initial/source-snapshot` |
| After | `target/llvm-cov/latest/details.json` | `/Volumes/scratch/repos/gwr-wt/pr-175/target/llvm-cov/latest/source-snapshot` |

## Totals

| Metric | Before | After | Delta |
| --- | ---: | ---: | ---: |
| Lines | 13209/20490 (64.47%) | 13205/20490 (64.45%) | -4 / 0 (-0.02%) |
| Functions | 1444/2187 (66.03%) | 1444/2187 (66.03%) | 0 / 0 (0.00%) |
| Regions | 18742/29804 (62.88%) | 18737/29804 (62.87%) | -5 / 0 (-0.02%) |

## Files: showing 1/148 changed files

| File | Lines Before | Lines After | Lines Delta | Functions Before | Functions After | Functions Delta | Regions Before | Regions After | Regions Delta |
| --- | ---: | ---: | ---: | ---: | ---: | ---: | ---: | ---: | ---: |
| `gwr-models/src/processing_element/operators/add.rs` | 373/377 (98.94%) | 369/377 (97.88%) | -4 / 0 (-1.06%) | 37/37 (100.00%) | 37/37 (100.00%) | 0 / 0 (0.00%) | 656/679 (96.61%) | 651/679 (95.88%) | -5 / 0 (-0.74%) |

## Line Coverage Changes

Rows are shown as `before-line -> after-line [before-hits -> after-hits] | source`; `_` means the line or hit count is absent.

### `gwr-models/src/processing_element/operators/add.rs` changes

```diff
@@ -160,168 +160,168 @@
  160 -> 160 [_ -> _] |         // cause the output to be of the right shape. So we choose one and update
  161 -> 161 [_ -> _] |         // that.
  162 -> 162 [2 -> 2] |         let (input_a_shape, input_b_shape) = if rng.random_bool(0.5) {
- 163 -> 163 [1 -> 0] |             (
- 164 -> 164 [1 -> 0] |                 choose_input_shape(output, rng, expand_ratio),
- 165 -> 165 [1 -> 0] |                 output.shape.clone(),
  166 -> 166 [_ -> _] |             )
  167 -> 167 [_ -> _] |         } else {
  168 -> 168 [1 -> 2] |             (
```

Do you understand where this diff comes from?

[pthedinger]

I have fixed the output so that there is no legend if there are no files. However, I've also improved the reporting so it now shows:

Line Coverage Changes

Rows are shown as before-line -> after-line [before-hits -> after-hits] | source; _ means the line or hit count is absent.

gwr-code-coverage/src/foo.rs changes

@@ -_ +1,4 @@
+ _ -> 1 [_ -> 0] | pub fn bar() {
+ _ -> 2 [_ -> 0] |     print!("bee");
+ _ -> 3 [_ -> 0] | }
+ _ -> 4 [_ -> _] | 

gwr-code-coverage/src/lib.rs changes

@@ -8,13 +8,14 @@
   8 ->  8 [_ -> _] | mod markdown;
   9 ->  9 [_ -> _] | mod paths;
  10 -> 10 [_ -> _] | mod source_files;
+  _ -> 11 [_ -> _] | mod foo;
  11 -> 12 [_ -> _] | 
  12 -> 13 [_ -> _] | pub use coverage::file::coverage_did_not_decrease;
  13 -> 14 [_ -> _] | pub use coverage::report::CoverageReport;

In terms of the operators/add.rs it came from the fact that it was using an unseeded RNG so the tests are seeing different code paths on different runs. I have fixed this as well by adding the FixedBoolRng.

[samchesney]

I think it'd be good to just look at making sure the temp files created during tests are cleaned up before we merge this.

[samchesney]

I think we should remove the temp dir at the end of the test, a simple way to do this might be to switch to using tempfile::TempDir().

[pthedinger]

Thanks for the feedback - more good points. All addressed I believe.

[samchesney]

LGTM