Re: [PATCH] tests: fix autostash

2013-06-08 Thread Felipe Contreras 
On Fri, Jun 7, 2013 at 10:29 PM, Ramkumar Ramachandra
artag...@gmail.com wrote:
 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.

--- 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: fix autostash

2013-06-08 Thread Ramkumar Ramachandra 
Felipe Contreras wrote:
 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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: fix autostash

2013-06-08 Thread Felipe Contreras 
On Sat, Jun 8, 2013 at 8:19 AM, Ramkumar Ramachandra artag...@gmail.com wrote:
 Felipe Contreras wrote:

 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.

 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.

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

Re: [PATCH] tests: fix autostash

2013-06-08 Thread Ramkumar Ramachandra 
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?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: fix autostash

2013-06-08 Thread Felipe Contreras 
On Sat, Jun 8, 2013 at 9:04 AM, Ramkumar Ramachandra artag...@gmail.com wrote:
 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.

 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: fix autostash

2013-06-08 Thread Antoine Pelisse 
On Sat, Jun 8, 2013 at 4:04 PM, Ramkumar Ramachandra artag...@gmail.com wrote:
 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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: fix autostash

2013-06-08 Thread Ramkumar Ramachandra 
Antoine Pelisse wrote:
 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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: fix autostash

2013-06-08 Thread Jonathan Nieder 
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] tests: fix autostash

2013-06-07 Thread Felipe Contreras 
We should call 'git rebase --abort', like a normal user would do.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 t/t3420-rebase-autostash.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

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

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: fix autostash

2013-06-07 Thread Ramkumar Ramachandra 
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.  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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html