Re: [PATCH v2] tests: modernize the test script t0010-racy-git.sh

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 29, 2024, 23:14 UTC
Message-ID: <xmqqle72c17i.fsf@gitster.g>
In-Reply-To: <pull.1675.v2.git.1709243831190.gitgitgadget@gmail.com>

"Aryan Gupta via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Aryan Gupta <garyan447@gmail.com>
>
> Modernize the formatting of the test script to align with current
> standards and improve its overall readability.

OK.

Show 9 quoted lines

> diff --git a/t/t0010-racy-git.sh b/t/t0010-racy-git.sh
> index 837c8b7228b..2ceac06c9c2 100755
> --- a/t/t0010-racy-git.sh
> +++ b/t/t0010-racy-git.sh
> @@ -1,6 +1,6 @@
>  #!/bin/sh
>  
> -test_description='racy GIT'
> +test_description='racy git'

This does not look like updating formatting to the current standard, or improving readability, though.

Show 14 quoted lines

> -	test_expect_success \
> -	"Racy GIT trial #$trial part A" \
> -	'test "" != "$files"'
> +	test_expect_success "Racy git trial #$trial part A" '
> +		test "" != "$files"
> +	'
> ...
> -	test_expect_success \
> -	"Racy GIT trial #$trial part B" \
> -	'test "" != "$files"'
> -
> +	test_expect_success "Racy git trial #$trial part B" '
> +		test "" != "$files"
> +	'

These are not wrong per-se, but if we are updating, I wonder if we want to get rid of statements outside the test_expect_success block, not just the formatting of individual test_expect_success tests.

Looking at an iteration of the loop, the executable statements from here ...

                rm -f .git/index
                echo frotz >infocom
                git update-index --add infocom
                echo xyzzy >infocom

                files=$(git diff-files -p)

... to here is what we would make part of one test, namely ...

                test_expect_success \
                "Racy GIT trial #$trial part A" \
                'test "" != "$files"'

... this one.  Then

                sleep 1

is a cricial delay to have before the next test, which we may want to have outside to make it clear what is going on to readers. But the following parts ...

                echo xyzzy >cornerstone
                git update-index --add cornerstone

                files=$(git diff-files -p)

... up to here, we would make part of the next test ...

                test_expect_success \
                "Racy GIT trial #$trial part B" \
                'test "" != "$files"'

... in the modern style.

So, we may want to do it more like this, perhaps?

	test_expect_success "Racy GIT trial #$trial part A" '
		rm -f .git/index &&
		echo frotz >infocom &&
		git update-index --add infocom &&
		echo xyzzy >infocom &&

		files=$(git diff-files -p) &&
                test "" != "$files"
	'

	sleep 1

	test_expect_success "Racy GIT trial #$trial part B" '
		echo xyzzy >cornerstone &&
		git update-index --add cornerstone &&

		files=$(git diff-files -p) &&
		test "" != "$files"
	'

An added benefit of the more modern style to place as much as possible to &&-chain in test_expect_success block is that we would catch breakage in "git update-index" and other things used to set-up the test as well. With the original loop, breakages in things outside the test_expect_success blocks will go unchecked.