Re: [PATCH 0/2] microproject: avoid using pipes in test

On Tue, Feb 22, 2022 at 03:24:31PM +0100, Ævar Arnfjörð Bjarmason wrote:

On Tue, Feb 22 2022, Shubham Mishra wrote:

quoted

pipes doesn't care about error codes and ignore them thus we should not use them in tests.

Aside from what Derrick Stolee mentioned in his feedback, all of which I
agree with.

I think these changes are good, but it's not the case that we try to
avoid using pipes at all in our tests.

It's often a hassle, and just not worth it, e.g.:

    oid=$(echo foo | git hash-object --stdin -w) &&

Sure, we can make that:

    echo foo >in &&
    oid=$(git hash-object --stdin -w <in) &&

But in the general case it's not worth worrying about.

Agreed, and I would add that we don't necessarily need to worry about
non-Git commands on the left-hand side of a pipe. So something like:

    find ... | sort >actual

isn't a problem for us, because our test suite assumes that something
like find will not fail. So leaving instances of those alone is OK,
but...

What we *do* try to avoid, and what's actually important is to never
invoke "git" or other programs we invoke on the LHS of a pipe, or to
otherwise do so in a way that hides potential errors.

That's not isolated to just pipes, but e.g. calling it within helper
functions that don't &&-chain, but pipes are probably the most common
offender.

The reason we do that is because in hacking git we may make it error,
segfault etc. If it's on the LHS of a pipe that failure becomes
indistinguishable from success.

And if the test is really checking e.g. "when I run git like this, it
produces no output" printing nothing with an exit of 0 will become the
same as a segafault for the purposes of test.

...yes, we do care about Git failures. So something like:

    git ls-files | grep "want"

would be no-good, since any failures running 'git ls-files' would be
quashed by the pipe.

Thanks,
Taylor