Re: [GSOC][PATCH] t5500: simplify test implementation and fix git exit code suppression
Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:
Show 5 quoted lines
> The 'shallow since with commit graph and already-seen commit” test previously used a
> convoluted here-doc that combined manual input construction with packetize, echo and
> embedded Git commands. This structure hid failures from the git commands, as their
> exit codes were suppressed inside echo command substitution and pipe upstream,
> also making the test harder to follow.
Very nicely written problem statement, but your line-wrap setting is way too big.
Drop "previously". The problem statement that describes the status quo is written in the present tense. I.e., "Now it is X, which has problems Y and Z", not "It used to be X (before this patch), which had problems Y and Z".
"and pipe upstream" -> ??? "and being on the upstream side of pipes"???
> The changes simplify and make the test more robust.
And then you tell somebody sitting in front of a keyboard to make changes to the code to make it better. I.e.,
Instead of computing the pack protocol lines inside here-doc
that is fed to the program being tested (i.e., 'git
upload-pack'), use the "test-tool pkt-line pack" helper to
prepare the input to the command in a file first, and then feed
it to the command. This has a few advantages: - It makes debugging of the pkt-lines that are fed to the
command easier. - We no longer need to count number of bytes on each line
ourselves; the tool does it for us. - Execution of "git" commands are done outside the here-doc,
and it is easier to see any failure would be captured before
we even run the "git upload-pack" test.or something, perhaps?
Show 27 quoted lines
> * Assign the results of Git commands to variables up front and chain them with &&,
> so the test detects any failures immediately, avoiding any exit code suppression.
>
> * Use test-tool pkt-line pack to construct the input and then pass it to git-upload
> in a temp file, instead of relying on here-doc and manual packetization.
> This avoids formatting issues and ensures correct v2 protocol guidelines.
>
> Signed-off-by: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>
> ---
> t/t5500-fetch-pack.sh | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index 2677cd5faa..62cf0e1ff7 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -892,15 +892,20 @@ test_expect_success 'shallow since with commit graph and already-seen commit' '
> test_commit other &&
> git commit-graph write --reachable &&
> git config core.commitGraph true &&
> -
> - GIT_PROTOCOL=version=2 git upload-pack . <<-EOF >/dev/null
> - 0012command=fetch
> - $(echo "object-format=$(test_oid algo)" | packetize)
> - 00010013deepen-since 1
> - $(echo "want $(git rev-parse other)" | packetize)
> - $(echo "have $(git rev-parse main)" | packetize)
> + oid_algo=$(test_oid algo) &&
> + oid_other=$(git rev-parse other) &&
> + oid_main=$(git rev-parse main) &&
Show 9 quoted lines
> + test-tool pkt-line pack >input <<-EOF &&
> + command=fetch
> + object-format=$oid_algo
> + 0001
> + deepen-since 1
> + want $oid_other
> + have $oid_main
> 0000
> EOF
> + GIT_PROTOCOL=version=2 git upload-pack . <input >/dev/null
There is a trailing whitespace on the above line.
> )
> '
Shreyansh Paliwal
