Re: Git Test Coverage Report for v2.37.0-rc0

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 15, 2022, 10:16 UTC
Message-ID: <220615.86mteexls9.gmgdl@evledraar.gmail.com>
In-Reply-To: <3d1c6dfd-1df6-3393-df5e-692719375772@github.com>

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...

Show 13 quoted lines

> On 6/14/2022 4:18 PM, Derrick Stolee wrote:
>> Ævar Arnfjörð Bjarmason	338959da remote.c: remove braces from one-statement "for"-loops
>> remote.c
>> 338959da 149) for (i = 0; i < remote->url_nr; i++)
>> 338959da 153) for (i = 0; i < remote->pushurl_nr; i++)
>> Ævar Arnfjörð Bjarmason	323822c7 remote.c: don't dereference NULL in freeing loop
>> remote.c
>> 323822c7 151) FREE_AND_NULL(remote->url);
>
> Just noting that these lines are inside remote_clear() which is called by
> remote_state_clear(), which is called by repo_clear(). Apparently we have
> no tests that clear a 'struct repository' that loaded remotes? This sounds
> likely since we don't close these unless they are not the_repository.

Yeah, it's also why we didn't segfault in practice on the landmine I fixed in 323822c72be (remote.c: don't dereference NULL in freeing loop, 2022-06-07).

Show 11 quoted lines

>> Æ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.

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 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!