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

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:

Great!

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 ;)).

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:

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?