From: Derrick Stolee <derrickstolee@github•com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail•com>
Cc: Git Mailing List <git@vger•kernel.org>,
Taylor Blau <me@ttaylorr•com>,
Christian Couder <christian.couder@gmail•com>,
Jeff Hostetler <git@jeffhostetler•com>,
Neeraj Singh <nksingh85@gmail•com>
Subject: Re: Git Test Coverage Report for v2.37.0-rc0
Date: Wed, 15 Jun 2022 09:21:48 -0400 [thread overview]
Message-ID: <604c694e-7b8f-40bd-c8da-93b00dfe3541@github.com> (raw)
In-Reply-To: <220615.86mteexls9.gmgdl@evledraar.gmail.com>
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
prev parent reply other threads:[~2022-06-15 13:21 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-14 20:18 Git Test Coverage Report for v2.37.0-rc0 Derrick Stolee
2022-06-14 20:20 ` Derrick Stolee
2022-06-14 20:34 ` Derrick Stolee
2022-06-14 23:46 ` Taylor Blau
2022-06-15 10:16 ` Ævar Arnfjörð Bjarmason
2022-06-15 13:21 ` Derrick Stolee [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=604c694e-7b8f-40bd-c8da-93b00dfe3541@github.com \
--to=derrickstolee@github$(echo .)com \
--cc=avarab@gmail$(echo .)com \
--cc=christian.couder@gmail$(echo .)com \
--cc=git@jeffhostetler$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=me@ttaylorr$(echo .)com \
--cc=nksingh85@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox