threads / patch / 34060

patchtests: fix autostash

Subject: [PATCH] tests: fix autostash

## tl;dr

10 messages between Jun 7, 2013 and Jun 8, 2013. Diffs are folded; open one to read it.

replies: 9people: 4as markdown or json

Felipe Contreras· Jun 7, 2013, 23:45 UTC · lore
We should call 'git rebase --abort', like a normal user would do.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t3420-rebase-autostash.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
Show changes to t/t3420-rebase-autostash.sh +1 −2
diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index a5e69f3..ff370a3 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -71,8 +71,7 @@ testrebase() {
 		test_must_fail git rebase$type related-onto-branch &&
 		test_path_is_file $dotest/autostash &&
 		! grep dirty file3 &&
-		rm -rf $dotest &&
-		git reset --hard &&
+		git rebase --abort &&
 		git checkout feature-branch
 	'
 

-- 
1.8.3.698.g079b096
Ramkumar Ramachandra· Jun 8, 2013, 03:29 UTC · re: Felipe Contreras · lore

Re: [PATCH] tests: fix autostash

Felipe Contreras wrote:
Show 13 quoted lines
> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
> index a5e69f3..ff370a3 100755
> --- a/t/t3420-rebase-autostash.sh
> +++ b/t/t3420-rebase-autostash.sh
> @@ -71,8 +71,7 @@ testrebase() {
>                 test_must_fail git rebase$type related-onto-branch &&
>                 test_path_is_file $dotest/autostash &&
>                 ! grep dirty file3 &&
> -               rm -rf $dotest &&
> -               git reset --hard &&
> +               git rebase --abort &&
>                 git checkout feature-branch
>         '

Incorrect. I don't assume that --abort works yet, in this test. It is non-trivial to get --abort to work with autostash, and this is tested in a separate test: see "rebase$type: --abort" in the same file.

Felipe Contreras· Jun 8, 2013, 10:24 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH] tests: fix autostash

On Fri, Jun 7, 2013 at 10:29 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

Show 16 quoted lines
> Felipe Contreras wrote:
>> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
>> index a5e69f3..ff370a3 100755
>> --- a/t/t3420-rebase-autostash.sh
>> +++ b/t/t3420-rebase-autostash.sh
>> @@ -71,8 +71,7 @@ testrebase() {
>>                 test_must_fail git rebase$type related-onto-branch &&
>>                 test_path_is_file $dotest/autostash &&
>>                 ! grep dirty file3 &&
>> -               rm -rf $dotest &&
>> -               git reset --hard &&
>> +               git rebase --abort &&
>>                 git checkout feature-branch
>>         '
>
> Incorrect.  I don't assume that --abort works yet, in this test.

Yes you do. The rest of the tests expect that the previous rebase has been aborted.

In fact, all the tests depend on the previous test finishing correctly, which is not the way tests should be written.

Show changes to t/t3420-rebase-autostash.sh +1 −1
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -70,7 +70,7 @@ testrebase() {
                echo dirty >>file3 &&
                test_must_fail git rebase$type related-onto-branch &&
                test_path_is_file $dotest/autostash &&
-               ! grep dirty file3 &&
+               false ! grep dirty file3 &&
                rm -rf $dotest &&
                git reset --hard &&
                git checkout feature-branch

# failed 19 among 22 test(s)

Doing 'rm -rf $dotest' is even worst than 'git rebase --abort',
because it relies on the implementation of 'git rebase', which might
need to remove more files than $dotest.

This wouldn't be a problem if the tests were implemented correctly,
but they are not, so 'git rebase --abort' is the only sane option.

-- 
Felipe Contreras
Ramkumar Ramachandra· Jun 8, 2013, 13:19 UTC · re: Felipe Contreras · lore

Re: [PATCH] tests: fix autostash

Felipe Contreras wrote:
Show 5 quoted lines
> Yes you do. The rest of the tests expect that the previous rebase has
> been aborted.
>
> In fact, all the tests depend on the previous test finishing
> correctly, which is not the way tests should be written.

How else am I supposed to write them? If there is a stale state from the previous test, there isn't too much I can do. Or should I be cleaning up state at the beginning of each test, instead of at the end?

> Doing 'rm -rf $dotest' is even worst than 'git rebase --abort',
> because it relies on the implementation of 'git rebase', which might
> need to remove more files than $dotest.
Huh?  Tests aren't allowed to rely on how a command is implemented?
  $ git grep test_path t

Ofcourse they're implementation details. Even in this very test, I check $dotest/autostash plenty of times.

Have you read rr/rebase-autostash? The whole idea is to inject $dotest/autostash and teach various scripts about how their assumptions about $dotest have changed.

> This wouldn't be a problem if the tests were implemented correctly,
> but they are not, so 'git rebase --abort' is the only sane option.
Then show me how to do it correctly.
Felipe Contreras· Jun 8, 2013, 14:01 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH] tests: fix autostash

On Sat, Jun 8, 2013 at 8:19 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
Show 10 quoted lines
>> Doing 'rm -rf $dotest' is even worst than 'git rebase --abort',
>> because it relies on the implementation of 'git rebase', which might
>> need to remove more files than $dotest.
>
> Huh?  Tests aren't allowed to rely on how a command is implemented?
>
>   $ git grep test_path t
>
> Ofcourse they're implementation details.  Even in this very test, I
> check $dotest/autostash plenty of times.
The more the test relies on implementation details, the worst.
Show 8 quoted lines
> Have you read rr/rebase-autostash?  The whole idea is to inject
> $dotest/autostash and teach various scripts about how their
> assumptions about $dotest have changed.
>
>> This wouldn't be a problem if the tests were implemented correctly,
>> but they are not, so 'git rebase --abort' is the only sane option.
>
> Then show me how to do it correctly.
Something like this.
Show changes to t/t3420-rebase-autostash.sh +42 −41
diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index 6ba449b..b96a56a 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -33,54 +33,56 @@ test_expect_success setup '
 	git commit -m "related commit"
 '

+setup_tmp () {
+	git clone . tmp &&
+	cd tmp &&
+	git fetch -u origin "refs/heads/*:refs/heads/*" &&
+	test_config rebase.autostash true &&
+	git checkout -b rebased-feature-branch feature-branch
+}
+
 testrebase() {
 	type=$1
 	dotest=$2

 	test_expect_success "rebase$type: dirty worktree, non-conflicting rebase" '
-		test_config rebase.autostash true &&
-		git reset --hard &&
-		git checkout -b rebased-feature-branch feature-branch &&
-		test_when_finished git branch -D rebased-feature-branch &&
+		test_when_finished "rm -rf tmp" &&
+		(
+		setup_tmp &&
 		echo dirty >>file3 &&
 		git rebase$type unrelated-onto-branch &&
 		grep unrelated file4 &&
-		grep dirty file3 &&
-		git checkout feature-branch
+		grep dirty file3
+		)
 	'

 	test_expect_success "rebase$type: dirty index, non-conflicting rebase" '
-		test_config rebase.autostash true &&
-		git reset --hard &&
-		git checkout -b rebased-feature-branch feature-branch &&
-		test_when_finished git branch -D rebased-feature-branch &&
+		test_when_finished "rm -rf tmp" &&
+		(
+		setup_tmp &&
 		echo dirty >>file3 &&
 		git add file3 &&
 		git rebase$type unrelated-onto-branch &&
 		grep unrelated file4 &&
-		grep dirty file3 &&
-		git checkout feature-branch
+		grep dirty file3
+		)
 	'

 	test_expect_success "rebase$type: conflicting rebase" '
-		test_config rebase.autostash true &&
-		git reset --hard &&
-		git checkout -b rebased-feature-branch feature-branch &&
-		test_when_finished git branch -D rebased-feature-branch &&
+		test_when_finished "rm -rf tmp" &&
+		(
+		setup_tmp &&
 		echo dirty >>file3 &&
 		test_must_fail git rebase$type related-onto-branch &&
 		test_path_is_file $dotest/autostash &&
-		false ! grep dirty file3 &&
-		rm -rf $dotest &&
-		git reset --hard &&
-		git checkout feature-branch
+		! grep dirty file3
+		)
 	'

 	test_expect_success "rebase$type: --continue" '
-		test_config rebase.autostash true &&
-		git reset --hard &&
-		git checkout -b rebased-feature-branch feature-branch &&
-		test_when_finished git branch -D rebased-feature-branch &&
+		test_when_finished "rm -rf tmp" &&
+		(
+		setup_tmp &&
 		echo dirty >>file3 &&
 		test_must_fail git rebase$type related-onto-branch &&
 		test_path_is_file $dotest/autostash &&
@@ -89,45 +91,43 @@ testrebase() {
 		git add file2 &&
 		git rebase --continue &&
 		test_path_is_missing $dotest/autostash &&
-		grep dirty file3 &&
-		git checkout feature-branch
+		grep dirty file3
+		)
 	'

 	test_expect_success "rebase$type: --skip" '
-		test_config rebase.autostash true &&
+		test_when_finished "rm -rf tmp" &&
+		(
+		setup_tmp &&
 		git reset --hard &&
-		git checkout -b rebased-feature-branch feature-branch &&
-		test_when_finished git branch -D rebased-feature-branch &&
 		echo dirty >>file3 &&
 		test_must_fail git rebase$type related-onto-branch &&
 		test_path_is_file $dotest/autostash &&
 		! grep dirty file3 &&
 		git rebase --skip &&
 		test_path_is_missing $dotest/autostash &&
-		grep dirty file3 &&
-		git checkout feature-branch
+		grep dirty file3
+		)
 	'

 	test_expect_success "rebase$type: --abort" '
-		test_config rebase.autostash true &&
-		git reset --hard &&
-		git checkout -b rebased-feature-branch feature-branch &&
-		test_when_finished git branch -D rebased-feature-branch &&
+		test_when_finished "rm -rf tmp" &&
+		(
+		setup_tmp &&
 		echo dirty >>file3 &&
 		test_must_fail git rebase$type related-onto-branch &&
 		test_path_is_file $dotest/autostash &&
 		! grep dirty file3 &&
 		git rebase --abort &&
 		test_path_is_missing $dotest/autostash &&
-		grep dirty file3 &&
-		git checkout feature-branch
+		grep dirty file3
+		)
 	'

 	test_expect_success "rebase$type: non-conflicting rebase, conflicting stash" '
-		test_config rebase.autostash true &&
-		git reset --hard &&
-		git checkout -b rebased-feature-branch feature-branch &&
-		test_when_finished git branch -D rebased-feature-branch &&
+		test_when_finished "rm -rf tmp" &&
+		(
+		setup_tmp &&
 		echo dirty >file4 &&
 		git add file4 &&
 		git rebase$type unrelated-onto-branch &&
@@ -138,6 +138,7 @@ testrebase() {
 		git checkout feature-branch &&
 		git stash pop &&
 		grep dirty file4
+		)
 	'
 }

-- 
Felipe Contreras
Ramkumar Ramachandra· Jun 8, 2013, 14:04 UTC · re: Felipe Contreras · lore

Re: [PATCH] tests: fix autostash

Felipe Contreras wrote:
>> Ofcourse they're implementation details.  Even in this very test, I
>> check $dotest/autostash plenty of times.
>
> The more the test relies on implementation details, the worst.
I'm not convinced about this.
>> Then show me how to do it correctly.
>
> Something like this.
Yeah, this is definitely better.  Can you submit this patch?
Felipe Contreras· Jun 8, 2013, 14:13 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH] tests: fix autostash

On Sat, Jun 8, 2013 at 9:04 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
Show 7 quoted lines
> Felipe Contreras wrote:
>>> Ofcourse they're implementation details.  Even in this very test, I
>>> check $dotest/autostash plenty of times.
>>
>> The more the test relies on implementation details, the worst.
>
> I'm not convinced about this.

There's even a model called test-driven development, where you start developing the tests even before there's any implementation. There's also black-box testing.

There's reasons for that.
Show 5 quoted lines
>>> Then show me how to do it correctly.
>>
>> Something like this.
>
> Yeah, this is definitely better.  Can you submit this patch?
Maybe, I don't know when.
-- 
Felipe Contreras
Antoine Pelisse· Jun 8, 2013, 14:13 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH] tests: fix autostash

On Sat, Jun 8, 2013 at 4:04 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
Show 7 quoted lines
> Felipe Contreras wrote:
>>> Ofcourse they're implementation details.  Even in this very test, I
>>> check $dotest/autostash plenty of times.
>>
>> The more the test relies on implementation details, the worst.
>
> I'm not convinced about this.

My understanding of these tests is that they make sure new/better implementations don't break the user experience/defined behavior. If the test relies on the implementation, then they lose most of their interest.

Ramkumar Ramachandra· Jun 8, 2013, 15:35 UTC · re: Antoine Pelisse · lore

Re: [PATCH] tests: fix autostash

Antoine Pelisse wrote:
Show 8 quoted lines
>>> The more the test relies on implementation details, the worst.
>>
>> I'm not convinced about this.
>
> My understanding of these tests is that they make sure new/better
> implementations don't break the user experience/defined behavior. If
> the test relies on the implementation, then they lose most of their
> interest.
Makes sense, thanks.
Jonathan Nieder· Jun 8, 2013, 16:56 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH] tests: fix autostash

Ramkumar Ramachandra wrote:
> How else am I supposed to write them?  If there is a stale state from
> the previous test, there isn't too much I can do.  Or should I be
> cleaning up state at the beginning of each test, instead of at the
> end?

That's one strategy. "test_when_finished" to restore the set-up state is another.

Making tests skippable unless labelled otherwise is currently an aspirational goal rather than a practical one. Hopefully some day we'll get there and the test harness can start checking it. :) It makes reorganizing test scripts, for example by reordering tests, much easier.

Thanks, Jonathan

← back to recent threads