* Tests failed with GIT_TEST_FAIL_PREREQS and/or GIT_TEST_PROTOCOL_VERSION @ 2021-03-16 9:45 Son Luong Ngoc 2021-03-16 13:52 ` Taylor Blau 0 siblings, 1 reply; 10+ messages in thread From: Son Luong Ngoc @ 2021-03-16 9:45 UTC (permalink / raw) To: git; +Cc: Taylor Blau Hi folks, Running the test suit with GIT_TEST_FAIL_PREREQS=1 on master (and next) seem to result in some failures: Test Summary Report ------------------- t5300-pack-object.sh (Wstat: 256 Tests: 46 Failed: 2) Failed tests: 35-36 Non-zero exit status: 1 t7810-grep.sh (Wstat: 256 Tests: 229 Failed: 1) Failed test: 160 Non-zero exit status: 1 Files=924, Tests=22400, 422 wallclock secs (12.57 usr 2.52 sys + 601.02 cusr 1047.02 csys = 1663.13 CPU) Result: FAIL A quick git-bisect run seems to point back to this commit: 3b1ca60f8f317b483c8c1805ab500ff2b014cbec is the first bad commit commit 3b1ca60f8f317b483c8c1805ab500ff2b014cbec Author: Taylor Blau <me@ttaylorr.com> Date: Tue Dec 8 17:03:14 2020 -0500 ewah/ewah_bitmap.c: avoid open-coding ALLOC_GROW() 'ewah/ewah_bitmap.c:buffer_grow()' is responsible for growing the buffer used to store the bits of an EWAH bitmap. It is essentially doing the same task as the 'ALLOC_GROW()' macro, so use that instead. This simplifies the callers of 'buffer_grow()', who no longer have to ask for a specific size, but rather specify how much of the buffer they need. They also no longer need to guard 'buffer_grow()' behind an if statement, since 'ALLOC_GROW()' (and, by extension, 'buffer_grow()') is a noop if the buffer is already large enough. But, the most significant change is that this fixes a bug when calling buffer_grow() with both 'alloc_size' and 'new_size' set to 1. In this case, truncating integer math will leave the new size set to 1, causing the buffer to never grow. Instead, let alloc_nr() handle this, which asks for '(new_size + 16) * 3 / 2' instead of 'new_size * 3 / 2'. Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> ewah/ewah_bitmap.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) I also found that a test is failing with protocol V1 set (GIT_TEST_PROTOCOL_VERSION=1) Test Summary Report ------------------- t5606-clone-options.sh (Wstat: 256 Tests: 15 Failed: 1) Failed test: 14 Non-zero exit status: 1 Files=924, Tests=22852, 568 wallclock secs (12.69 usr 2.73 sys + 842.87 cusr 1322.91 csys = 2181.20 CPU) Result: FAIL Which git-bisect is telling me that was caused by the same commit. Regards, Son Luong. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Tests failed with GIT_TEST_FAIL_PREREQS and/or GIT_TEST_PROTOCOL_VERSION 2021-03-16 9:45 Tests failed with GIT_TEST_FAIL_PREREQS and/or GIT_TEST_PROTOCOL_VERSION Son Luong Ngoc @ 2021-03-16 13:52 ` Taylor Blau 2021-03-17 13:38 ` Son Luong Ngoc 0 siblings, 1 reply; 10+ messages in thread From: Taylor Blau @ 2021-03-16 13:52 UTC (permalink / raw) To: Son Luong Ngoc; +Cc: git, Taylor Blau, Ævar Arnfjörð Bjarmason Hi, On Tue, Mar 16, 2021 at 10:45:20AM +0100, Son Luong Ngoc wrote: > Hi folks, > > Running the test suit with GIT_TEST_FAIL_PREREQS=1 on master (and > next) seem to result in some failures: Ack, thanks for reporting. > A quick git-bisect run seems to point back to this commit: > > 3b1ca60f8f317b483c8c1805ab500ff2b014cbec is the first bad commit > commit 3b1ca60f8f317b483c8c1805ab500ff2b014cbec > Author: Taylor Blau <me@ttaylorr.com> > Date: Tue Dec 8 17:03:14 2020 -0500 Hmm. I get a different result. $ cat run.sh #!/bin/sh make -j20 DEVELOPER=1 O=0 && cd t && GIT_TEST_FAIL_PREREQS=1 ./t5300-*.sh -i $ git bisect start $ git bisect bad $ git bisect good v2.30.0 Bisecting: 453 revisions left to test after this (roughly 9 steps) $ git bisect run $PWD/run.sh [snip] a926c4b904bdc339568c2898af955cdc61b31542 is the first bad commit commit a926c4b904bdc339568c2898af955cdc61b31542 Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Date: Thu Feb 11 02:53:51 2021 +0100 tests: remove most uses of C_LOCALE_OUTPUT > ewah/ewah_bitmap.c: avoid open-coding ALLOC_GROW() I'll take a look at what's going on here, but Ævar (cc'd) will probably beat me to it. Is it possible that your bisection script doesn't report success properly? Bisecting the same range (v2.30.0..v2.31.0) with $ cat run.sh #!/bin/sh false does say that my 3b1ca60f8f (ewah/ewah_bitmap.c: avoid open-coding ALLOC_GROW(), 2020-12-08) is the first bad commit. Thanks, Taylor ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Tests failed with GIT_TEST_FAIL_PREREQS and/or GIT_TEST_PROTOCOL_VERSION 2021-03-16 13:52 ` Taylor Blau @ 2021-03-17 13:38 ` Son Luong Ngoc 2021-03-17 15:42 ` [PATCH] t5606: run clone branch name test with protocol v2 Jonathan Tan 2021-03-17 17:54 ` Tests failed with GIT_TEST_FAIL_PREREQS and/or GIT_TEST_PROTOCOL_VERSION Jeff King 0 siblings, 2 replies; 10+ messages in thread From: Son Luong Ngoc @ 2021-03-17 13:38 UTC (permalink / raw) To: Taylor Blau; +Cc: git, avarab, jonathantanmy, gitster Hi Taylor, On Tue, Mar 16, 2021 at 09:52:47AM -0400, Taylor Blau wrote: > Hi, > > Is it possible that your bisection script doesn't report success > properly? Bisecting the same range (v2.30.0..v2.31.0) with > > $ cat run.sh > #!/bin/sh > false > > does say that my 3b1ca60f8f (ewah/ewah_bitmap.c: avoid open-coding > ALLOC_GROW(), 2020-12-08) is the first bad commit. You are spot on. It was a busy day and I only had a few minutes to look at our internal pipeline of the test suite. I guess I was doing something along the line of. $ git bisect start HEAD v2.30.0 $ git bisect run 'cd t && GIT_TEST_PROTOCOL_VERSION=1 ./t5606-clone-options.sh' Which does indeed errored out and pointed to your commit. > > Thanks, > Taylor I have properly re-run the bisection in a './test.sh' bash script and here are the suspicious commits: 1. For t7810 and t5300 failing when GIT_TEST_FAIL_PREREQS=1: a926c4b904bdc339568c2898af955cdc61b31542 is the first bad commit commit a926c4b904bdc339568c2898af955cdc61b31542 Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Date: Thu Feb 11 02:53:51 2021 +0100 tests: remove most uses of C_LOCALE_OUTPUT As a follow-up to d162b25f956 (tests: remove support for GIT_TEST_GETTEXT_POISON, 2021-01-20) remove those uses of the now always true C_LOCALE_OUTPUT prerequisite from those tests which declare it as an argument to test_expect_{success,failure}. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 2. For failing t5606 while 'GIT_TEST_PROTOCOL_VERSION=1' was used: 4f37d45706514a4b3d0259d26f719678a0cf3521 is the first bad commit commit 4f37d45706514a4b3d0259d26f719678a0cf3521 Author: Jonathan Tan <jonathantanmy@google.com> Date: Fri Feb 5 12:48:49 2021 -0800 clone: respect remote unborn HEAD Teach Git to use the "unborn" feature introduced in a previous patch as follows: Git will always send the "unborn" argument if it is supported by the server. During "git clone", if cloning an empty repository, Git will use the new information to determine the local branch to create. In all other cases, Git will ignore it. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> Documentation/config/init.txt | 2 +- builtin/clone.c | 16 ++++++++++++++-- connect.c | 28 ++++++++++++++++++++++++++-- t/t5606-clone-options.sh | 8 +++++--- t/t5702-protocol-v2.sh | 25 +++++++++++++++++++++++++ transport.h | 8 ++++++++ 6 files changed, 79 insertions(+), 8 deletions(-) Thanks, Son Luong. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] t5606: run clone branch name test with protocol v2 2021-03-17 13:38 ` Son Luong Ngoc @ 2021-03-17 15:42 ` Jonathan Tan 2021-03-17 17:42 ` Jeff King 2021-03-17 18:18 ` Junio C Hamano 2021-03-17 17:54 ` Tests failed with GIT_TEST_FAIL_PREREQS and/or GIT_TEST_PROTOCOL_VERSION Jeff King 1 sibling, 2 replies; 10+ messages in thread From: Jonathan Tan @ 2021-03-17 15:42 UTC (permalink / raw) To: git; +Cc: Jonathan Tan, sluongng 4f37d45706 ("clone: respect remote unborn HEAD", 2021-02-05) introduces a new feature (if the remote has an unborn HEAD, e.g. when the remote repository is empty, use it as the name of the branch) that only works in protocol v2, but did not ensure that one of its tests always uses protocol v2, and thus that test would fail if GIT_TEST_PROTOCOL_VERSION=0 (or 1) is used. Therefore, add "-c protocol.version=2" to the appropriate test. (The rest of the tests from that commit have "-c protocol.version=2" already added.) Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- Thanks, Son Luong, for noticing this. Here's a fix for the GIT_TEST_PROTOCOL_VERSION part. This was built on 4f37d45706 but also applies cleanly on master. t/t5606-clone-options.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh index ca6339a5fb..5e30772735 100755 --- a/t/t5606-clone-options.sh +++ b/t/t5606-clone-options.sh @@ -106,7 +106,7 @@ test_expect_success 'chooses correct default initial branch name' ' git -c init.defaultBranch=foo init --bare empty && test_config -C empty lsrefs.unborn advertise && GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \ - git -c init.defaultBranch=up clone empty whats-up && + git -c init.defaultBranch=up -c protocol.version=2 clone empty whats-up && test refs/heads/foo = $(git -C whats-up symbolic-ref HEAD) && test refs/heads/foo = $(git -C whats-up config branch.foo.merge) ' -- 2.31.0.rc2.261.g7f71774620-goog ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] t5606: run clone branch name test with protocol v2 2021-03-17 15:42 ` [PATCH] t5606: run clone branch name test with protocol v2 Jonathan Tan @ 2021-03-17 17:42 ` Jeff King 2021-03-17 18:18 ` Junio C Hamano 1 sibling, 0 replies; 10+ messages in thread From: Jeff King @ 2021-03-17 17:42 UTC (permalink / raw) To: Jonathan Tan; +Cc: git, sluongng On Wed, Mar 17, 2021 at 08:42:00AM -0700, Jonathan Tan wrote: > 4f37d45706 ("clone: respect remote unborn HEAD", 2021-02-05) introduces > a new feature (if the remote has an unborn HEAD, e.g. when the remote > repository is empty, use it as the name of the branch) that only works > in protocol v2, but did not ensure that one of its tests always uses > protocol v2, and thus that test would fail if > GIT_TEST_PROTOCOL_VERSION=0 (or 1) is used. Therefore, add "-c > protocol.version=2" to the appropriate test. > > (The rest of the tests from that commit have "-c protocol.version=2" > already added.) Thanks, this looks like the obvious and correct fix (and clearly makes the test pass ;) ). -Peff ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] t5606: run clone branch name test with protocol v2 2021-03-17 15:42 ` [PATCH] t5606: run clone branch name test with protocol v2 Jonathan Tan 2021-03-17 17:42 ` Jeff King @ 2021-03-17 18:18 ` Junio C Hamano 1 sibling, 0 replies; 10+ messages in thread From: Junio C Hamano @ 2021-03-17 18:18 UTC (permalink / raw) To: Jonathan Tan; +Cc: git, sluongng Jonathan Tan <jonathantanmy@google.com> writes: > 4f37d45706 ("clone: respect remote unborn HEAD", 2021-02-05) introduces > a new feature (if the remote has an unborn HEAD, e.g. when the remote > repository is empty, use it as the name of the branch) that only works > in protocol v2, but did not ensure that one of its tests always uses > protocol v2, and thus that test would fail if > GIT_TEST_PROTOCOL_VERSION=0 (or 1) is used. Therefore, add "-c > protocol.version=2" to the appropriate test. > > (The rest of the tests from that commit have "-c protocol.version=2" > already added.) > > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- > Thanks, Son Luong, for noticing this. Here's a fix for the > GIT_TEST_PROTOCOL_VERSION part. This was built on 4f37d45706 but also > applies cleanly on master. Makes sense. And I do not see need for any other changes, like test_expect_failure with protocol 0 (or 1). Will queue as a candidate for maint-2.31. Thanks. > > t/t5606-clone-options.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh > index ca6339a5fb..5e30772735 100755 > --- a/t/t5606-clone-options.sh > +++ b/t/t5606-clone-options.sh > @@ -106,7 +106,7 @@ test_expect_success 'chooses correct default initial branch name' ' > git -c init.defaultBranch=foo init --bare empty && > test_config -C empty lsrefs.unborn advertise && > GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \ > - git -c init.defaultBranch=up clone empty whats-up && > + git -c init.defaultBranch=up -c protocol.version=2 clone empty whats-up && > test refs/heads/foo = $(git -C whats-up symbolic-ref HEAD) && > test refs/heads/foo = $(git -C whats-up config branch.foo.merge) > ' ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Tests failed with GIT_TEST_FAIL_PREREQS and/or GIT_TEST_PROTOCOL_VERSION 2021-03-17 13:38 ` Son Luong Ngoc 2021-03-17 15:42 ` [PATCH] t5606: run clone branch name test with protocol v2 Jonathan Tan @ 2021-03-17 17:54 ` Jeff King 2021-03-17 22:47 ` [PATCH] t: annotate !PTHREADS tests with !FAIL_PREREQS Jeff King 1 sibling, 1 reply; 10+ messages in thread From: Jeff King @ 2021-03-17 17:54 UTC (permalink / raw) To: Son Luong Ngoc; +Cc: Taylor Blau, git, avarab, jonathantanmy, gitster On Wed, Mar 17, 2021 at 02:38:18PM +0100, Son Luong Ngoc wrote: > 1. For t7810 and t5300 failing when GIT_TEST_FAIL_PREREQS=1: > > a926c4b904bdc339568c2898af955cdc61b31542 is the first bad commit > commit a926c4b904bdc339568c2898af955cdc61b31542 > Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > Date: Thu Feb 11 02:53:51 2021 +0100 > > tests: remove most uses of C_LOCALE_OUTPUT > > As a follow-up to d162b25f956 (tests: remove support for > GIT_TEST_GETTEXT_POISON, 2021-01-20) remove those uses of the now > always true C_LOCALE_OUTPUT prerequisite from those tests which > declare it as an argument to test_expect_{success,failure}. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > Signed-off-by: Junio C Hamano <gitster@pobox.com> I looked at the one in t5300, and I don't think it _ever_ worked, nor can it be made to work in this mode. It is expecting that running: git index-pack --threads=2 will issue a warning in a build without pthread support. But in the fake "pretend the pthread prereq is not satisfied" mode, it will of course not do that, because the build itself is not aware that it's supposed to be pretending that pthreads aren't supported! Before the patch mentioned above, its prereqs were: !PTHREADS,C_LOCALE_OUTPUT which would never be satisfied under the "pretend" mode, because it would _also_ disable C_LOCALE_OUTPUT, and we'd always skip it. So I think something like this creates a similar situation; diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh index d586fdc7a9..87d26bb70c 100755 --- a/t/t5300-pack-object.sh +++ b/t/t5300-pack-object.sh @@ -427,7 +427,8 @@ test_expect_success 'index-pack --strict <pack> works in non-repo' ' test_path_is_file foo.idx ' -test_expect_success !PTHREADS 'index-pack --threads=N or pack.threads=N warns when no pthreads' ' +test_expect_success !PTHREADS,IGNORE_FAIL_PREREQS \ + 'index-pack --threads=N or pack.threads=N warns when no pthreads' ' test_must_fail git index-pack --threads=2 2>err && grep ^warning: err >warnings && test_line_count = 1 warnings && @@ -445,7 +446,8 @@ test_expect_success !PTHREADS 'index-pack --threads=N or pack.threads=N warns wh grep -F "no threads support, ignoring pack.threads" err ' -test_expect_success !PTHREADS 'pack-objects --threads=N or pack.threads=N warns when no pthreads' ' +test_expect_success !PTHREADS,IGNORE_FAIL_PREREQS \ + 'pack-objects --threads=N or pack.threads=N warns when no pthreads' ' git pack-objects --threads=2 --stdout --all </dev/null >/dev/null 2>err && grep ^warning: err >warnings && test_line_count = 1 warnings && but I think this points to a failing of the FAIL_PREREQS mode. It is generally OK to say "skip this test by pretending you do not have a prereq satisfied" (and that is the point: to see if skipping a test confuses later tests). But given a negated prereq here, it is not OK to say "run this test that we usually wouldn't", because it is almost certainly going to be mismatched with the actual build. So I think the FAIL_PREREQS mode should probably be treating negated prereqs differently (and always pretending that yes, we have them). I hadn't investigated the t7810 case yet, but looking at it now, it seems to be the exact same thing. -Peff ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] t: annotate !PTHREADS tests with !FAIL_PREREQS 2021-03-17 17:54 ` Tests failed with GIT_TEST_FAIL_PREREQS and/or GIT_TEST_PROTOCOL_VERSION Jeff King @ 2021-03-17 22:47 ` Jeff King 2021-03-18 21:17 ` Junio C Hamano 0 siblings, 1 reply; 10+ messages in thread From: Jeff King @ 2021-03-17 22:47 UTC (permalink / raw) To: Son Luong Ngoc; +Cc: Taylor Blau, git, avarab, jonathantanmy, gitster On Wed, Mar 17, 2021 at 01:54:25PM -0400, Jeff King wrote: > -test_expect_success !PTHREADS 'pack-objects --threads=N or pack.threads=N warns when no pthreads' ' > +test_expect_success !PTHREADS,IGNORE_FAIL_PREREQS \ > + 'pack-objects --threads=N or pack.threads=N warns when no pthreads' ' > git pack-objects --threads=2 --stdout --all </dev/null >/dev/null 2>err && > grep ^warning: err >warnings && > test_line_count = 1 warnings && > > but I think this points to a failing of the FAIL_PREREQS mode. It is > generally OK to say "skip this test by pretending you do not have a > prereq satisfied" (and that is the point: to see if skipping a test > confuses later tests). But given a negated prereq here, it is not OK to > say "run this test that we usually wouldn't", because it is almost > certainly going to be mismatched with the actual build. > > So I think the FAIL_PREREQS mode should probably be treating negated > prereqs differently (and always pretending that yes, we have them). > > I hadn't investigated the t7810 case yet, but looking at it now, it > seems to be the exact same thing. It looks like the problem is indeed somewhat widespread, and there is a magic prereq already to skip such tests. I do still think that this is a fundamental failing of the FAIL_PREREQS mode, but it probably makes sense to annotate these tests in the meantime (I don't plan on looking further into it myself). Another rough edge I noticed: if you set GIT_TEST_HTTPD or GIT_TEST_GIT_DAEMON to "yes" in your config.mak, these play quite badly with GIT_TEST_FAIL_PREREQS. We think NOT_ROOT is not satisfied, so refuse to start httpd, and then complain that the setup fails (and the point of "yes" for those values is to loudly complain when setup fails, rather than quietly skipping the tests). -- >8 -- Subject: [PATCH] t: annotate !PTHREADS tests with !FAIL_PREREQS Some tests in t5300 and t7810 expect us to complain about a "--threads" argument when Git is compiled without pthread support. Running these under GIT_TEST_FAIL_PREREQS produces a confusing failure: we pretend to the tests that there is no pthread support, so they expect the warning, but of course the actual build is perfectly happy to respect the --threads argument. We never noticed before the recent a926c4b904 (tests: remove most uses of C_LOCALE_OUTPUT, 2021-02-11), because the tests also were marked as requiring the C_LOCALE_OUTPUT prerequisite. Which means they'd never have run in FAIL_PREREQS mode, since it would always pretend that the locale prereq was not satisfied. These tests can't possibly work in this mode; it is a mismatch between what the tests expect and what the build was told to do. So let's just mark them to be skipped, using the special prereq introduced by dfe1a17df9 (tests: add a special setup where prerequisites fail, 2019-05-13). Reported-by: Son Luong Ngoc <sluongng@gmail.com> Signed-off-by: Jeff King <peff@peff.net> --- t/t5300-pack-object.sh | 6 ++++-- t/t7810-grep.sh | 3 ++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh index d586fdc7a9..e830a37a38 100755 --- a/t/t5300-pack-object.sh +++ b/t/t5300-pack-object.sh @@ -427,7 +427,8 @@ test_expect_success 'index-pack --strict <pack> works in non-repo' ' test_path_is_file foo.idx ' -test_expect_success !PTHREADS 'index-pack --threads=N or pack.threads=N warns when no pthreads' ' +test_expect_success !PTHREADS,!FAIL_PREREQS \ + 'index-pack --threads=N or pack.threads=N warns when no pthreads' ' test_must_fail git index-pack --threads=2 2>err && grep ^warning: err >warnings && test_line_count = 1 warnings && @@ -445,7 +446,8 @@ test_expect_success !PTHREADS 'index-pack --threads=N or pack.threads=N warns wh grep -F "no threads support, ignoring pack.threads" err ' -test_expect_success !PTHREADS 'pack-objects --threads=N or pack.threads=N warns when no pthreads' ' +test_expect_success !PTHREADS,!FAIL_PREREQS \ + 'pack-objects --threads=N or pack.threads=N warns when no pthreads' ' git pack-objects --threads=2 --stdout --all </dev/null >/dev/null 2>err && grep ^warning: err >warnings && test_line_count = 1 warnings && diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh index edfaa9a6d1..5830733f3d 100755 --- a/t/t7810-grep.sh +++ b/t/t7810-grep.sh @@ -969,7 +969,8 @@ do " done -test_expect_success !PTHREADS 'grep --threads=N or pack.threads=N warns when no pthreads' ' +test_expect_success !PTHREADS,!FAIL_PREREQS \ + 'grep --threads=N or pack.threads=N warns when no pthreads' ' git grep --threads=2 Hello hello_world 2>err && grep ^warning: err >warnings && test_line_count = 1 warnings && -- 2.31.0.559.g509d4a088b ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] t: annotate !PTHREADS tests with !FAIL_PREREQS 2021-03-17 22:47 ` [PATCH] t: annotate !PTHREADS tests with !FAIL_PREREQS Jeff King @ 2021-03-18 21:17 ` Junio C Hamano 2021-03-18 21:53 ` Jeff King 0 siblings, 1 reply; 10+ messages in thread From: Junio C Hamano @ 2021-03-18 21:17 UTC (permalink / raw) To: Jeff King; +Cc: Son Luong Ngoc, Taylor Blau, git, avarab, jonathantanmy Jeff King <peff@peff.net> writes: >> So I think the FAIL_PREREQS mode should probably be treating negated >> prereqs differently (and always pretending that yes, we have them). >> >> I hadn't investigated the t7810 case yet, but looking at it now, it >> seems to be the exact same thing. > > It looks like the problem is indeed somewhat widespread, and there is a > magic prereq already to skip such tests. > > I do still think that this is a fundamental failing of the FAIL_PREREQS > mode, but it probably makes sense to annotate these tests in the > meantime (I don't plan on looking further into it myself). The README file in t/ directory claims that this "is useful for discovering issues with the tests where say a later test implicitly depends on an optional earlier test." but apparently it does not work well with these negated prerequisites. Its implementation probably should force a safe bypass of the whole test_have_prereq() etc. done in test_skip by hooking into test_verify_prereq and overwrite any non-empty test_prereq with a single hardcoded PRETEND_FAIL_PREREQ prerequisite that is never satisfied, or something. > Another rough edge I noticed: if you set GIT_TEST_HTTPD or > GIT_TEST_GIT_DAEMON to "yes" in your config.mak, these play quite badly > with GIT_TEST_FAIL_PREREQS. We think NOT_ROOT is not satisfied, so > refuse to start httpd, and then complain that the setup fails (and the > point of "yes" for those values is to loudly complain when setup fails, > rather than quietly skipping the tests). ... and I think this would also be gone, as the NOT_ROOT test is done with test_have_prereq that we wouldn't be mucking with if we limit the FAIL_PREREQS only to tweak the test_expect_* prereqs. In short, the biggest mistake in the current FAIL_PREREQS design is to hook into test_have_prereq while the stated objective only needs to futz with the prerequisite given to the test_expect_* functions, I would think. > -- >8 -- > Subject: [PATCH] t: annotate !PTHREADS tests with !FAIL_PREREQS > > Some tests in t5300 and t7810 expect us to complain about a "--threads" > argument when Git is compiled without pthread support. Running these > under GIT_TEST_FAIL_PREREQS produces a confusing failure: we pretend to > the tests that there is no pthread support, so they expect the warning, > but of course the actual build is perfectly happy to respect the > --threads argument. > > We never noticed before the recent a926c4b904 (tests: remove most uses > of C_LOCALE_OUTPUT, 2021-02-11), because the tests also were marked as > requiring the C_LOCALE_OUTPUT prerequisite. Which means they'd never > have run in FAIL_PREREQS mode, since it would always pretend that the > locale prereq was not satisfied. > > These tests can't possibly work in this mode; it is a mismatch between > what the tests expect and what the build was told to do. So let's just > mark them to be skipped, using the special prereq introduced by > dfe1a17df9 (tests: add a special setup where prerequisites fail, > 2019-05-13). > > Reported-by: Son Luong Ngoc <sluongng@gmail.com> > Signed-off-by: Jeff King <peff@peff.net> > --- > t/t5300-pack-object.sh | 6 ++++-- > t/t7810-grep.sh | 3 ++- > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh > index d586fdc7a9..e830a37a38 100755 > --- a/t/t5300-pack-object.sh > +++ b/t/t5300-pack-object.sh > @@ -427,7 +427,8 @@ test_expect_success 'index-pack --strict <pack> works in non-repo' ' > test_path_is_file foo.idx > ' > > -test_expect_success !PTHREADS 'index-pack --threads=N or pack.threads=N warns when no pthreads' ' > +test_expect_success !PTHREADS,!FAIL_PREREQS \ > + 'index-pack --threads=N or pack.threads=N warns when no pthreads' ' > test_must_fail git index-pack --threads=2 2>err && > grep ^warning: err >warnings && > test_line_count = 1 warnings && > @@ -445,7 +446,8 @@ test_expect_success !PTHREADS 'index-pack --threads=N or pack.threads=N warns wh > grep -F "no threads support, ignoring pack.threads" err > ' > > -test_expect_success !PTHREADS 'pack-objects --threads=N or pack.threads=N warns when no pthreads' ' > +test_expect_success !PTHREADS,!FAIL_PREREQS \ > + 'pack-objects --threads=N or pack.threads=N warns when no pthreads' ' > git pack-objects --threads=2 --stdout --all </dev/null >/dev/null 2>err && > grep ^warning: err >warnings && > test_line_count = 1 warnings && > diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh > index edfaa9a6d1..5830733f3d 100755 > --- a/t/t7810-grep.sh > +++ b/t/t7810-grep.sh > @@ -969,7 +969,8 @@ do > " > done > > -test_expect_success !PTHREADS 'grep --threads=N or pack.threads=N warns when no pthreads' ' > +test_expect_success !PTHREADS,!FAIL_PREREQS \ > + 'grep --threads=N or pack.threads=N warns when no pthreads' ' > git grep --threads=2 Hello hello_world 2>err && > grep ^warning: err >warnings && > test_line_count = 1 warnings && ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] t: annotate !PTHREADS tests with !FAIL_PREREQS 2021-03-18 21:17 ` Junio C Hamano @ 2021-03-18 21:53 ` Jeff King 0 siblings, 0 replies; 10+ messages in thread From: Jeff King @ 2021-03-18 21:53 UTC (permalink / raw) To: Junio C Hamano; +Cc: Son Luong Ngoc, Taylor Blau, git, avarab, jonathantanmy On Thu, Mar 18, 2021 at 02:17:19PM -0700, Junio C Hamano wrote: > In short, the biggest mistake in the current FAIL_PREREQS design is > to hook into test_have_prereq while the stated objective only needs > to futz with the prerequisite given to the test_expect_* functions, > I would think. Yeah, that matches my intuition of the problem, too. -Peff ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-03-18 21:54 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-16 9:45 Tests failed with GIT_TEST_FAIL_PREREQS and/or GIT_TEST_PROTOCOL_VERSION Son Luong Ngoc 2021-03-16 13:52 ` Taylor Blau 2021-03-17 13:38 ` Son Luong Ngoc 2021-03-17 15:42 ` [PATCH] t5606: run clone branch name test with protocol v2 Jonathan Tan 2021-03-17 17:42 ` Jeff King 2021-03-17 18:18 ` Junio C Hamano 2021-03-17 17:54 ` Tests failed with GIT_TEST_FAIL_PREREQS and/or GIT_TEST_PROTOCOL_VERSION Jeff King 2021-03-17 22:47 ` [PATCH] t: annotate !PTHREADS tests with !FAIL_PREREQS Jeff King 2021-03-18 21:17 ` Junio C Hamano 2021-03-18 21:53 ` Jeff King
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).