Re: [PATCH] t4216: fix no-op test that breaks TAP output

[Taylor Blau <me@ttaylorr.com>]

Hi Patrick,

A couple of thanks are owed: one to Todd for reporting this issue in the
first place, another to Peff for analyizing why it didn't appear broken
before, and a third for you for proposing a patch to fix it.

If you choose to delete this piece of test infrastructure entirely (I
think that there is an alternative direction that I would prefer, but
see below for more on why), I think the patch you wrote below is OK.

But...

On Fri, Jun 19, 2026 at 09:20:20AM +0200, Patrick Steinhardt wrote:
> In t4216 we have have a prerequisite that is active in case the system's
> `char` type is signed by default. This prerequisite isn't really used by
> anything though: while it is used to guard one of our tests, that
> specific test is essentially a no-op. So all this infrastructure does is
> to provide some debugging hint to a reader that pays a lot of attention.

I don't think that this is guarding nothing, but I agree that the test
as written is strange. As I recall, this was to sanity check the v1
Bloom values, but allow failures on platforms where the `char` type is
unsigned by default.

I don't feel that strongly about whether or not we check the exact
value of the filter, but I think there are a couple of arguments in
favor of doing so. Most compelling would be that we know that our
murmur3 implementation is correct (in at least one case) and that we
don't regress that case in the future. We do have these checks for v2
changed-path Bloom filters where the signed-ness of `char` is
irrelevant.

> Besides that, the way we set up the prerequisite also results in broken
> TAP output on systems where `char` is unsigned by default: we use
> `test_cmp()` to diff two files outside of of any test body, and if the
> files differ we enable the prerequisite. If so, the call to `test_cmp()`
> would also print output, and that output is of course not valid TAP
> output.

Given this and the above, I would probably err on the side of
designating this as 'test_lazy_prereq' or otherwise silencing the output
of 'test_cmp' so that this does not taint the TAP output.

Thanks,
Taylor