threads / discuss / 42114

make test Unexpected passes

Subject: make test Unexpected passes

## tl;dr

6 messages between Apr 22, 2016 and Apr 27, 2016.

replies: 5people: 4as markdown or json

Ramsay Jones· Apr 22, 2016, 20:05 UTC · lore
Hi Ben, Junio,
Tonight, the testsuite passed with a couple of 'unexpected passes', viz:

$ tail -17 ptest-out [13:24:29] All tests successful.

Test Summary Report
-------------------
t3421-rebase-topology-linear.sh                  (Wstat: 0 Tests: 76 Failed: 0)
  TODO passed:   50, 54
t6036-recursive-corner-cases.sh                  (Wstat: 0 Tests: 22 Failed: 0)
  TODO passed:   11
Files=746, Tests=13515, 445 wallclock secs ( 3.83 usr  0.61 sys + 52.78 cusr 27.89 csys = 85.11 CPU)
Result: PASS
make clean-except-prove-cache
make[2]: Entering directory `/home/ramsay/git/t'
rm -f -r 'trash directory'.* 'test-results'
rm -f -r valgrind/bin
make[2]: Leaving directory `/home/ramsay/git/t'
make[1]: Leaving directory `/home/ramsay/git/t'
$ 

In the first case, t3421-*.sh, git bisect fingered commit f32ec670 ("git-rebase--merge: don't include absent parent as a base", 20-04-2016).

In the second case, t6036-*.sh, git bisect fingered commit b61f9d6e ("ll-merge: use a longer conflict marker for internal merge", 14-04-2016).

I won't have any time tonight to look into this any further (are these false positives?), so I thought I would just make sure you were aware of these 'unexpected passes'.

ATB, Ramsay Jones

Elijah Newren· Apr 22, 2016, 22:19 UTC · re: Ramsay Jones · lore

Re: make test Unexpected passes

On Fri, Apr 22, 2016 at 1:05 PM, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:

> Hi Ben, Junio,
>
> In the second case, t6036-*.sh, git bisect fingered commit b61f9d6e
> ("ll-merge: use a longer conflict marker for internal merge", 14-04-2016).

Yeah, the t6036 testcase 'git detects conflict w/ criss-cross+contrived resolution' could be made to pass by tweaking the conflict markers. In fact, any tweak would make it appear to pass, but the test could be updated to still fail by updating the contrived resolution of a previous merge to use the newer conflict marker style as well.

The test is kind of fragile in this way, and is really there just to document this slightly weird and never seen in practice corner case. I don't think we'll ever fix the underlying "problem"; not even sure if it's possible without fundamentally re-thinking our merge strategy altogether. I just thought that when I was writing all those tests that documenting this particular behavior as a testcase had some value, but if the conflict markers are going to be updated periodically, then the cost of the test may outweigh its value and we should just toss this one test from that file.

Elijah
Junio C Hamano· Apr 27, 2016, 22:05 UTC · re: Elijah Newren · lore

Re: make test Unexpected passes

Elijah Newren <newren@gmail.com> writes:
Show 6 quoted lines
> Yeah, the t6036 testcase 'git detects conflict w/
> criss-cross+contrived resolution' could be made to pass by tweaking
> the conflict markers.  In fact, any tweak would make it appear to
> pass, but the test could be updated to still fail by updating the
> contrived resolution of a previous merge to use the newer conflict
> marker style as well.

Isn't what the test expects bogus in the first place? I'd suggest removing the test as "pointless waste of resource".

Comments?
-- >8 --
Subject: [PATCH] t6036: remove pointless test that expects failure

One test in t6036 prepares a file whose contents contain these lines:

	<<<<<<< Temporary merge branch 1
	C
	=======
	B
	>>>>>>> Temporary merge branch 2
and uses recursive merge strategy to run criss-cross merge with it.

Manual merge resolution by users fundamentally depends on being able to tell what is the tracked contents and what is the separator lines added by "git merge" to tell users which block of lines came from where. You can deliberately craft a file with lines that resemble conflict marker lines to make it impossible for the user (here, the outer merge of merge-recursive also counts as a user of the internal merge) to tell which part is which, and write a test to demonstrate that with such a file that "git merge" fundamentally cannot work with and has to fail on. It however is pointless and waste of time and resource to run such a test that asserts the obvious.

In real life, people who do need to track files with such lines that have <<<< ==== >>>> as their prefixes set the conflict-marker-size attribute to make sure they will be able to tell between the tracked lines that happen to begin with these (confusing) prefixes and the marker lines that are added by "git merge".

Remove the test as pointless waste of resource.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t6036-recursive-corner-cases.sh | 83 ---------------------------------------
 1 file changed, 83 deletions(-)
diff --git a/t/t6036-recursive-corner-cases.sh b/t/t6036-recursive-corner-cases.sh
index cc1ee6a..c7b0ae6 100755
--- a/t/t6036-recursive-corner-cases.sh
+++ b/t/t6036-recursive-corner-cases.sh
@@ -304,89 +304,6 @@ test_expect_success 'git detects conflict merging criss-cross+modify/delete, rev
 	test $(git rev-parse :3:file) = $(git rev-parse B:file)
 '
 
-#
-# criss-cross + modify/modify with very contrived file contents:
-#
-#      B   D
-#      o---o
-#     / \ / \
-#  A o   X   ? F
-#     \ / \ /
-#      o---o
-#      C   E
-#
-#   Commit A: file with contents 'A\n'
-#   Commit B: file with contents 'B\n'
-#   Commit C: file with contents 'C\n'
-#   Commit D: file with contents 'D\n'
-#   Commit E: file with contents:
-#      <<<<<<< Temporary merge branch 1
-#      C
-#      =======
-#      B
-#      >>>>>>> Temporary merge branch 2
-#
-# Now, when we merge commits D & E, does git detect the conflict?
-
-test_expect_success 'setup differently handled merges of content conflict' '
-	git clean -fdqx &&
-	rm -rf .git &&
-	git init &&
-
-	echo A >file &&
-	git add file &&
-	test_tick &&
-	git commit -m A &&
-
-	git branch B &&
-	git checkout -b C &&
-	echo C >file &&
-	git add file &&
-	test_tick &&
-	git commit -m C &&
-
-	git checkout B &&
-	echo B >file &&
-	git add file &&
-	test_tick &&
-	git commit -m B &&
-
-	git checkout B^0 &&
-	test_must_fail git merge C &&
-	echo D >file &&
-	git add file &&
-	test_tick &&
-	git commit -m D &&
-	git tag D &&
-
-	git checkout C^0 &&
-	test_must_fail git merge B &&
-	cat <<EOF >file &&
-<<<<<<< Temporary merge branch 1
-C
-=======
-B
->>>>>>> Temporary merge branch 2
-EOF
-	git add file &&
-	test_tick &&
-	git commit -m E &&
-	git tag E
-'
-
-test_expect_failure 'git detects conflict w/ criss-cross+contrived resolution' '
-	git checkout D^0 &&
-
-	test_must_fail git merge -s recursive E^0 &&
-
-	test 3 -eq $(git ls-files -s | wc -l) &&
-	test 3 -eq $(git ls-files -u | wc -l) &&
-	test 0 -eq $(git ls-files -o | wc -l) &&
-
-	test $(git rev-parse :2:file) = $(git rev-parse D:file) &&
-	test $(git rev-parse :3:file) = $(git rev-parse E:file)
-'
-
 #
 # criss-cross + d/f conflict via add/add:
 #   Commit A: Neither file 'a' nor directory 'a/' exists.
Elijah Newren· Apr 27, 2016, 22:39 UTC · re: Junio C Hamano · lore

Re: make test Unexpected passes

On Wed, Apr 27, 2016 at 3:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
> Isn't what the test expects bogus in the first place?  I'd suggest
> removing the test as "pointless waste of resource".
>
> Comments?
>
> -- >8 --
Yes, toss it; I find your arguments below compelling.
Show 16 quoted lines
> Manual merge resolution by users fundamentally depends on being able
> to tell what is the tracked contents and what is the separator lines
> added by "git merge" to tell users which block of lines came from
> where.  You can deliberately craft a file with lines that resemble
> conflict marker lines to make it impossible for the user (here, the
> outer merge of merge-recursive also counts as a user of the internal
> merge) to tell which part is which, and write a test to demonstrate
> that with such a file that "git merge" fundamentally cannot work
> with and has to fail on.  It however is pointless and waste of time
> and resource to run such a test that asserts the obvious.
>
> In real life, people who do need to track files with such lines that
> have <<<< ==== >>>> as their prefixes set the conflict-marker-size
> attribute to make sure they will be able to tell between the tracked
> lines that happen to begin with these (confusing) prefixes and the
> marker lines that are added by "git merge".
Ben Woosley· Apr 22, 2016, 23:16 UTC · re: Ramsay Jones · lore

Re: make test Unexpected passes

Ramsay Jones <ramsay <at> ramsayjones.plus.com> writes:
Show 11 quoted lines
> 
> Hi Ben, Junio,
> 
> Tonight, the testsuite passed with a couple of 'unexpected passes', viz:
>
> In the first case, t3421-*.sh, git bisect fingered commit f32ec670
> ("git-rebase--merge: don't include absent parent as a base", 20-04-2016).
>
> ATB,
> Ramsay Jones
> 
 
Yep,
These know breakages:

ok 50 - rebase -m --onto --root ok 54 - rebase -m without --onto --root with disjoint history

Have to do with rebasing a root/orphan branch with the -m flag, which defaults to -- merge=recursive, which is the case the patch fixed.

Here are the necessary changes:
--- a/t/t3421-rebase-topology-linear.sh
+++ b/t/t3421-rebase-topology-linear.sh
@@ -253,7 +253,7 @@ test_run_rebase () {
 	"
 }
 test_run_rebase success ''
-test_run_rebase failure -m
+test_run_rebase success -m
 test_run_rebase success -i
 test_run_rebase success -p
 
@@ -268,7 +268,7 @@ test_run_rebase () {
 	"
 }
 test_run_rebase success ''
-test_run_rebase failure -m
+test_run_rebase success -m
 test_run_rebase success -i
 test_run_rebase failure -p
 
-- 
2.8.1.211.g8e54d77
Junio C Hamano· Apr 24, 2016, 19:09 UTC · re: Ben Woosley · lore

Re: make test Unexpected passes

Ben Woosley <ben.woosley@gmail.com> writes:
Show 10 quoted lines
> These know breakages:
>
> ok 50 - rebase -m --onto --root
> ok 54 - rebase -m without --onto --root with disjoint history
>
> Have to do with rebasing a root/orphan branch with the -m flag,
> which defaults to -- merge=recursive, which is the case the patch fixed.
>
> Here are the necessary changes:
> ...
Thanks, will squash them in and reword the log message accordingly.
    git-rebase--merge: don't include absent parent as a base
    Absent this fix, attempts to rebase an orphan branch using "rebase -m"
    fails with:
        $ git rebase -m ORPHAN_TARGET_BASE
        First, rewinding head to replay your work on top of it...
        ...
    Note the default rebase behavior does not fail:
        $ git rebase ORPHAN_TARGET_BASE
        First, rewinding head to replay your work on top of it...
        Applying: ORPHAN_ROOT_COMMIT_MSG
        Using index info to reconstruct a base tree...
    A few tests were expecting the old behaviour to forbid rebasing such
    a history with "rebase -m", which now need to expect them to succeed.
    Signed-off-by: Ben Woosley <ben.woosley@gmail.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

← back to recent threads