[2/2] RF+ENH(TST): compare the entire list of submodule status --recursive to stay intact
diff mbox series

Message ID 20181211040839.17472-2-debian@onerussian.com
State New
Headers show
Series
  • [1/2] submodule: Add --reset-hard option for git submodule update
Related show

Commit Message

Yaroslav O Halchenko Dec. 11, 2018, 4:08 a.m. UTC
For submodule update --reset-hard the best test is comparison of the
entire status as shown by submodule status --recursive.  Upon update
--reset-hard we should get back to the original state, with all the
branches being the same (no detached HEAD) and commits identical to
original  (so no merges, new commits, etc).

For that, I have introduced two helpers: {record,compare}_submodules_status and
an additional test for --reset-hard in nested submodule.

I have kept this as a separate PATCH to demonstrate the diff from the original
test composition as introduced in the prior patch, and this one where
all tests could be of the same type:

    record_submodule_status &&
    perform evil actions &&
    ! compare_submodule_status &&   # to double check that evil was done
    git submodule --reset-hard . &&
    compare_submodule_status        # assure that we are all good

Signed-off-by: Yaroslav Halchenko <debian@onerussian.com>
---
 t/t7406-submodule-update.sh | 37 ++++++++++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 7 deletions(-)

Comments

Stefan Beller Dec. 12, 2018, 7:48 p.m. UTC | #1
On Mon, Dec 10, 2018 at 8:09 PM Yaroslav Halchenko
<debian@onerussian.com> wrote:

Thanks for the patches. The first patch looks good to me!

> [PATCH 2/2] RF+ENH(TST): compare the entire list of submodule status --recursive to stay intact

The subject is a bit cryptic (specifically the first part before the
colon), maybe

  t7406: compare entire submodule status for --reset-hard mode

?


> For submodule update --reset-hard the best test is comparison of the
> entire status as shown by submodule status --recursive.  Upon update
> --reset-hard we should get back to the original state, with all the
> branches being the same (no detached HEAD) and commits identical to
> original  (so no merges, new commits, etc).

"original state" can mean different things to different people. I'd think
we could be more precise:

   ... we should get to the state that the submodule is reset to the
    object id as the superprojects gitlink points at, irrespective of the
    submodule branch.


>  test_expect_success 'submodule update --merge staying on master' '
>         (cd super/submodule &&
> -         git reset --hard HEAD~1
> +        git reset --hard HEAD~1

unrelated white space change?

>         ) &&
>         (cd super &&
>          (cd submodule &&
> @@ -307,16 +318,28 @@ test_expect_success 'submodule update --merge staying on master' '
>  '
>
>  test_expect_success 'submodule update --reset-hard staying on master' '
> [..]
> +'
> +

The tests look good to me, though I wonder if we'd rather want to inline
{record/compare}_submodule_status as then you'd not need to look it up
and the functions are rather short?
Yaroslav O Halchenko Dec. 13, 2018, 4:42 p.m. UTC | #2
Thank you Stefan for the review and please pardon my delay with the
reply, and sorry it got a bit too long by the end ;)

On Wed, 12 Dec 2018, Stefan Beller wrote:
> Thanks for the patches. The first patch looks good to me!

Great!

> > [PATCH 2/2] RF+ENH(TST): compare the entire list of submodule status --recursive to stay intact

> The subject is a bit cryptic (specifically the first part before the
> colon), maybe

>   t7406: compare entire submodule status for --reset-hard mode

> ?


> > For submodule update --reset-hard the best test is comparison of the
> > entire status as shown by submodule status --recursive.  Upon update
> > --reset-hard we should get back to the original state, with all the
> > branches being the same (no detached HEAD) and commits identical to
> > original  (so no merges, new commits, etc).

> "original state" can mean different things to different people. I'd think
> we could be more precise:

>    ... we should get to the state that the submodule is reset to the
>     object id as the superprojects gitlink points at, irrespective of the
>     submodule branch.

ok, I will update the description.  But I wonder if there could be some
short term to be used to describe the composite "git submodule status"
and "git status" (refers to below ;)).

> >  test_expect_success 'submodule update --merge staying on master' '
> >         (cd super/submodule &&
> > -         git reset --hard HEAD~1
> > +        git reset --hard HEAD~1

> unrelated white space change?

I was tuning formatting to be uniform and I guess missed that this is in
the other (not my) test.  I will revert that piece, thanks!

BTW -- should I just squash to PATCHes now?  I kept them separate primarily to
show the use of those helpers:

> >         ) &&
> >         (cd super &&
> >          (cd submodule &&
> > @@ -307,16 +318,28 @@ test_expect_success 'submodule update --merge staying on master' '
> >  '

> >  test_expect_success 'submodule update --reset-hard staying on master' '
> > [..]
> > +'
> > +

> The tests look good to me, though I wonder if we'd rather want to inline
> {record/compare}_submodule_status as then you'd not need to look it up
> and the functions are rather short?

compare_submodules_status  is already a compound action, so code would
become quite more "loaded" if it is expanded, e.g. instead of 

	(cd super &&
	 record_submodules_status &&
	 (cd submodule &&
	  git reset --hard HEAD~1
	 ) &&
	 ! compare_submodules_status &&
	 git submodule update --reset-hard submodule &&
	 compare_submodules_status
	)

it would become something like this I guess?

	(cd super &&
	 git submodule status --recursive >expect &&
	 (cd submodule &&
	  git reset --hard HEAD~1
	 ) &&
	 ! {git submodule status --recursive >actual && 
        test_i18ncmp expect actual;} &&
	 git submodule update --reset-hard submodule &&
	 {git submodule status --recursive >actual && 
      test_i18ncmp expect actual;}
	)

IMHO a bit mouth full.  I was thinking also to extend compare_ with additional
testing e.g. using "git status" since "git submodule status" does not care
about untracked files etc.  For --reset-hard I would like to assure that it is
not just some kind of a mixed reset leaving files behind.  That would make
tests even more overloaded.

On that point: Although I also like explicit calls at times, I also do
like test fixtures as a concept to do more testing around the actual
test-specific code block, thus minimizing boiler plate, which even if explicit
makes code actually harder to grasp (at least to me).  

Since for the majority of the --reset-hard tests the fixture and test(s) are
pretty much the same, actually ideally I would have liked to have
something like this:

test_expect_unchanged_submodule_status 'submodule update --reset-hard staying on master' \
  super \
  '(cd submodule && git reset --hard HEAD~1)' \
  'git submodule update --reset-hard submodule'

where I just pass 
  the path to work in, 
  the test setup function, 
  and the test action.  

The rest (initial cd, record, run setup, verify that there is a change, run
action, verify there is no changes) is done by the
test_expect_unchanged_submodule_status in a uniform way, absorbing all the
boiler plate.  (I am not married to the name, could be more descriptive/generic
may be)

Then we could breed a good number of tests with little to no boiler plate, with
only relevant pieces and as extended as needed testing done by this
test_expect_unchanged_submodule_status helper. e.g smth like

test_expect_unchanged_submodule_status 'submodule update --reset-hard staying on master when I do a new commit' \
  super \
  '(cd submodule && git commit --allow-empty -m "new one"' \
  'git submodule update --reset-hard submodule'

and kaboom -- we have a new test.  If we decide to test more -- just tune up
test_expect_unchanged_submodule_status and done -- all the tests remain
sufficiently prescribed.

What do you think?
Stefan Beller Dec. 13, 2018, 8:44 p.m. UTC | #3
On Thu, Dec 13, 2018 at 8:42 AM Yaroslav O Halchenko
<debian@onerussian.com> wrote:
>
> Thank you Stefan for the review and please pardon my delay with the
> reply, and sorry it got a bit too long by the end ;)
>
> On Wed, 12 Dec 2018, Stefan Beller wrote:
> > Thanks for the patches. The first patch looks good to me!
>
> Great!
>
> > > [PATCH 2/2] RF+ENH(TST): compare the entire list of submodule status --recursive to stay intact
>
> > The subject is a bit cryptic (specifically the first part before the
> > colon), maybe
>
> >   t7406: compare entire submodule status for --reset-hard mode
>
> > ?
>
>
> > > For submodule update --reset-hard the best test is comparison of the
> > > entire status as shown by submodule status --recursive.  Upon update
> > > --reset-hard we should get back to the original state, with all the
> > > branches being the same (no detached HEAD) and commits identical to
> > > original  (so no merges, new commits, etc).
>
> > "original state" can mean different things to different people. I'd think
> > we could be more precise:
>
> >    ... we should get to the state that the submodule is reset to the
> >     object id as the superprojects gitlink points at, irrespective of the
> >     submodule branch.
>
> ok, I will update the description.  But I wonder if there could be some
> short term to be used to describe the composite "git submodule status"
> and "git status" (refers to below ;)).
>
> > >  test_expect_success 'submodule update --merge staying on master' '
> > >         (cd super/submodule &&
> > > -         git reset --hard HEAD~1
> > > +        git reset --hard HEAD~1
>
> > unrelated white space change?
>
> I was tuning formatting to be uniform and I guess missed that this is in
> the other (not my) test.  I will revert that piece, thanks!

The tests in that file are not quite following the coding style that is
currently deemed the best. So if you want to clean that up
as a preparatory patch, feel welcome to do so. :-)
(c.f. t/t7400-submodule-basic.sh for good style, specifically
indentation by tabs and the cd <path> on its own line in
a subshell)
The latest style update I found is
80938c39e2 (pack-objects test: modernize style, 2018-10-30)
and submodule related test style
31158c7efc (t7410: update to new style, 2018-08-15)

So I was not opposed to have style changes, but to have
multiple unrelated things in one patch (feature work vs
cleanup).

> BTW -- should I just squash to PATCHes now?  I kept them separate primarily to
> show the use of those helpers:

That would make sense.

> compare_submodules_status  is already a compound action, so code would
> become quite more "loaded" if it is expanded, e.g. instead of
...
> it would become something like this I guess?
...
>          ! {git submodule status --recursive >actual &&

you could keep the status out of the negation.

>         test_i18ncmp expect actual;} &&
>          git submodule update --reset-hard submodule &&
>          {git submodule status --recursive >actual &&
>       test_i18ncmp expect actual;}
>         )
>
> IMHO a bit mouth full.  I was thinking also to extend compare_ with additional
> testing e.g. using "git status" since "git submodule status" does not care
> about untracked files etc.  For --reset-hard I would like to assure that it is
> not just some kind of a mixed reset leaving files behind.  That would make
> tests even more overloaded.

ok, that makes sense.

> On that point: Although I also like explicit calls at times, I also do
> like test fixtures as a concept to do more testing around the actual
> test-specific code block, thus minimizing boiler plate, which even if explicit
> makes code actually harder to grasp (at least to me).
>
> Since for the majority of the --reset-hard tests the fixture and test(s) are
> pretty much the same, actually ideally I would have liked to have
> something like this:
>
> test_expect_unchanged_submodule_status 'submodule update --reset-hard staying on master' \
>   super \
>   '(cd submodule && git reset --hard HEAD~1)' \
>   'git submodule update --reset-hard submodule'
>
> where I just pass
>   the path to work in,
>   the test setup function,
>   and the test action.
>
> The rest (initial cd, record, run setup, verify that there is a change, run
> action, verify there is no changes) is done by the
> test_expect_unchanged_submodule_status in a uniform way, absorbing all the
> boiler plate.  (I am not married to the name, could be more descriptive/generic
> may be)

The issue with submodules is that we're already deviating from the
'standard' git test suite at times (See the submodule test suite
lib-submodule-update.sh that is used via t1013, t2013 or t3906
and others).

I guess if we keep the test_expect_unchanged_submodule_status
as a file local function, it could be okay.

> Then we could breed a good number of tests with little to no boiler plate, with
> only relevant pieces and as extended as needed testing done by this
> test_expect_unchanged_submodule_status helper. e.g smth like
>
> test_expect_unchanged_submodule_status 'submodule update --reset-hard staying on master when I do a new commit' \
>   super \
>   '(cd submodule && git commit --allow-empty -m "new one"' \

In new tests we're a big fan of using -C, as that can save the
subshell, i.e. replace the whole line by

    git -C submodule commit --allow-empty -m "new one"'  &&


>   'git submodule update --reset-hard submodule'
>
> and kaboom -- we have a new test.  If we decide to test more -- just tune up
> test_expect_unchanged_submodule_status and done -- all the tests remain
> sufficiently prescribed.
>
> What do you think?

That is pretty cool. Maybe my gut reaction on the previous patch
also had to do with the numbers, i.e. having 2 extra function for
only having 2 tests more legible. A framework is definitely better
once we have more tests.

Stefan
Yaroslav O Halchenko Dec. 13, 2018, 10:43 p.m. UTC | #4
On Thu, 13 Dec 2018, Stefan Beller wrote:

> > and kaboom -- we have a new test.  If we decide to test more -- just tune up
> > test_expect_unchanged_submodule_status and done -- all the tests remain
> > sufficiently prescribed.

> > What do you think?

> That is pretty cool. Maybe my gut reaction on the previous patch
> also had to do with the numbers, i.e. having 2 extra function for
> only having 2 tests more legible. A framework is definitely better
> once we have more tests.

cool, thanks for the feedback - I will then try to make it happen

quick one (so when I get to it I know):  should I replicate all those
tests you have for other update strategies? (treating of config
specifications etc)  There is no easy way to parametrize them somehow?
;)    In Python world I might have mocked the actual underlying call to
update, to see what option it would be getting and assure that it is the
one I specified via config, and then sweepped through all of them
to make sure nothing interim changes it.  Just wondering if may be
something like that exists in git's tests support.


BTW - sorry if RTFM and unrelated, is there  a way to  

    update --merge  

but allowing only  fast-forwards?  My use case is collection of this
submodules: http://datasets.datalad.org/?dir=/openneuro  which all
should come from github and I should not have any changes of my own.
Sure thing if all is clean etc, merge should result in fast-forward.  I
just do not want to miss a case where there was some (temporary?) "dirt"
which I forgot to reset and it would then get merged etc.
Stefan Beller Dec. 13, 2018, 11:58 p.m. UTC | #5
On Thu, Dec 13, 2018 at 2:44 PM Yaroslav O Halchenko
<debian@onerussian.com> wrote:
>
>
> On Thu, 13 Dec 2018, Stefan Beller wrote:
>
> > > and kaboom -- we have a new test.  If we decide to test more -- just tune up
> > > test_expect_unchanged_submodule_status and done -- all the tests remain
> > > sufficiently prescribed.
>
> > > What do you think?
>
> > That is pretty cool. Maybe my gut reaction on the previous patch
> > also had to do with the numbers, i.e. having 2 extra function for
> > only having 2 tests more legible. A framework is definitely better
> > once we have more tests.
>
> cool, thanks for the feedback - I will then try to make it happen
>
> quick one (so when I get to it I know):  should I replicate all those
> tests you have for other update strategies? (treating of config
> specifications etc)

If there is a sensible way to do so?
I have the impression that there are enough differences, that it
may not be possible to replicate all tests meaningfully from the
other modes.

> There is no easy way to parametrize them somehow?

There is t/lib-submodule-update.sh, which brings this to
an extreme, as it makes a "test suite in a test suite"; and I would
not follow that example for this change.

> ;)    In Python world I might have mocked the actual underlying call to
> update, to see what option it would be getting and assure that it is the
> one I specified via config, and then sweepped through all of them
> to make sure nothing interim changes it.  Just wondering if may be
> something like that exists in git's tests support.

gits tests are very heavy on end to end testing, i.e. run a whole command
and observe its output. This makes our command setup code, (i.e. finding
the repository, parsing options, reading possible config, etc) a really well
exercised code path. ;-)

There is a recent push towards testing only units, most of
t/helper is used for that, e.g. c.f. 4c7bb45269 (test-reach:
test get_reachable_subset, 2018-11-02).

So if you have a good idea how to focus the submodule
tests more on the (new) unit that you add, that would be cool.

> BTW - sorry if RTFM and unrelated, is there  a way to
>
>     update --merge
>
> but allowing only  fast-forwards?  My use case is collection of this
> submodules: http://datasets.datalad.org/?dir=/openneuro  which all
> should come from github and I should not have any changes of my own.

So you want the merge option  --ff-only
to be passed to the submodule merge command. I guess you could make
a patch, that update takes another option (--ff-only, only useful when
--merge is given), which is then propagated.

I am not sure if we could have a more generalized option passing,
which would allow to pass any option (for its respective command)
to the command that is run in the update mode.

> Sure thing if all is clean etc, merge should result in fast-forward.  I
> just do not want to miss a case where there was some (temporary?) "dirt"
> which I forgot to reset and it would then get merged etc.

maybe use --rebase, such that your potential change would bubble
up and possibly produce a merge conflict?
Yaroslav O Halchenko Dec. 14, 2018, 4:22 a.m. UTC | #6
On Thu, 13 Dec 2018, Stefan Beller wrote:

> > cool, thanks for the feedback - I will then try to make it happen

> > quick one (so when I get to it I know):  should I replicate all those
> > tests you have for other update strategies? (treating of config
> > specifications etc)

> If there is a sensible way to do so?
> I have the impression that there are enough differences, that it
> may not be possible to replicate all tests meaningfully from the
> other modes.

oh, by replicate I just meant to copy/paste and adjust for expected for
--reset-hard test behavior (and possibly introduced helper),
nothing fancy, just duplication as for replication ;-) 

> > There is no easy way to parametrize them somehow?

> There is t/lib-submodule-update.sh, which brings this to
> an extreme, as it makes a "test suite in a test suite"; and I would
> not follow that example for this change.

ok

> > ;)    In Python world I might have mocked the actual underlying call to
> > update, to see what option it would be getting and assure that it is the
> > one I specified via config, and then sweepped through all of them
> > to make sure nothing interim changes it.  Just wondering if may be
> > something like that exists in git's tests support.

> gits tests are very heavy on end to end testing, i.e. run a whole command
> and observe its output. This makes our command setup code, (i.e. finding
> the repository, parsing options, reading possible config, etc) a really well
> exercised code path. ;-)

> There is a recent push towards testing only units, most of
> t/helper is used for that, e.g. c.f. 4c7bb45269 (test-reach:
> test get_reachable_subset, 2018-11-02).

> So if you have a good idea how to focus the submodule
> tests more on the (new) unit that you add, that would be cool.

no, not really any good ideas -- I am new here, but I will keep an eye open.

> > BTW - sorry if RTFM and unrelated, is there  a way to

> >     update --merge

> > but allowing only  fast-forwards?  My use case is collection of this
> > submodules: http://datasets.datalad.org/?dir=/openneuro  which all
> > should come from github and I should not have any changes of my own.

> So you want the merge option  --ff-only
> to be passed to the submodule merge command. I guess you could make
> a patch, that update takes another option (--ff-only, only useful when
> --merge is given), which is then propagated.

> I am not sure if we could have a more generalized option passing,
> which would allow to pass any option (for its respective command)
> to the command that is run in the update mode.

wouldn't it be (theoretically) possible, in principle, to pass
them via some config variable?  e.g. instead of  

submodule update --reset-hard

have

-c submodule.update.reset.opts=--hard update --reset

and then analogously

-c submodule.update.merge.opts=--ff-only update --merge

(--ff-only I guess would make no sense for any "supermodule" - a repo
with submodules)

> > Sure thing if all is clean etc, merge should result in fast-forward.  I
> > just do not want to miss a case where there was some (temporary?) "dirt"
> > which I forgot to reset and it would then get merged etc.

> maybe use --rebase, such that your potential change would bubble
> up and possibly produce a merge conflict?

that is a good idea as a workaround, thanks!

Patch
diff mbox series

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 2e08e0047c..1927424f47 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -21,6 +21,17 @@  compare_head()
     test "$sha_master" = "$sha_head"
 }
 
+record_submodules_status()
+{
+	git submodule status --recursive >expect
+}
+
+compare_submodules_status()
+{
+	git submodule status --recursive >actual &&
+	test_i18ncmp expect actual
+}
+
 
 test_expect_success 'setup a submodule tree' '
 	echo file > file &&
@@ -294,7 +305,7 @@  test_expect_success 'submodule update --rebase staying on master' '
 
 test_expect_success 'submodule update --merge staying on master' '
 	(cd super/submodule &&
-	  git reset --hard HEAD~1
+	 git reset --hard HEAD~1
 	) &&
 	(cd super &&
 	 (cd submodule &&
@@ -307,16 +318,28 @@  test_expect_success 'submodule update --merge staying on master' '
 '
 
 test_expect_success 'submodule update --reset-hard staying on master' '
-	(cd super/submodule &&
-	  git reset --hard HEAD~1
-	) &&
 	(cd super &&
+	 record_submodules_status &&
 	 (cd submodule &&
-	  compare_head
+	  git reset --hard HEAD~1
 	 ) &&
+	 ! compare_submodules_status &&
 	 git submodule update --reset-hard submodule &&
-	 cd submodule &&
-	 compare_head
+	 compare_submodules_status
+	)
+'
+
+test_expect_success 'submodule update --reset-hard in nested submodule' '
+	(cd recursivesuper &&
+	 git submodule update --init --recursive &&
+	 record_submodules_status &&
+	 (cd super/submodule &&
+	  echo 123 >> file &&
+	  git commit -m "new commit" file
+	 ) &&
+	 ! compare_submodules_status &&
+	 git submodule update --reset-hard --recursive &&
+	 compare_submodules_status
 	)
 '