Re: [PATCH 2/6] t7408: merge short tests, factor out testing method

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.

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.

It is more customary to name two "expect" vs "actual", and compare
them using "test_cmp" not "diff".

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.

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?

On Fri, Aug 5, 2016 at 1:45 PM, Junio C Hamano [off-list ref] wrote:

Stefan Beller [off-list ref] writes:

quoted

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

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.

quoted

+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.

quoted

+     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.

quoted

+     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

+     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

+}
+
 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

This is where the "diff" and "current" above came from.

quoted

-     )
-'

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.

I guess that would require even more knowledge of the underlying
content that we track in Git.

Originally I started the diff boundary topic, as I assumed that the
new line detection is not doing harm. What Michael came up with
is impressive though I fear there might be a selection bias in the corpus,
i.e. we are missing some projects that get worse by it and these projects
would have had a great influence on the selection of the tuning parameters.
I guess we'll just wait until someone speaks up and points at worse
examples there.

What you're asking here is a complete new ballpark IMHO
as it takes diff to a whole new (syntactical) level. As of now
the world agrees that '\n' seems to be a good character to
put in text documents, so we use it as a splitting token
in our underlying diff driver, i.e. the diffs are primarily by line,
no matter how many characters change in that line. Sometimes
there is a "secondary" aspect such as coloring and pointing out
the characters that changed in a line[1].

[1] random example:
https://gerrit-review.googlesource.com/#/c/79354/3/project.py
Visually we focus on the changed characters, but the underlying
diff is still "by line".

We could write another "diff driver" that would not segment the
text by '\n' as a primary method and then showing the changed
hunks with +/-, but it would try to find rearrangements in the
underlying text:

As an example, this patch with such a diff driver could read partially as:
----8<----
***diff-driver: moved-text line 42 -> 14, length:7, post-operations: 1
 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
      )
***diff-driver:post-operation 1:
*** modify moved text with:

+test_alternate_usage() {
+        alternates_file="$1" &&
+        working_dir="$2" &&
*             echo "0 objects, 0 kilobytes" > expected &&
*             git count-objects > current &&
*             diff expected current
+ }

***diff-driver: deletion 40,50

 -     )
 -'
 -
 -test_expect_success 'after add: existence of info/alternates' '
 -     test_line_count = 1 super/.git/modules/sub/objects/info/alternates
 -'
 -
----8<----

Thinking about this further, this would not require knowledge of
the underlying content, it is actually "just" an intra-file-rename
detection.

We have a rename detection from one blob to another, so we
could also have a similar thing to deduplicate within one blob,
which would track moving code around.

quoted

-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?

"We need it in a later test."

It comes down to philosophy of how to write tests.

I spent some time in 740* and this is a surprising short test.
Compare to 7404 and 7400 for example. These are very long,
so when you want to add another test (no matter if testing for a
fixed regression or a new feature), you have lots of repos like

    super, super2, super3, sub, sub1, sumodule, anothersub,

and none of the names make sense. (Can I reuse them?
do they have some weird corner case configuration
that I need to undo? etc)

To stop that from happening again I want to have a clean slate,
i.e. all clones are deleted shortly after using, so it is obvious what
to use for testing.

quoted

+     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