Re: [PATCH v3 2/8] t/helper: add 'find-pack' test-tool

[Taylor Blau <me@ttaylorr.com>]

On Mon, Jul 24, 2023 at 10:59:03AM +0200, Christian Couder wrote:
> ---
>  Makefile                  |  1 +
>  t/helper/test-find-pack.c | 35 +++++++++++++++++++++++++++++++++++
>  t/helper/test-tool.c      |  1 +
>  t/helper/test-tool.h      |  1 +
>  4 files changed, 38 insertions(+)
>  create mode 100645 t/helper/test-find-pack.c

Everything that you wrote here seems reasonable to me, and the
implementation of the new test tool is very straightforward.

I'm pretty sure that everything here is correct, and we'll implicitly
test the behavior of the new helper in following patches.

That said, I think that it might be prudent here to "test the tests" and
write a simple test script that exercises this test helper over a more
trivial case. There is definitely prior art for testing our helpers
directly in the t00?? tests.

Among the test helpers that I can think of off the top of my head, I
think a good handful of them have tests:

  - t0011-hashmap.sh
  - t0015-hash.sh
  - t0016-oidmap.sh
  - t0019-json-writer.sh
  - t0052-simple-ipc.sh
  - t0060-path-utils.sh
  - t0061-run-command.sh
  - t0063-string-list.sh
  - t0064-oid-array.sh
  - t0066-dir-iterator.sh
  - t0095-bloom.sh

I would definitely recommend adding a test here, too. Like I said
earlier, I think that you are implicitly testing the new behavior here,
but it's going to happen in much more complicated environments than
something you could construct synthetically here.

Thanks,
Taylor