Add test coverage tracking with CI/CD

Partially fixes issue #250 (closed)

This PR adds a job coverage in the test stage, as a follow-up on the work in trinity-1686a/arti!1 (closed).

I initially followed this blog-post, but then realised grcov actually has support for cobertura-type outputs now due to this.

requested review from @eta

two things :

• you should probably base your changes on trinity-1686a/arti!2 (closed) which leverage ./maint/with_coverage.sh
• I did not submit either patches because there was an issue with them (and by lack of time): grcov generates big cobertura reports (about 43MiB when I tested, see this issue on grcov tracker), and Gitlab refuses to process files larger than 10MiB for performances reasons. It marks them as simple artifacts instead of proper coverage reports. You can still download them, but they are not displayed on Gitlab interface

Thank you for the comment @trinity-1686a!

you should probably base your changes on trinity-1686a/arti!2 (closed) which leverage ./maint/with_coverage.sh

Sorry about that! I hadn't realised that there was a continuation of your work. I just assumed you'd left it idle. Question: Why did you make those changes to leverage ./maint/with_coverage.sh?

I did not submit either patches because there was an issue with them (and by lack of time): grcov generates big cobertura reports (about 43MiB when I tested, see this issue on grcov tracker), and Gitlab refuses to process files larger than 10MiB for performances reasons. It marks them as simple artifacts instead of proper coverage reports. You can still download them, but they are not displayed on Gitlab interface

So would this not be helpful even for the visualization in the file-diff view? Do you think splitting the file could be a good work-around? I saw this issue being discussed in this issue.

Sorry about that

No need to apologize

Question: Why did you make those changes to leverage ./maint/with_coverage.sh?

When I started, this script did not exist yet. It has some tweaks on how coverage is computed, for instance grcov has additional arguments to not count covered lines in test.

So would this not be helpful even for the visualization in the file-diff view? Do you think splitting the file could be a good work-around? I saw this issue being discussed in this issue.

file-diff visualization did not appear to work in my tests. Splitting might work, but grcov generate lots of duplicate entries in it's cobertura report, and fixing that (upstream, or by merging redundant entries ourselves) should be enough to go under 10M

Hey @arturomf94, thanks for the MR! Looks like the pipeline isn't running here for some reason; gitlab's troubleshooting guide suggests this might be if your fork of the repo is set to private, and indeed https://gitlab.torproject.org/arturomf94/arti/-/tree/coverage-tracking throws a 404 for me. Would you mind setting your fork to public, so the pipelines work? (especially since it seems like this might affect future MRs you make, as well)

@eta - of course! I just set is as public. Hopefully this will allow the pipeline to run.

I found @trinity-1686a's comments interesting and helpful. Specifically, I think splitting might work, as suggested here, but I think maybe we should see the pipeline's output here? Wdyt?

As far as I can tell the job can be found here and it created the artifacts that can be browsed here. In particular, this html report looks fairly good, but I don't know how we could test the file-diff visualization. Also, I'm new to GitLab so I might be missing something here

@arturomf94 nice, thanks for changing that! the HTML report looks pretty nice indeed; I think that'd be a useful thing to have, irrespective of whether we can get the gitlab coverage integration working.

I think it'd be a good idea to integrate your changes into the existing ./maint/with_coverage.sh script, as @trinity-1686a pointed out, and I'd be happy to merge this once that's done!

If you want to try the splitting thing, feel free to, although that can always be experimented with later; no need to do that in this MR if you don't want to!

added 2 commits

• 3213b893 - Add changes in `maint`
• b3ba3282 - Use `maint/with_coverage.sh` in coverage job

Compare with previous version

added 1 commit

• 899691e5 - Install bs4 in coverage job

Compare with previous version

changed the description

Hey @eta - I just committed the changes so as to use @trinity-1686a's patches on ./maint/with_coverage.sh. It ran fine... You can find the report here.

I also changed the MR's description to point out that it only partially solves issue #250 (closed).

approved this merge request

merged

mentioned in commit 785ad605

Nice, thanks!

I've reformatted the with_coverage.sh script a little bit to make it easier to review future changes (40c67e5e), and restored the behavior where we don't count coverage from tor-bench (see !210 (merged), 5b6b0d6a).

Quick question that I couldn't figure out: what does this do, and do we still need it?

	--ignore="*/github.com-1ecc6299db9ec823/*"

mentioned in commit 5b6b0d6a

@nickm - sweet! Thanks for the changes! I particularly liked the "let's avoid being the people who accidentally remove somebody's filesystem" justification .

The --ignore="*/github.com-1ecc6299db9ec823/*" flag comes from @trinity-1686a's changes, but now that I think of it I reckon it is due to this.

Since we don't have that line now, I think it should be fine to remove it. Wdyt?

Quick question that I couldn't figure out: what does this do, and do we still need it? --ignore="*/github.com-1ecc6299db9ec823/*"

For some reason every dependency of Arti was present in coverage reports in cobertura format. In an effort to reduce its size and make it acceptable by gitlab, this was added to ignore anything pulled from crates-io from coverage report. It reduce size from 83M to 12M for ./maint/with_coverage.sh -f cobertura -o coverage.xml cargo test, which is great but not enough

Hm. In that case, should we change it to ignore */registry/index/* and */registry/src/* ? Or maybe we should use --keep-only "crates/*"? Both of those seem a little cleaner.

I'm fine with either (if ignoring, registry/index can be omitted, it does not contain code).
This was the first thing that came to mind, and I was still trying to get Gitlab to show me the report, so I did not yet care about getting it "cleaner"