On Thu, Dec 13, 2018 at 8:42 AM Yaroslav O Halchenko
<deb...@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
