Re: [PATCH] t1091: don't grep for `strerror()` string
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Mar 9, 2020, 13:07 UTC
- Message-ID
- <30680fbc-203d-ef31-9863-794ff8f55382@gmail.com>
- In-Reply-To
- <20200308084627.26677-1-martin.agren@gmail.com>
On 3/8/2020 4:46 AM, Martin Ågren wrote:
Show 15 quoted lines
> We grep for "File exists" in stderr of the failing `git sparse-checkout`
> to make sure that it failed for the right reason. We expect the string
> to show up there since we call `strerror(errno)` in
> `unable_to_lock_message()` in lockfile.c.
>
> On the NonStop platform, this fails because the error string is "File
> already exists", which doesn't match our grepping.
>
> See 9042140097 ("test-dir-iterator: do not assume errno values",
> 2019-07-30) for a somewhat similar fix. There, we patched a test helper,
> which meant we had access to `errno` and could investigate it better in
> the test helper instead of just outputting the numerical value and
> evaluating it in the test script. The current situation is different,
> since (short of modifying the lockfile machinery, e.g., to be more
> verbose) we don't have more than the output from `strerror()` available.This is the better design, and I should have used it in the first place. The test is really trying to reveal that we are using a .lock file, and the new error message check makes this even more clear.
Show 13 quoted lines
> Except we do: We prefix `strerror(errno)` with `_("Unable to create
> '%s.lock': ")`. Let's grep for that part instead. It verifies that we
> were indeed unable to create the lock file. (If that fails for some
> other reason than the file existing, we really really should expect
> other tests to fail as well.)
>
> An alternative fix would be to loosen the expression a bit and grep for
> "File.* exists" instead. There would be no guarantee that some other
> implementation couldn't come up with another error string, That is, that
> could be the first move in an endless game of whack-a-mole. Of course,
> it could also take us from "99" to "100" percent of the platforms and
> we'd never have this problem again. But since we have another way of
> addressing this, let's not even try the "loosen it up a bit" strategy.Your alternate substring is better than loosening the pattern in several ways.
Show 6 quoted lines
> Reported-by: Randall S. Becker <rsbecker@nexbridge.com> > Signed-off-by: Martin Ågren <martin.agren@gmail.com> > --- > Hi Randall, > > Thanks for the report.
I'll second the thanks on the report, and extra thanks to Martin for the fix!
Show 29 quoted lines
> > test to fail. The error message generated is "File already exists" not "File > > exists" as is required in the test. We should not be testing for specific > > text content originating from strerror - I thought we had this decision in a > > different thread. > > https://public-inbox.org/git/xmqq36intlpj.fsf@gitster-ct.c.googlers.com/ > > > error: 'grep File exists err' didn't find a match in: > > fatal: Unable to create '/home/ituglib/randall/git/t/trash > > directory.t1091-sparse-checkout-builtin/repo/.git/info/sparse-checkout.lock' > > : File already exists. <----- this is the test issue > > Does this patch solve it? > > t/t1091-sparse-checkout-builtin.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh > index b4c9c32a03..44a91205d6 100755 > --- a/t/t1091-sparse-checkout-builtin.sh > +++ b/t/t1091-sparse-checkout-builtin.sh > @@ -305,7 +305,7 @@ test_expect_success 'fail when lock is taken' ' > test_when_finished rm -rf repo/.git/info/sparse-checkout.lock && > touch repo/.git/info/sparse-checkout.lock && > test_must_fail git -C repo sparse-checkout set deep 2>err && > - test_i18ngrep "File exists" err > + test_i18ngrep "Unable to create .*\.lock" err > ' > > test_expect_success '.gitignore should not warn about cone mode' '
This change looks good to me. The commit message is superb.
Thanks, -Stolee