diff mbox series

[1/7] test: add merge style config test

Message ID 20210609192842.696646-2-felipe.contreras@gmail.com (mailing list archive)
State New
Headers show
Series Make diff3 the default conflict style | expand

Commit Message

Felipe Contreras June 9, 2021, 7:28 p.m. UTC
We want to test different combinations of merge.conflictstyle, and a new
file is the best place to do that.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t6440-config-conflict-markers.sh | 44 ++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100755 t/t6440-config-conflict-markers.sh

Comments

Eric Sunshine June 9, 2021, 7:42 p.m. UTC | #1
On Wed, Jun 9, 2021 at 3:29 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> We want to test different combinations of merge.conflictstyle, and a new
> file is the best place to do that.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
> diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh
> @@ -0,0 +1,44 @@
> +fill () {
> +       for i
> +       do
> +               echo "$i"
> +       done
> +}

This seems to duplicate the behavior of test_write_lines()...

> +test_expect_success 'merge' '
> +       test_create_repo merge &&
> +       (
> +               cd merge &&
> +               fill 1 2 3 >content &&

... which could be used here instead:

    test_write_lines 1 2 3 >content &&
Felipe Contreras June 9, 2021, 8:29 p.m. UTC | #2
Eric Sunshine wrote:
> On Wed, Jun 9, 2021 at 3:29 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> > We want to test different combinations of merge.conflictstyle, and a new
> > file is the best place to do that.
> >
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> > diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh
> > @@ -0,0 +1,44 @@
> > +fill () {
> > +       for i
> > +       do
> > +               echo "$i"
> > +       done
> > +}
> 
> This seems to duplicate the behavior of test_write_lines()...

Right, I'll update the patch.

The above function is used in:

  t6440-config-conflict-markers.sh
  t7201-co.sh

So those two probably should be updated.

Cheers.
Phillip Wood June 10, 2021, 9:18 a.m. UTC | #3
On 09/06/2021 20:28, Felipe Contreras wrote:
> We want to test different combinations of merge.conflictstyle, and a new
> file is the best place to do that.

I'm not sure what this particular tests adds over the existing ones in 
t6427-diff3-conflict-markers.sh. The commit message does not explain why 
a new file is better than adding this test to that file. There are 
already diff3 tests for checkout so it ends up being confusing when a 
checkout test gets added to this file rather than with those tests later 
in the series because there is no longer a single location for diff3 
checkout tests.

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>   t/t6440-config-conflict-markers.sh | 44 ++++++++++++++++++++++++++++++
>   1 file changed, 44 insertions(+)
>   create mode 100755 t/t6440-config-conflict-markers.sh
> 
> diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh
> new file mode 100755
> index 0000000000..6952552c58
> --- /dev/null
> +++ b/t/t6440-config-conflict-markers.sh
> @@ -0,0 +1,44 @@
> +#!/bin/sh
> +
> +test_description='merge style conflict markers configurations'
> +
> +. ./test-lib.sh
> +
> +fill () {
> +	for i
> +	do
> +		echo "$i"
> +	done
> +}
> +
> +test_expect_success 'merge' '
> +	test_create_repo merge &&
> +	(
> +		cd merge &&
> +
> +		fill 1 2 3 >content &&
> +		git add content &&
> +		git commit -m base &&
> +
> +		git checkout -b r &&
> +		echo six >>content &&
> +		git commit -a -m right &&
> +
> +		git checkout master &&
> +		echo 7 >>content &&
> +		git commit -a -m left &&
> +
> +		test_must_fail git merge r &&
> +		! grep -E "\|+" content &&

! grep "|"  would be simpler and just as effective. This is quite a weak 
test, something like "^|||||| " would be a stronger test for conflict 
markers

Best Wishes

Phillip

> +		git reset --hard &&
> +		test_must_fail git -c merge.conflictstyle=diff3 merge r &&
> +		grep -E "\|+" content &&
> +
> +		git reset --hard &&
> +		test_must_fail git -c merge.conflictstyle=merge merge r &&
> +		! grep -E "\|+" content
> +	)
> +'
> +
> +test_done
>
Felipe Contreras June 10, 2021, 1:26 p.m. UTC | #4
Phillip Wood wrote:
> On 09/06/2021 20:28, Felipe Contreras wrote:
> > We want to test different combinations of merge.conflictstyle, and a new
> > file is the best place to do that.
> 
> I'm not sure what this particular tests adds over the existing ones in 
> t6427-diff3-conflict-markers.sh.

That file is for diff3 conflict markers. The tests in this file are not.

> The commit message does not explain why a new file is better than
> adding this test to that file.

Because there's no file that is testing for this.

> There are already diff3 tests for checkout

This file is not doing diff3 tests.


As stated above, it's testing different *combinations* of
merge.conflictstyle, diff3 is only *one* of the possibilities, another
possibility is:

  git -c merge.conflictstyle=diff3 checkout -m --conflict=merge

That is *not* a diff3 test.

> > diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh
> > new file mode 100755

> > +test_expect_success 'merge' '
> > +	test_create_repo merge &&
> > +	(
> > +		cd merge &&
> > +
> > +		fill 1 2 3 >content &&
> > +		git add content &&
> > +		git commit -m base &&
> > +
> > +		git checkout -b r &&
> > +		echo six >>content &&
> > +		git commit -a -m right &&
> > +
> > +		git checkout master &&
> > +		echo 7 >>content &&
> > +		git commit -a -m left &&
> > +
> > +		test_must_fail git merge r &&
> > +		! grep -E "\|+" content &&
> 
> ! grep "|"  would be simpler and just as effective.

But that would fail if there's a "command1 | command2".

> This is quite a weak 
> test, something like "^|||||| " would be a stronger test for conflict 
> markers

But that doesn't work in all the tests.

Cheers.
Phillip Wood June 10, 2021, 2:54 p.m. UTC | #5
On 10/06/2021 14:26, Felipe Contreras wrote:
> Phillip Wood wrote:
>> On 09/06/2021 20:28, Felipe Contreras wrote:
>>> We want to test different combinations of merge.conflictstyle, and a new
>>> file is the best place to do that.
>>
>> I'm not sure what this particular tests adds over the existing ones in
>> t6427-diff3-conflict-markers.sh.
> 
> That file is for diff3 conflict markers. The tests in this file are not.
> 
>> The commit message does not explain why a new file is better than
>> adding this test to that file.
> 
> Because there's no file that is testing for this.
> 
>> There are already diff3 tests for checkout
> 
> This file is not doing diff3 tests.
> 
> 
> As stated above, it's testing different *combinations* of
> merge.conflictstyle, diff3 is only *one* of the possibilities, another
> possibility is:
> 
>    git -c merge.conflictstyle=diff3 checkout -m --conflict=merge
> 
> That is *not* a diff3 test.

I think that is an artificial distinction, it is testing the behavior of 
checkout when merge.conflictStyle=diff3 just like the other tests, it 
just happens to be checking that the config option can be combined with 
a command line option.

Best Wishes

Phillip

>>> diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh
>>> new file mode 100755
> 
>>> +test_expect_success 'merge' '
>>> +	test_create_repo merge &&
>>> +	(
>>> +		cd merge &&
>>> +
>>> +		fill 1 2 3 >content &&
>>> +		git add content &&
>>> +		git commit -m base &&
>>> +
>>> +		git checkout -b r &&
>>> +		echo six >>content &&
>>> +		git commit -a -m right &&
>>> +
>>> +		git checkout master &&
>>> +		echo 7 >>content &&
>>> +		git commit -a -m left &&
>>> +
>>> +		test_must_fail git merge r &&
>>> +		! grep -E "\|+" content &&
>>
>> ! grep "|"  would be simpler and just as effective.
> 
> But that would fail if there's a "command1 | command2".
> 
>> This is quite a weak
>> test, something like "^|||||| " would be a stronger test for conflict
>> markers
> 
> But that doesn't work in all the tests.
> 
> Cheers.
>
Phillip Wood June 10, 2021, 2:58 p.m. UTC | #6
On 10/06/2021 14:26, Felipe Contreras wrote:
> Phillip Wood wrote:
>> On 09/06/2021 20:28, Felipe Contreras wrote:
>>> We want to test different combinations of merge.conflictstyle, and a new
>>> file is the best place to do that.
>>[...]
>>> diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh
>>> new file mode 100755
> 
>>> +test_expect_success 'merge' '
>>> +	test_create_repo merge &&
>>> +	(
>>> +		cd merge &&
>>> +
>>> +		fill 1 2 3 >content &&
>>> +		git add content &&
>>> +		git commit -m base &&
>>> +
>>> +		git checkout -b r &&
>>> +		echo six >>content &&
>>> +		git commit -a -m right &&
>>> +
>>> +		git checkout master &&
>>> +		echo 7 >>content &&
>>> +		git commit -a -m left &&
>>> +
>>> +		test_must_fail git merge r &&
>>> +		! grep -E "\|+" content &&
>>
>> ! grep "|"  would be simpler and just as effective.
> 
> But that would fail if there's a "command1 | command2".

I don't understand. What are you expecting content to contain? Why 
doesn't "\|+" fail in that case?

>> This is quite a weak
>> test, something like "^|||||| " would be a stronger test for conflict
>> markers
> 
> But that doesn't work in all the tests.

So test for what you actually expect, you don't need to have the same 
check in all the tests if the expected output is different.

Best Wishes

Phillip

> Cheers.
>
Felipe Contreras June 10, 2021, 4:34 p.m. UTC | #7
Phillip Wood wrote:
> On 10/06/2021 14:26, Felipe Contreras wrote:
> > Phillip Wood wrote:
> >> There are already diff3 tests for checkout
> > 
> > This file is not doing diff3 tests.
> > 
> > 
> > As stated above, it's testing different *combinations* of
> > merge.conflictstyle, diff3 is only *one* of the possibilities, another
> > possibility is:
> > 
> >    git -c merge.conflictstyle=diff3 checkout -m --conflict=merge
> > 
> > That is *not* a diff3 test.
> 
> I think that is an artificial distinction, it is testing the behavior of 
> checkout when merge.conflictStyle=diff3

That is one of the things it's testing, it's not the only thing it's
testing, it's testing other things as well.
Felipe Contreras June 10, 2021, 4:47 p.m. UTC | #8
Phillip Wood wrote:
> On 10/06/2021 14:26, Felipe Contreras wrote:
> > Phillip Wood wrote:
> >> On 09/06/2021 20:28, Felipe Contreras wrote:
> >>> We want to test different combinations of merge.conflictstyle, and a new
> >>> file is the best place to do that.
> >>[...]
> >>> diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh
> >>> new file mode 100755
> > 
> >>> +test_expect_success 'merge' '
> >>> +	test_create_repo merge &&
> >>> +	(
> >>> +		cd merge &&
> >>> +
> >>> +		fill 1 2 3 >content &&
> >>> +		git add content &&
> >>> +		git commit -m base &&
> >>> +
> >>> +		git checkout -b r &&
> >>> +		echo six >>content &&
> >>> +		git commit -a -m right &&
> >>> +
> >>> +		git checkout master &&
> >>> +		echo 7 >>content &&
> >>> +		git commit -a -m left &&
> >>> +
> >>> +		test_must_fail git merge r &&
> >>> +		! grep -E "\|+" content &&
> >>
> >> ! grep "|"  would be simpler and just as effective.
> > 
> > But that would fail if there's a "command1 | command2".
> 
> I don't understand. What are you expecting content to contain?

Not a sequence of |.

> Why doesn't "\|+" fail in that case?

It would, perhaps "\|\|+" would be better, or maybe "\|{2,}".

> >> This is quite a weak
> >> test, something like "^|||||| " would be a stronger test for conflict
> >> markers
> > 
> > But that doesn't work in all the tests.
> 
> So test for what you actually expect, you don't need to have the same 
> check in all the tests if the expected output is different.

I don't need to, but it makes the tests simpler, and as you pointed out
in another comment: more tests are needed.

Perhaps once we know exactly what we want to test, and how to fix the
current issues it would make sense to revisit these.
Phillip Wood June 11, 2021, 9:19 a.m. UTC | #9
On 10/06/2021 17:47, Felipe Contreras wrote:
> Phillip Wood wrote:
>> On 10/06/2021 14:26, Felipe Contreras wrote:
>>> Phillip Wood wrote:
>>>> On 09/06/2021 20:28, Felipe Contreras wrote:
>>>>> We want to test different combinations of merge.conflictstyle, and a new
>>>>> file is the best place to do that.
>>>> [...]
>>>>> diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh
>>>>> new file mode 100755
>>>
>>>>> +test_expect_success 'merge' '
>>>>> +	test_create_repo merge &&
>>>>> +	(
>>>>> +		cd merge &&
>>>>> +
>>>>> +		fill 1 2 3 >content &&
>>>>> +		git add content &&
>>>>> +		git commit -m base &&
>>>>> +
>>>>> +		git checkout -b r &&
>>>>> +		echo six >>content &&
>>>>> +		git commit -a -m right &&
>>>>> +
>>>>> +		git checkout master &&
>>>>> +		echo 7 >>content &&
>>>>> +		git commit -a -m left &&
>>>>> +
>>>>> +		test_must_fail git merge r &&
>>>>> +		! grep -E "\|+" content &&
>>>>
>>>> ! grep "|"  would be simpler and just as effective.
>>>
>>> But that would fail if there's a "command1 | command2".
>>
>> I don't understand. What are you expecting content to contain?
> 
> Not a sequence of |.
> 
>> Why doesn't "\|+" fail in that case?
> 
> It would, perhaps "\|\|+" would be better, or maybe "\|{2,}".

The point of my original comment was that you do not need an ERE - 'grep 
"||"' matches the same set of lines as 'grep -E "\|\|+"'. As it is 
testing for conflict markers anchoring the pattern to the beginning of a 
line would probably be a good idea.

Best Wishes

Phillip

>>>> This is quite a weak
>>>> test, something like "^|||||| " would be a stronger test for conflict
>>>> markers
>>>
>>> But that doesn't work in all the tests.
>>
>> So test for what you actually expect, you don't need to have the same
>> check in all the tests if the expected output is different.
> 
> I don't need to, but it makes the tests simpler, and as you pointed out
> in another comment: more tests are needed.
> 
> Perhaps once we know exactly what we want to test, and how to fix the
> current issues it would make sense to revisit these.
>
Felipe Contreras June 11, 2021, 2:39 p.m. UTC | #10
Phillip Wood wrote:
> On 10/06/2021 17:47, Felipe Contreras wrote:
> > Phillip Wood wrote:
> >> On 10/06/2021 14:26, Felipe Contreras wrote:
> >>> Phillip Wood wrote:
> >>>> On 09/06/2021 20:28, Felipe Contreras wrote:
> >>>>> We want to test different combinations of merge.conflictstyle, and a new
> >>>>> file is the best place to do that.
> >>>> [...]
> >>>>> diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh
> >>>>> new file mode 100755
> >>>
> >>>>> +test_expect_success 'merge' '
> >>>>> +	test_create_repo merge &&
> >>>>> +	(
> >>>>> +		cd merge &&
> >>>>> +
> >>>>> +		fill 1 2 3 >content &&
> >>>>> +		git add content &&
> >>>>> +		git commit -m base &&
> >>>>> +
> >>>>> +		git checkout -b r &&
> >>>>> +		echo six >>content &&
> >>>>> +		git commit -a -m right &&
> >>>>> +
> >>>>> +		git checkout master &&
> >>>>> +		echo 7 >>content &&
> >>>>> +		git commit -a -m left &&
> >>>>> +
> >>>>> +		test_must_fail git merge r &&
> >>>>> +		! grep -E "\|+" content &&
> >>>>
> >>>> ! grep "|"  would be simpler and just as effective.
> >>>
> >>> But that would fail if there's a "command1 | command2".
> >>
> >> I don't understand. What are you expecting content to contain?
> > 
> > Not a sequence of |.
> > 
> >> Why doesn't "\|+" fail in that case?
> > 
> > It would, perhaps "\|\|+" would be better, or maybe "\|{2,}".
> 
> The point of my original comment was that you do not need an ERE - 'grep 
> "||"' matches the same set of lines as 'grep -E "\|\|+"'. As it is 
> testing for conflict markers anchoring the pattern to the beginning of a 
> line would probably be a good idea.

Right, I didn't add that because I saw some tests not doing ^ when they
clearly should, so I thought perhaps there was a compatibility issue,
but now I see that ^ is already used in many tests, therefore "^\|\|+"
makes sense.
diff mbox series

Patch

diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh
new file mode 100755
index 0000000000..6952552c58
--- /dev/null
+++ b/t/t6440-config-conflict-markers.sh
@@ -0,0 +1,44 @@ 
+#!/bin/sh
+
+test_description='merge style conflict markers configurations'
+
+. ./test-lib.sh
+
+fill () {
+	for i
+	do
+		echo "$i"
+	done
+}
+
+test_expect_success 'merge' '
+	test_create_repo merge &&
+	(
+		cd merge &&
+
+		fill 1 2 3 >content &&
+		git add content &&
+		git commit -m base &&
+
+		git checkout -b r &&
+		echo six >>content &&
+		git commit -a -m right &&
+
+		git checkout master &&
+		echo 7 >>content &&
+		git commit -a -m left &&
+
+		test_must_fail git merge r &&
+		! grep -E "\|+" content &&
+
+		git reset --hard &&
+		test_must_fail git -c merge.conflictstyle=diff3 merge r &&
+		grep -E "\|+" content &&
+
+		git reset --hard &&
+		test_must_fail git -c merge.conflictstyle=merge merge r &&
+		! grep -E "\|+" content
+	)
+'
+
+test_done