Re: Git Test Coverage Report (Nov 25)

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Stolee,

thank you for keeping doing these reports.

On Mon, 25 Nov 2019, Derrick Stolee wrote:

> Here is today's test coverage report.
>
> Sorry for the delay, but I had some issues working out the new
> "ignored lines" logic. Now there are a few phrases that are
> ignored automatically, including "BUG(" or "die(".
>
> [...]
> Johannes Schindelin	eea4a7f4 mingw: demonstrate that all file handles are inherited by child processes
> t/helper/test-run-command.c
> eea4a7f4 203) static int inherit_handle(const char *argv0)
> eea4a7f4 205) struct child_process cp = CHILD_PROCESS_INIT;
> eea4a7f4 210) xsnprintf(path, sizeof(path), "out-XXXXXX");
> eea4a7f4 211) tmp = xmkstemp(path);
> eea4a7f4 213) argv_array_pushl(&cp.args,
> eea4a7f4 215) cp.in = -1;
> eea4a7f4 216) cp.no_stdout = cp.no_stderr = 1;
> eea4a7f4 217) if (start_command(&cp) < 0)
> eea4a7f4 221) close(tmp);
> eea4a7f4 222) if (unlink(path))
> eea4a7f4 225) if (close(cp.in) < 0 || finish_command(&cp) < 0)
> eea4a7f4 228) return 0;
> eea4a7f4 231) static int inherit_handle_child(void)
> eea4a7f4 233) struct strbuf buf = STRBUF_INIT;
> eea4a7f4 235) if (strbuf_read(&buf, 0, 0) < 0)
> eea4a7f4 237) printf("Received %s\n", buf.buf);
> eea4a7f4 238) strbuf_release(&buf);
> eea4a7f4 240) return 0;
> eea4a7f4 251) exit(inherit_handle(argv[0]));
> eea4a7f4 253) exit(inherit_handle_child());

I think this is actually covered, but only in the Windows build.

> Johannes Schindelin	116a408b commit: give correct advice for empty commit during a rebase
> builtin/commit.c
> 116a408b 478) else if (whence == FROM_CHERRY_PICK) {
> 116a408b 479) if (rebase_in_progress && !sequencer_in_use)
> 116a408b 1186) else if (whence == FROM_CHERRY_PICK) {
> 116a408b 1187) if (rebase_in_progress && !sequencer_in_use)

My understanding is that Phillip is working on a replacement for this
patch, so I'll leave this as-is for now.

> Johannes Schindelin	0de7b036 built-in add -i: offer the `quit` command
> add-interactive.c
> [...]
>
> Johannes Schindelin	d17e3a76 built-in add -i: prepare for multi-selection commands
> add-interactive.c
> [...]
>
> Johannes Schindelin	dea080c8 built-in add -i: re-implement `revert` in C
> add-interactive.c
> [...]
>
> Johannes Schindelin	daabf56b built-in add -i: re-implement `add-untracked` in C
> add-interactive.c
> [...]
>
> Johannes Schindelin	4ba7c22a built-in add -i: implement the `update` command
> add-interactive.c
> [...]
>
> Johannes Schindelin	9d86921e built-in add -i: implement the `patch` command
> add-interactive.c
> [...]
>
> Johannes Schindelin	a5ced6db built-in add -i: allow filtering the modified files list
> add-interactive.c
> [...]
>
> Johannes Schindelin	465a9699 built-in add -i: re-implement the `diff` command
> add-interactive.c
> [...]
>
> Johannes Schindelin	68db1cbf built-in add -i: support `?` (prompt help)
> add-interactive.c
> [...]
>
> Johannes Schindelin	76b74323 built-in add -i: show unique prefixes of the commands
> add-interactive.c
> [...]
>
> Johannes Schindelin	6348bfba built-in add -i: implement the main loop
> add-interactive.c
> [...]
>
> Johannes Schindelin	f83dff60 Start to implement a built-in version of `git add --interactive`
> add-interactive.c
> f83dff60 571) int run_add_i(struct repository *r, const struct pathspec *ps)
>
> builtin/add.c
> f83dff60 197) return !!run_add_i(the_repository, pathspec);

This ginormous amount of code without test coverage is exactly the type of
problem I pointed out with the approach I had to take in the built-in add
-i/-p series, and which I described in the commit message
https://public-inbox.org/git/5d9962d4344fa182b37cd8d969da01bc603414be.1573648866.git.gitgitgadget@gmail.com/
Most of it _will_ be covered by the tests, but _only_ at the very end,
when all six patch series will be merged.

> Johannes Schindelin	867bc1d2 rebase-merges: move labels' whitespace mangling into `label_oid()`
> sequencer.c
> 867bc1d2 4539) }

Not sure why this closing parenthesis isn't covered :-D

> Johannes Schindelin	4d17fd25 remote-curl: unbreak http.extraHeader with custom allocators
> http.c
> 4d17fd25 417) string_list_clear(&extra_http_headers, 0);

I think it is okay to have that uncovered. This line is in the unlikely
code path where a user will try to unset the extra headers via `git -c
http.extraHeaders [...]`, i.e. without setting a value for that setting.

> Johannes Schindelin	2e0afafe Add git-bundle: move objects and references by archive
> builtin/bundle.c
> 2e0afafe 115) return 1;

I have to admit that I am slightly curious how this commit from 2007 ended
up in this list: 2e0afafebd8 (Add git-bundle: move objects and references
by archive, 2007-02-22). That's quite the blast of the past.

> Johannes Schindelin	116d1fa6 vreportf(): avoid relying on stdio buffering
> usage.c
> 116d1fa6 16) fprintf(stderr, "BUG!!! too long a prefix '%s'\n", prefix);
> 116d1fa6 17) abort();
> 116d1fa6 22) *p = '\0'; /* vsnprintf() failed, clip at prefix */

Those are defensive programming, so this is expected not to be covered.

> Johannes Schindelin	d54dea77 fetch: let --jobs=<n> parallelize --multiple, too
> builtin/fetch.c
> d54dea77 113) fetch_parallel_config = git_config_int(k, v);
> d54dea77 114) if (fetch_parallel_config < 0)
> d54dea77 116) return 0;
> d54dea77 1566) static int fetch_failed_to_start(struct strbuf *out, void *cb, void *task_cb)
> d54dea77 1568) struct parallel_fetch_state *state = cb;
> d54dea77 1569) const char *remote = task_cb;
> d54dea77 1573) return 0;

I guess that yes, this points out that setting the number of parallel
fetches via `fetch.parallel` is untested... I am willing to leave it like
that until the time a regression is found in that code (if ever).

> Johannes Schindelin	e145d993 rebase -r: support merge strategies other than `recursive`
> sequencer.c
> e145d993 3304) (!opts->strategy || !strcmp(opts->strategy, "recursive")) ?

I am a bit puzzled by this because the added test case in t3430 should
cover precisely this line.

> Johannes Schindelin	e2683d51 Fix .git/ discovery at the root of UNC shares
> setup.c
> e2683d51 952)     !is_dir_sep(dir->buf[min_offset - 1])) {
> e2683d51 953) strbuf_addch(dir, '/');
> e2683d51 954) min_offset++;

I think this is only tested on Windows.

> Johannes Schindelin	be5d88e1 test-tool run-command: learn to run (parts of) the testsuite
> t/helper/test-run-command.c
> be5d88e1 66) static int next_test(struct child_process *cp, struct strbuf *err, void *cb,
> be5d88e1 69) struct testsuite *suite = cb;
> be5d88e1 71) if (suite->next >= suite->tests.nr)
> be5d88e1 72) return 0;
> be5d88e1 74) test = suite->tests.items[suite->next++].string;
> be5d88e1 75) argv_array_pushl(&cp->args, "sh", test, NULL);
> be5d88e1 76) if (suite->quiet)
> be5d88e1 77) argv_array_push(&cp->args, "--quiet");
> be5d88e1 78) if (suite->immediate)
> be5d88e1 79) argv_array_push(&cp->args, "-i");
> be5d88e1 80) if (suite->verbose)
> be5d88e1 81) argv_array_push(&cp->args, "-v");
> be5d88e1 82) if (suite->verbose_log)
> be5d88e1 83) argv_array_push(&cp->args, "-V");
> be5d88e1 84) if (suite->trace)
> be5d88e1 85) argv_array_push(&cp->args, "-x");
> be5d88e1 86) if (suite->write_junit_xml)
> be5d88e1 87) argv_array_push(&cp->args, "--write-junit-xml");
> be5d88e1 89) strbuf_addf(err, "Output of '%s':\n", test);
> be5d88e1 90) *task_cb = (void *)test;
> be5d88e1 92) return 1;
> be5d88e1 95) static int test_finished(int result, struct strbuf *err, void *cb,
> be5d88e1 98) struct testsuite *suite = cb;
> be5d88e1 99) const char *name = (const char *)task_cb;
> be5d88e1 101) if (result)
> be5d88e1 102) string_list_append(&suite->failed, name);
> be5d88e1 104) strbuf_addf(err, "%s: '%s'\n", result ? "FAIL" : "SUCCESS", name);
> be5d88e1 106) return 0;
> be5d88e1 109) static int test_failed(struct strbuf *out, void *cb, void *task_cb)
> be5d88e1 111) struct testsuite *suite = cb;
> be5d88e1 112) const char *name = (const char *)task_cb;
> be5d88e1 114) string_list_append(&suite->failed, name);
> be5d88e1 115) strbuf_addf(out, "FAILED TO START: '%s'\n", name);
> be5d88e1 117) return 0;
> be5d88e1 125) static int testsuite(int argc, const char **argv)
> be5d88e1 127) struct testsuite suite = TESTSUITE_INIT;
> be5d88e1 128) int max_jobs = 1, i, ret;
> be5d88e1 131) struct option options[] = {
> be5d88e1 145) memset(&suite, 0, sizeof(suite));
> be5d88e1 146) suite.tests.strdup_strings = suite.failed.strdup_strings = 1;
> be5d88e1 148) argc = parse_options(argc, argv, NULL, options,
> be5d88e1 151) if (max_jobs <= 0)
> be5d88e1 152) max_jobs = online_cpus();
> be5d88e1 154) dir = opendir(".");
> be5d88e1 155) if (!dir)
> be5d88e1 157) while ((d = readdir(dir))) {
> be5d88e1 158) const char *p = d->d_name;
> be5d88e1 160) if (*p != 't' || !isdigit(p[1]) || !isdigit(p[2]) ||
> be5d88e1 161)     !isdigit(p[3]) || !isdigit(p[4]) || p[5] != '-' ||
> be5d88e1 162)     !ends_with(p, ".sh"))
> be5d88e1 163) continue;
> be5d88e1 166) if (!argc) {
> be5d88e1 167) string_list_append(&suite.tests, p);
> be5d88e1 168) continue;
> be5d88e1 171) for (i = 0; i < argc; i++)
> be5d88e1 172) if (!wildmatch(argv[i], p, 0)) {
> be5d88e1 173) string_list_append(&suite.tests, p);
> be5d88e1 174) break;
> be5d88e1 177) closedir(dir);
> be5d88e1 179) if (!suite.tests.nr)
> be5d88e1 181) if (max_jobs > suite.tests.nr)
> be5d88e1 182) max_jobs = suite.tests.nr;
> be5d88e1 184) fprintf(stderr, "Running %d tests (%d at a time)\n",
> be5d88e1 187) ret = run_processes_parallel(max_jobs, next_test, test_failed,
> be5d88e1 190) if (suite.failed.nr > 0) {
> be5d88e1 191) ret = 1;
> be5d88e1 192) fprintf(stderr, "%d tests failed:\n\n", suite.failed.nr);
> be5d88e1 193) for (i = 0; i < suite.failed.nr; i++)
> be5d88e1 194) fprintf(stderr, "\t%s\n", suite.failed.items[i].string);
> be5d88e1 197) string_list_clear(&suite.tests, 0);
> be5d88e1 198) string_list_clear(&suite.failed, 0);
> be5d88e1 200) return !!ret;
> be5d88e1 209) exit(testsuite(argc - 1, argv + 1));

This is exercised extensively in the Visual Studio job of our Azure
Pipeline.

Thanks,
DschO