Re: [PATCH 4/4] t7401: add a WARNING and a NEEDSWORK

From: Taylor Blau <me@ttaylorr.com>
To: Shourya Shukla <shouryashukla.oo@gmail.com>
Cc: git@vger.kernel.org, gitster@pobox.com,
	christian.couder@gmail.com, kaartic.sivaraam@gmail.com,
	johannes.schindelin@gmx.de, liu.denton@gmail.com,
	Christian Couder <chriscool@tuxfamily.org>
Subject: Re: [PATCH 4/4] t7401: add a WARNING and a NEEDSWORK
Date: Wed, 5 Aug 2020 17:04:08 -0400	[thread overview]
Message-ID: <20200805210408.GI9546@syl.lan> (raw)
In-Reply-To: <20200805174921.16000-5-shouryashukla.oo@gmail.com>

On Wed, Aug 05, 2020 at 11:19:21PM +0530, Shourya Shukla wrote:
> Add a WARNING regarding the usage of 'git add' instead of 'git
> submodule add' to add submodules to the superproject. Also add a
> NEEDSWORK regarding the outdated syntax and working of the test, which
> may need to be improved to obtain better and desired results.
>
> While at it, change the word 'test' to 'test script' in the test
> description to avoid ambiguity.
>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> ---
>  t/t7401-submodule-summary.sh | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
> index 145914cd5a..2db4cf5cbf 100755
> --- a/t/t7401-submodule-summary.sh
> +++ b/t/t7401-submodule-summary.sh
> @@ -5,8 +5,13 @@
>
>  test_description='Summary support for submodules
>
> -This test tries to verify the sanity of summary subcommand of git submodule.
> +This test script tries to verify the sanity of summary subcommand of git submodule.
>  '
> +# WARNING: This test script uses 'git add' instead of 'git submodule add' to add
> +# submodules to the superproject. Some submodule subcommands such as init and
> +# deinit might not work as expected in this script.
> +
> +# NEEDSWORK: This test script is old fashioned and may need a big cleanup.

It would be worth saying why, especially if you're re-rolling anyway.
Even something as simple as: "there are lots of commands taking place
outside of a 'test_expect_{success,failure}' block, which is no longer
in good-style".

I also wouldn't be upset to see some of those fixed up in this series,
but I realize that may be a bigger endeavor than you are willing to take
on for now.

>
>  . ./test-lib.sh
>
> --
> 2.27.0

Thanks,
Taylor