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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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?
-- 
Yaroslav O. Halchenko
Center for Open Neuroscience     http://centerforopenneuroscience.org
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
Phone: +1 (603) 646-9834                       Fax: +1 (603) 646-1419
WWW:   http://www.linkedin.com/in/yarik        



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux