Re: [RFC PATCH 2/6] t4041, t4060: modernize test style
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 13, 2023, 19:41 UTC
- Message-ID
- <xmqqedqtbbf4.fsf@gitster.g>
- In-Reply-To
- <20230213182134.2173280-3-calvinwan@google.com>
Calvin Wan <calvinwan@google.com> writes:
Show 5 quoted lines
> From: Josh Steadmon <steadmon@google.com> > > In preparation for later changes, move setup code into test_expect > blocks. Smaller sections are moved into existing blocks, while larger > sections with a standalone purpose are given their own new blocks.
Nice.
Show 8 quoted lines
> -test_create_repo sm1 && > -add_file . foo >/dev/null > - > -head1=$(add_file sm1 foo1 foo2) > -fullhead1=$(cd sm1; git rev-parse --verify HEAD) > +test_expect_success 'setup' ' > + test_create_repo sm1 && > + add_file . foo >/dev/null &&
Now this is inside test_expect_success, redirection to /dev/null is unnecessary.
> + head1=$(add_file sm1 foo1 foo2) && > + fullhead1=$(cd sm1 && git rev-parse --verify HEAD) > +'
Or "fullhead1=$(git -C sm1 rev-parse ...)".
Both of the above can be ignored if we are trying to be a strict rewrite of the original, but moving code inside test_expect_success block is a large enough change that there may not be much point in avoiding such an obvious modernization "while at it".
Show 6 quoted lines
> -rm sm2 > -mv sm2-bak sm2 > - > test_expect_success 'setup nested submodule' ' > + rm sm2 && > + mv sm2-bak sm2 &&
To me, this looks more like something test_when_finished in the test that wanted not to have sm2 (i.e. "deleted submodule with .git file") should have done as part of its own clean-up.
There certainly can be two schools of thought when it comes to how to arrange the precondition of subsequent tests. More modern tests tend to clean after themselves by reverting the damage they made to the environment inside test_when_finished in themselves. This one, as the posted patch does, goes to the other extreme and forces the subsequent test to undo the damage done by the previous ones.
The latter approach has two major downsides. It would not work if the tester wants to skip an earlier step, or if an earlier step failed to cause the expected damage this step wants to undo. The correctness of "what we should see as sm2 here must be in sm2-bak because we know an earlier step should have moved it there" can easily be broken. It also makes it harder to update the earlier step to cause different damage to the environment---the "undoing the damage done by the previous step(s)" done as early parts of this step also needs to be updated.
Whichever approach we pick to use in each script, it would be better to stick to one philosophy, and if we can make each step revert the damage it caused when it is done, that would be nice.
Show 6 quoted lines
> -mv sm2-bak sm2 > +test_expect_success 'submodule cleanup' ' > + mv sm2-bak sm2 > +' > > test_done
Likewise.
Thanks.