Re: [PATCH v3 3/7] t7411: be nicer to future tests and really clean things up

[Antonio Ospite <ao2@ao2.it>]

On Tue, 14 Aug 2018 13:16:38 -0700
Junio C Hamano <gitster@pobox.com> wrote:

> Antonio Ospite <ao2@ao2.it> writes:
> 
[...]
> >  test_expect_success 'error message contains blob reference' '
> > +	# Remove the error introduced in the previous test.
> > +	# It is not needed in the following tests.
> > +	test_when_finished "git -C super reset --hard HEAD^" &&
> 
> Hmm, that is ugly.  Depending on where in the subshell the previous
> test failed, you'd still be taking us to an unexpected place.
> Imagine if "git commit -m 'add error'" failed, for example, in the
> test before this one.
> 
> I am wondering if the proper fix is to merge the previous one and
> this one into a single test.  The combined test would
> 
>     - remember where the HEAD in super is and arrange to come back
>       to it when test is done
>     - break .gitmodules and commit it
>     - run test-tool and check its output
>     - also check its error output
> 
> in a single test_expect_success.
>

I will try that.

> > @@ -123,6 +126,7 @@ test_expect_success 'using different treeishs works' '
> >  '
> >  
> >  test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
> > +	test_when_finished "git -C super reset --hard HEAD^" &&
> >  	(cd super &&
> >  		git config -f .gitmodules \
> >  			submodule.submodule.fetchrecursesubmodules blabla &&
> > @@ -134,8 +138,7 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
> >  			HEAD b \
> >  			HEAD submodule \
> >  				>actual &&
> > -		test_cmp expect_error actual  &&
> > -		git reset --hard HEAD^
> > +		test_cmp expect_error actual
> >  	)
> >  '
> 
> If we want to be more robust, you'd probably need to find a better
> anchoring point than HEAD, which can be pointing different commit
> depending on where in the subshell the process is hit with ^C,
> i.e.
> 
> 	ORIG=$(git -C super rev-parse HEAD) &&
> 	test_when_finished "git -C super reset --hard $ORIG" &&
> 	(
> 		cd super &&
> 		...
>

I see, ORIG is set and evaluated immediately but the value will be
used only at a later time.

I remember that you raised concerns also in the previous review round
but I didn't quite get what you meant, now I think I do.

> The patch is still an improvement compared to the current code,
> where a broken test-tool that does not produce expected output in
> the file 'actual' is guaranteed to leave us at a commit that we do
> not expect to be at, but not entirely satisfactory.

I can do a v4 with these fixes since there are also some comments about
other patches.

Thanks,
   Antonio

-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?