Re: [PATCH 2/6] t7408: merge short tests, factor out testing method
flat view
From: Junio C Hamano <hidden>
Date: 2016-08-05 20:45:44
Stefan Beller [off-list ref] writes:
Tests consisting of one line each can be consolidated to have fewer tests to run as well as fewer lines of code. When having just a few git commands, do not create a new shell but use the -C flag in Git to execute in the correct directory.
Good motivations.
quoted hunk
Signed-off-by: Stefan Beller <redacted> --- t/t7408-submodule-reference.sh | 50 +++++++++++++++--------------------------- 1 file changed, 18 insertions(+), 32 deletions(-)diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh index afcc629..1416cbd 100755 --- a/t/t7408-submodule-reference.sh +++ b/t/t7408-submodule-reference.sh@@ -10,6 +10,16 @@ base_dir=$(pwd) U=$base_dir/UPLOAD_LOG
Is this line needed anywhere? We (perhaps unfortunately) still need $base_dir because we want to give an absolute file:/// URL to "submodule add", but I do not think we use $U, so let's get rid of it.
+test_alternate_usage()
+{
According to Documentation/CodingGuidelines, this should be:
test_alternate_usage () {
Somehow the helper name sounds as if it is testing if an alternate
is used correctly (i.e. the machinery may attempt to use alternate
but not in a correct way), not testing if an alternate is correctly
used (i.e. the machinery incorrectly forgets to use an alternate at
all), but maybe it is just me.
+ alternates_file=$1 + working_dir=$2
These are good (they can be on a single line), but you would want &&-chain just as other lines.
+ test_line_count = 1 $alternates_file &&
This needs to quote "$alternates_file" especially in a helper function you have no control over future callers of. I wonder if we want to check the actual contents of the alternate; it may force us to worry about the infamous "should we expect $(pwd)/something or $PWD/something" if we did so, so it is not a strong suggestion.
quoted hunk
+ echo "0 objects, 0 kilobytes" >expect && + git -C $working_dir count-objects >current && + diff expect current
It is more customary to name two "expect" vs "actual", and compare them using "test_cmp" not "diff".
quoted hunk
+} + test_expect_success 'preparing first repository' ' test_create_repo A && (@@ -42,44 +52,20 @@ test_expect_success 'preparing superproject' ' ) ' -test_expect_success 'submodule add --reference' ' +test_expect_success 'submodule add --reference uses alternates' ' ( cd super && git submodule add --reference ../B "file://$base_dir/A" sub && git commit -m B-super-added - ) -' - -test_expect_success 'after add: existence of info/alternates' ' - test_line_count = 1 super/.git/modules/sub/objects/info/alternates -' - -test_expect_success 'that reference gets used with add' ' - ( - cd super/sub && - echo "0 objects, 0 kilobytes" > expected && - git count-objects > current && - diff expected current - ) -'
Completely unrelated tangent, but after seeing the "how would we make a more intelligent choice of the diff boundary" topic, I wondered if we can notice that at this point there is a logical boundary and do something intelligent about it. All the removed lines above have become "test_alternate" we see below, while all the removed lines below upto "test_alternate" correspond to the updated test at the end.
quoted hunk
-test_expect_success 'cloning superproject' ' - git clone super super-clone -' - -test_expect_success 'update with reference' ' - cd super-clone && git submodule update --init --reference ../B -' - -test_expect_success 'after update: existence of info/alternates' ' - test_line_count = 1 super-clone/.git/modules/sub/objects/info/alternates + ) && + test_alternate_usage super/.git/modules/sub/objects/info/alternates super/sub ' -test_expect_success 'that reference gets used with update' ' - cd super-clone/sub && - echo "0 objects, 0 kilobytes" > expected && - git count-objects > current && - diff expected current +test_expect_success 'updating superproject keeps alternates' ' + test_when_finished "rm -rf super-clone" &&
This one is new; we do not remove A, B or super. Does it matter if we leave super-clone behind? Is super-clone so special?
quoted hunk
+ git clone super super-clone && + git -C super-clone submodule update --init --reference ../B && + test_alternate_usage super-clone/.git/modules/sub/objects/info/alternates super-clone/sub ' test_done