[1/5] t3416: set $EDITOR in subshell

Commit Message

Phillip Wood Aug. 15, 2022, 3:11 p.m. UTC

From: Phillip Wood <[e-mail]>

As $EDITOR is exported setting it in one test affects all subsequent
tests. Avoid this by always setting it in a subshell and remove a
couple of unnecessary call to set_fake_editor.

Signed-off-by: Phillip Wood <[e-mail]>
---
 t/t3416-rebase-onto-threedots.sh | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

Comments

Junio C Hamano Aug. 15, 2022, 4:53 p.m. UTC | #1

"Phillip Wood via GitGitGadget" <[e-mail]> writes:

> From: Phillip Wood <[e-mail]>
>
> As $EDITOR is exported setting it in one test affects all subsequent
> tests. Avoid this by always setting it in a subshell and remove a
> couple of unnecessary call to set_fake_editor.

Unnecessary because it reuses the one that was established in the
previous test [1]?  Or unnecessary because we know "rebase -i" would
fail even before it gets to the point of asking an editor to tweak
the todo sequence [2]?  Or something else?

If [1], it makes us wonder what happens when an earlier test gets
skipped.  If [2], it makes us wonder what happens when "rebase -i"
fails to fail as expected (does the test correctly diagnose it as a
new breakage in "rebase -i"?).

> @@ -102,7 +106,6 @@ test_expect_success 'rebase -i --onto main...side' '
>  	git checkout side &&
>  	git reset --hard K &&
>  
> -	set_fake_editor &&
>  	test_must_fail git rebase -i --onto main...side J
>  '

This is one of the "removing" instances.

> @@ -187,8 +194,12 @@ test_expect_success 'rebase -i --keep-base main from side' '
>  	git checkout side &&
>  	git reset --hard K &&
>  
> -	set_fake_editor &&
>  	test_must_fail git rebase -i --keep-base main
>  '

And this is the other one.

Thanks.

Phillip Wood Aug. 16, 2022, 1:53 p.m. UTC | #2

Hi Junio

On 15/08/2022 17:53, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <[e-mail]> writes:
> 
>> From: Phillip Wood <[e-mail]>
>>
>> As $EDITOR is exported setting it in one test affects all subsequent
>> tests. Avoid this by always setting it in a subshell and remove a
>> couple of unnecessary call to set_fake_editor.
> 
> Unnecessary because it reuses the one that was established in the
> previous test [1]?  Or unnecessary because we know "rebase -i" would
> fail even before it gets to the point of asking an editor to tweak
> the todo sequence [2]?  Or something else?

I meant unnecessary as the editor does not change the todo list, but [2] 
also applies.

> If [1], it makes us wonder what happens when an earlier test gets
> skipped.  If [2], it makes us wonder what happens when "rebase -i"
> fails to fail as expected (does the test correctly diagnose it as a
> new breakage in "rebase -i"?).

I think those tests could be tightened up, I'll add a new patch that 
renames them to describe what they are testing (that we fail if there is 
more than one merge base) and greps for the expected error message.

Best Wishes

Phillip

>> @@ -102,7 +106,6 @@ test_expect_success 'rebase -i --onto main...side' '
>>   	git checkout side &&
>>   	git reset --hard K &&
>>   
>> -	set_fake_editor &&
>>   	test_must_fail git rebase -i --onto main...side J
>>   '
> 
> This is one of the "removing" instances.
> 
>> @@ -187,8 +194,12 @@ test_expect_success 'rebase -i --keep-base main from side' '
>>   	git checkout side &&
>>   	git reset --hard K &&
>>   
>> -	set_fake_editor &&
>>   	test_must_fail git rebase -i --keep-base main
>>   '
> 
> And this is the other one.
> 
> Thanks.

Jonathan Tan Aug. 24, 2022, 10:28 p.m. UTC | #3

Phillip Wood <[e-mail]> writes:
> Hi Junio
> 
> On 15/08/2022 17:53, Junio C Hamano wrote:
> > "Phillip Wood via GitGitGadget" <[e-mail]> writes:
> > 
> >> From: Phillip Wood <[e-mail]>
> >>
> >> As $EDITOR is exported setting it in one test affects all subsequent
> >> tests. Avoid this by always setting it in a subshell and remove a
> >> couple of unnecessary call to set_fake_editor.
> > 
> > Unnecessary because it reuses the one that was established in the
> > previous test [1]?  Or unnecessary because we know "rebase -i" would
> > fail even before it gets to the point of asking an editor to tweak
> > the todo sequence [2]?  Or something else?
> 
> I meant unnecessary as the editor does not change the todo list, but [2] 
> also applies.

Maybe this is moot with the other changes you're planning, but even if
the editor doesn't change the todo list, it's still necessary, right? At
the very least, we need to suppress the default interactive editor and
replace it with one that just reuses the input file without any
modification.

Phillip Wood Aug. 30, 2022, 3:12 p.m. UTC | #4

Hi Jonathan

On 24/08/2022 23:28, Jonathan Tan wrote:
> Phillip Wood <[e-mail]> writes:
>> Hi Junio
>>
>> On 15/08/2022 17:53, Junio C Hamano wrote:
>>> "Phillip Wood via GitGitGadget" <[e-mail]> writes:
>>>
>>>> From: Phillip Wood <[e-mail]>
>>>>
>>>> As $EDITOR is exported setting it in one test affects all subsequent
>>>> tests. Avoid this by always setting it in a subshell and remove a
>>>> couple of unnecessary call to set_fake_editor.
>>>
>>> Unnecessary because it reuses the one that was established in the
>>> previous test [1]?  Or unnecessary because we know "rebase -i" would
>>> fail even before it gets to the point of asking an editor to tweak
>>> the todo sequence [2]?  Or something else?
>>
>> I meant unnecessary as the editor does not change the todo list, but [2]
>> also applies.
> 
> Maybe this is moot with the other changes you're planning, but even if
> the editor doesn't change the todo list, it's still necessary, right? At
> the very least, we need to suppress the default interactive editor and
> replace it with one that just reuses the input file without any
> modification.

The default GIT_EDITOR when running the test suite is ":" and 
GIT_SEQUENCE_EDITOR and sequence.editor are unset so we don't need to 
set an editor in the tests unless we want to change the todo list.

Best Wishes

Phillip

Patch

diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
index 3e04802cb00..e1fc2dbd48e 100755
--- a/t/t3416-rebase-onto-threedots.sh
+++ b/t/t3416-rebase-onto-threedots.sh
@@ -79,8 +79,10 @@  test_expect_success 'rebase -i --onto main...topic' '
 	git reset --hard &&
 	git checkout topic &&
 	git reset --hard G &&
-	set_fake_editor &&
-	EXPECT_COUNT=1 git rebase -i --onto main...topic F &&
+	(
+		set_fake_editor &&
+		EXPECT_COUNT=1 git rebase -i --onto main...topic F
+	) &&
 	git rev-parse HEAD^1 >actual &&
 	git rev-parse C^0 >expect &&
 	test_cmp expect actual
@@ -90,8 +92,10 @@  test_expect_success 'rebase -i --onto main...' '
 	git reset --hard &&
 	git checkout topic &&
 	git reset --hard G &&
-	set_fake_editor &&
-	EXPECT_COUNT=1 git rebase -i --onto main... F &&
+	(
+		set_fake_editor &&
+		EXPECT_COUNT=1 git rebase -i --onto main... F
+	) &&
 	git rev-parse HEAD^1 >actual &&
 	git rev-parse C^0 >expect &&
 	test_cmp expect actual
@@ -102,7 +106,6 @@  test_expect_success 'rebase -i --onto main...side' '
 	git checkout side &&
 	git reset --hard K &&
 
-	set_fake_editor &&
 	test_must_fail git rebase -i --onto main...side J
 '
 
@@ -156,8 +159,10 @@  test_expect_success 'rebase -i --keep-base main from topic' '
 	git checkout topic &&
 	git reset --hard G &&
 
-	set_fake_editor &&
-	EXPECT_COUNT=2 git rebase -i --keep-base main &&
+	(
+		set_fake_editor &&
+		EXPECT_COUNT=2 git rebase -i --keep-base main
+	) &&
 	git rev-parse C >base.expect &&
 	git merge-base main HEAD >base.actual &&
 	test_cmp base.expect base.actual &&
@@ -171,8 +176,10 @@  test_expect_success 'rebase -i --keep-base main topic from main' '
 	git checkout main &&
 	git branch -f topic G &&
 
-	set_fake_editor &&
-	EXPECT_COUNT=2 git rebase -i --keep-base main topic &&
+	(
+		set_fake_editor &&
+		EXPECT_COUNT=2 git rebase -i --keep-base main topic
+	) &&
 	git rev-parse C >base.expect &&
 	git merge-base main HEAD >base.actual &&
 	test_cmp base.expect base.actual &&
@@ -187,8 +194,12 @@  test_expect_success 'rebase -i --keep-base main from side' '
 	git checkout side &&
 	git reset --hard K &&
 
-	set_fake_editor &&
 	test_must_fail git rebase -i --keep-base main
 '
 
+# This must be the last test in this file
+test_expect_success '$EDITOR and friends are unchanged' '
+	test_editor_unchanged
+'
+
 test_done