| Message ID | 20200506220741.71021-1-jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t5500: count objects through stderr, not trace | expand |
Jonathan Tan <jonathantanmy@google.com> writes: > In two tests introduced by 4fa3f00abb ("fetch-pack: in protocol v2, > in_vain only after ACK", 2020-04-28) and 2f0a093dd6 ("fetch-pack: in > protocol v2, reset in_vain upon ACK", 2020-04-28), the count of objects > downloaded is checked by grepping for a specific message in the packet > trace. However, this is flaky as that specific message may be delivered > over 2 or more packet lines. > > Instead, grep over stderr, just like the "fetch creating new shallow > root" test in the same file. > > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- > Thanks, Dscho. The commits introducing the flakiness have made it to > master, so this commit is on master. Hmph. Do we coalesce the payload we receive in adjacent two packet lines into one before writing it out, or do we know in this codepath that the progress messages are the only thing we are writing out to the standard error stream and we do not need to worry about other stuff getting mixed in? If that is the case, perhaps "just like we already do in another test" is a lot weaker justification (it sounds as if we are saying "another test is doing this thing, so if it is broken, we are not making things worse") than what you actually have ("the only thing we are writing out to the standard error stream in the non-error case is these progress messages, and the 'Total 3 ' string, even if it is carried in two separate fragments, will be shown without getting mixed with anything else on the standard error stream"). Assuming that the way I (re)read your justification is correct, the patch looks good to me. Thanks, both. > t/t5500-fetch-pack.sh | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh > index 52dd1a688c..8c54e34ef1 100755 > --- a/t/t5500-fetch-pack.sh > +++ b/t/t5500-fetch-pack.sh > @@ -386,7 +386,7 @@ test_expect_success 'clone shallow with packed refs' ' > ' > > test_expect_success 'in_vain not triggered before first ACK' ' > - rm -rf myserver myclient trace && > + rm -rf myserver myclient && > git init myserver && > test_commit -C myserver foo && > git clone "file://$(pwd)/myserver" myclient && > @@ -399,12 +399,12 @@ test_expect_success 'in_vain not triggered before first ACK' ' > # The new commit that the client wants to fetch. > test_commit -C myserver bar && > > - GIT_TRACE_PACKET="$(pwd)/trace" git -C myclient fetch --progress origin && > - test_i18ngrep "Total 3 " trace > + git -C myclient fetch --progress origin 2>log && > + test_i18ngrep "remote: Total 3 " log > ' > > test_expect_success 'in_vain resetted upon ACK' ' > - rm -rf myserver myclient trace && > + rm -rf myserver myclient && > git init myserver && > > # Linked list of commits on master. The first is common; the rest are > @@ -429,8 +429,8 @@ test_expect_success 'in_vain resetted upon ACK' ' > # first. The 256th commit is common between the client and the server, > # and should reset in_vain. This allows negotiation to continue until > # the client reports that first_anotherbranch_commit is common. > - GIT_TRACE_PACKET="$(pwd)/trace" git -C myclient fetch --progress origin master && > - test_i18ngrep "Total 3 " trace > + git -C myclient fetch --progress origin master 2>log && > + test_i18ngrep "Total 3 " log > ' > > test_expect_success 'fetch in shallow repo unreachable shallow objects' '
Junio C Hamano <gitster@pobox.com> writes: >> Thanks, Dscho. The commits introducing the flakiness have made it to >> master, so this commit is on master. I'll queue directly on top of jt/v2-fetch-neg-fix as that topic was designed to be later mergeable down to 'maint' so that the 2.26 track can (1) revert the default flip so protocol v0 is restored as the default, and (2) breakage of negotiataion in protocol v2 is fixed. Thanks.
Hi Jonathan, On Wed, 6 May 2020, Jonathan Tan wrote: > In two tests introduced by 4fa3f00abb ("fetch-pack: in protocol v2, > in_vain only after ACK", 2020-04-28) and 2f0a093dd6 ("fetch-pack: in > protocol v2, reset in_vain upon ACK", 2020-04-28), the count of objects > downloaded is checked by grepping for a specific message in the packet > trace. However, this is flaky as that specific message may be delivered > over 2 or more packet lines. > > Instead, grep over stderr, just like the "fetch creating new shallow > root" test in the same file. > > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- > Thanks, Dscho. The commits introducing the flakiness have made it to > master, so this commit is on master. Thank you for fixing this so quickly. I agree that the patch addresses the underlying problem. Thanks! Dscho > --- > t/t5500-fetch-pack.sh | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh > index 52dd1a688c..8c54e34ef1 100755 > --- a/t/t5500-fetch-pack.sh > +++ b/t/t5500-fetch-pack.sh > @@ -386,7 +386,7 @@ test_expect_success 'clone shallow with packed refs' ' > ' > > test_expect_success 'in_vain not triggered before first ACK' ' > - rm -rf myserver myclient trace && > + rm -rf myserver myclient && > git init myserver && > test_commit -C myserver foo && > git clone "file://$(pwd)/myserver" myclient && > @@ -399,12 +399,12 @@ test_expect_success 'in_vain not triggered before first ACK' ' > # The new commit that the client wants to fetch. > test_commit -C myserver bar && > > - GIT_TRACE_PACKET="$(pwd)/trace" git -C myclient fetch --progress origin && > - test_i18ngrep "Total 3 " trace > + git -C myclient fetch --progress origin 2>log && > + test_i18ngrep "remote: Total 3 " log > ' > > test_expect_success 'in_vain resetted upon ACK' ' > - rm -rf myserver myclient trace && > + rm -rf myserver myclient && > git init myserver && > > # Linked list of commits on master. The first is common; the rest are > @@ -429,8 +429,8 @@ test_expect_success 'in_vain resetted upon ACK' ' > # first. The 256th commit is common between the client and the server, > # and should reset in_vain. This allows negotiation to continue until > # the client reports that first_anotherbranch_commit is common. > - GIT_TRACE_PACKET="$(pwd)/trace" git -C myclient fetch --progress origin master && > - test_i18ngrep "Total 3 " trace > + git -C myclient fetch --progress origin master 2>log && > + test_i18ngrep "Total 3 " log > ' > > test_expect_success 'fetch in shallow repo unreachable shallow objects' ' > -- > 2.26.2.526.g744177e7f7-goog > >
Hi Jonathan, On Wed, 6 May 2020, Jonathan Tan wrote: > In two tests introduced by 4fa3f00abb ("fetch-pack: in protocol v2, > in_vain only after ACK", 2020-04-28) and 2f0a093dd6 ("fetch-pack: in > protocol v2, reset in_vain upon ACK", 2020-04-28), the count of objects > downloaded is checked by grepping for a specific message in the packet > trace. However, this is flaky as that specific message may be delivered > over 2 or more packet lines. > > Instead, grep over stderr, just like the "fetch creating new shallow > root" test in the same file. > > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- > Thanks, Dscho. The commits introducing the flakiness have made it to > master, so this commit is on master. Sorry for the blast from the past. Apparently, even with this fix, the first test is still flaky. I submitted a patch to work around the bug via https://github.com/gitgitgadget/git/pull/753; Please review when you have some time. Thanks, Dscho
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index 52dd1a688c..8c54e34ef1 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -386,7 +386,7 @@ test_expect_success 'clone shallow with packed refs' ' ' test_expect_success 'in_vain not triggered before first ACK' ' - rm -rf myserver myclient trace && + rm -rf myserver myclient && git init myserver && test_commit -C myserver foo && git clone "file://$(pwd)/myserver" myclient && @@ -399,12 +399,12 @@ test_expect_success 'in_vain not triggered before first ACK' ' # The new commit that the client wants to fetch. test_commit -C myserver bar && - GIT_TRACE_PACKET="$(pwd)/trace" git -C myclient fetch --progress origin && - test_i18ngrep "Total 3 " trace + git -C myclient fetch --progress origin 2>log && + test_i18ngrep "remote: Total 3 " log ' test_expect_success 'in_vain resetted upon ACK' ' - rm -rf myserver myclient trace && + rm -rf myserver myclient && git init myserver && # Linked list of commits on master. The first is common; the rest are @@ -429,8 +429,8 @@ test_expect_success 'in_vain resetted upon ACK' ' # first. The 256th commit is common between the client and the server, # and should reset in_vain. This allows negotiation to continue until # the client reports that first_anotherbranch_commit is common. - GIT_TRACE_PACKET="$(pwd)/trace" git -C myclient fetch --progress origin master && - test_i18ngrep "Total 3 " trace + git -C myclient fetch --progress origin master 2>log && + test_i18ngrep "Total 3 " log ' test_expect_success 'fetch in shallow repo unreachable shallow objects' '
In two tests introduced by 4fa3f00abb ("fetch-pack: in protocol v2, in_vain only after ACK", 2020-04-28) and 2f0a093dd6 ("fetch-pack: in protocol v2, reset in_vain upon ACK", 2020-04-28), the count of objects downloaded is checked by grepping for a specific message in the packet trace. However, this is flaky as that specific message may be delivered over 2 or more packet lines. Instead, grep over stderr, just like the "fetch creating new shallow root" test in the same file. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- Thanks, Dscho. The commits introducing the flakiness have made it to master, so this commit is on master. --- t/t5500-fetch-pack.sh | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)