On 6/15/22 6:16 AM, Ævar Arnfjörð Bjarmason wrote: > > On Tue, Jun 14 2022, Derrick Stolee wrote: > > Neat, thanks! Re git-test-coverage: Maybe it's covered somewhere, but is > there tooling to reproduce this somewhere? I.e. we have "make coverage" > in tree, but not this "do the diff and blame on lines" step, unless I've > missed it somewhere... There is a script at contrib/coverage-diff.sh, but I then created a separate project [1] that I could iterate on more quickly for new formats (like the commit-grouped output you see here). I was running builds on Azure Pipelines [2], but they started failing a couple years ago (I think because of timeouts, but maybe also because of failures in 'seen'). I tried resurrecting them recently, but failed. So, I just ran this locally on my own machine. [1] https://github.com/derrickstolee/git-test-coverage [2] https://dev.azure.com/git/git/_build?definitionId=12 >>> Ævar Arnfjörð Bjarmason fd3aaf53 run-command: add an "ungroup" option to run_process_parallel() >>> run-command.c >>> fd3aaf53 1645) code = pp->start_failure(pp->ungroup ? NULL : >>> fd3aaf53 1646) &pp->children[i].err, >>> fd3aaf53 1649) if (!pp->ungroup) { >>> fd3aaf53 1650) strbuf_addbuf(&pp->buffered_output, &pp->children[i].err); >>> fd3aaf53 1651) strbuf_reset(&pp->children[i].err); >> >> This change seems sufficiently complicated that it would be good to look >> into the test coverage here. It's possible that it is covered by the >> GIT_TEST_* variables that I didn't use when generating my test coverage. > > We should definitely have test coverage for these, but FWIW it's all > pre-existing-not-tested code. Yes, I agree that we should not hold up new changes just because old code isn't tested properly. It is worth considering whether we can add tests easily _or_ if we should just give the new code another look. > I.e. this has presumably been untested as far back as c553c72eed6 > (run-command: add an asynchronous parallel child processor, > 2015-12-15). It's just: > > if (start_command(&pp->children[i].process)) > /* this whole thing is untested, as no "start command" fails */ > > There's probably no easy way to automate this, but for significant bonus > points I think a script like this would be much more useful if once it > identifies given commit:line pairs it would check out $that_commit^, run > "make coverage" again, see how pre-image faired (presumably --word-diff > would associate them), and post that diff on top. I think it's worth pointing out which lines are changing untested code. Two core principles in "Working Effectively with Legacy Code" by Michael Feathers [3] are: 1. Untested code is legacy code. 2. Add tests before changing legacy code. [3] https://www.oreilly.com/library/view/working-effectively-with/0131177052/ The point of this report is to see how well we are fitting with those principles, but also to take a pragmatic approach to deciding whether new tests are worth the effort. > I see in this case it's as easy as tweaking t0061-run-command.sh to do: > > test-tool run-command run-command-parallel 5 this-command-does-not-exist > > I.e. we just don't stress that failure path, but should. > > I'm planning to submit a cleanup series for the "ungroup" feature, > i.e. the API is weird because we were trying to come up with a minimal > regression fix. I'll make sure to include tests for this scenario in > that series. Thanks! Thank you! It might also be worth pointing out that the 'coverage-test' build disables threading due to colliding in the *.gcov output. That leads to untested code that is actually tested by the "real" test suite. -Stolee
