{"thread":{"id":"34060","subject":"[PATCH] tests: fix autostash","startedAt":"2013-06-07T23:45:14Z","lastAt":"2013-06-08T16:56:11Z","messageCount":10,"participants":["Felipe Contreras","Ramkumar Ramachandra","Antoine Pelisse","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"219766","messageId":"1370648714-7872-1-git-send-email-felipe.contreras@gmail.com","threadId":"34060","inReplyTo":null,"subject":"[PATCH] tests: fix autostash","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-07T23:45:14Z","receivedAt":"2013-06-07T23:45:14Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"We should call 'git rebase --abort', like a normal user would do.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n t/t3420-rebase-autostash.sh | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\nindex a5e69f3..ff370a3 100755\n--- a/t/t3420-rebase-autostash.sh\n+++ b/t/t3420-rebase-autostash.sh\n@@ -71,8 +71,7 @@ testrebase() {\n \t\ttest_must_fail git rebase$type related-onto-branch &&\n \t\ttest_path_is_file $dotest/autostash &&\n \t\t! grep dirty file3 &&\n-\t\trm -rf $dotest &&\n-\t\tgit reset --hard &&\n+\t\tgit rebase --abort &&\n \t\tgit checkout feature-branch\n \t'\n \n-- \n1.8.3.698.g079b096\n"},{"id":"219782","messageId":"CALkWK0=OdcLDCd9OonUS6kBZhA=oz3HYJT8Eiqnh1A++s5yWww@mail.gmail.com","threadId":"34060","inReplyTo":"1370648714-7872-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH] tests: fix autostash","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-08T03:29:46Z","receivedAt":"2013-06-08T03:29:46Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Felipe Contreras wrote:\n> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\n> index a5e69f3..ff370a3 100755\n> --- a/t/t3420-rebase-autostash.sh\n> +++ b/t/t3420-rebase-autostash.sh\n> @@ -71,8 +71,7 @@ testrebase() {\n>                 test_must_fail git rebase$type related-onto-branch &&\n>                 test_path_is_file $dotest/autostash &&\n>                 ! grep dirty file3 &&\n> -               rm -rf $dotest &&\n> -               git reset --hard &&\n> +               git rebase --abort &&\n>                 git checkout feature-branch\n>         '\n\nIncorrect.  I don't assume that --abort works yet, in this test.  It\nis non-trivial to get --abort to work with autostash, and this is\ntested in a separate test: see \"rebase$type: --abort\" in the same\nfile.\n"},{"id":"219797","messageId":"CAMP44s3rYXOmGWR3JM8xy8ON4pqDRhYWVMQqOAyODVr5myq+yg@mail.gmail.com","threadId":"34060","inReplyTo":"CALkWK0=OdcLDCd9OonUS6kBZhA=oz3HYJT8Eiqnh1A++s5yWww@mail.gmail.com","subject":"Re: [PATCH] tests: fix autostash","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-08T10:24:00Z","receivedAt":"2013-06-08T10:24:00Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, Jun 7, 2013 at 10:29 PM, Ramkumar Ramachandra\n<artagnon@gmail.com> wrote:\n> Felipe Contreras wrote:\n>> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\n>> index a5e69f3..ff370a3 100755\n>> --- a/t/t3420-rebase-autostash.sh\n>> +++ b/t/t3420-rebase-autostash.sh\n>> @@ -71,8 +71,7 @@ testrebase() {\n>>                 test_must_fail git rebase$type related-onto-branch &&\n>>                 test_path_is_file $dotest/autostash &&\n>>                 ! grep dirty file3 &&\n>> -               rm -rf $dotest &&\n>> -               git reset --hard &&\n>> +               git rebase --abort &&\n>>                 git checkout feature-branch\n>>         '\n>\n> Incorrect.  I don't assume that --abort works yet, in this test.\n\nYes you do. The rest of the tests expect that the previous rebase has\nbeen aborted.\n\nIn fact, all the tests depend on the previous test finishing\ncorrectly, which is not the way tests should be written.\n\n--- a/t/t3420-rebase-autostash.sh\n+++ b/t/t3420-rebase-autostash.sh\n@@ -70,7 +70,7 @@ testrebase() {\n                echo dirty >>file3 &&\n                test_must_fail git rebase$type related-onto-branch &&\n                test_path_is_file $dotest/autostash &&\n-               ! grep dirty file3 &&\n+               false ! grep dirty file3 &&\n                rm -rf $dotest &&\n                git reset --hard &&\n                git checkout feature-branch\n\n# failed 19 among 22 test(s)\n\nDoing 'rm -rf $dotest' is even worst than 'git rebase --abort',\nbecause it relies on the implementation of 'git rebase', which might\nneed to remove more files than $dotest.\n\nThis wouldn't be a problem if the tests were implemented correctly,\nbut they are not, so 'git rebase --abort' is the only sane option.\n\n-- \nFelipe Contreras\n"},{"id":"219815","messageId":"CALkWK0=qVXL=DtJzYRGFLQQh3_-cGC_t_zpaiTL9KVKN4--mVg@mail.gmail.com","threadId":"34060","inReplyTo":"CAMP44s3rYXOmGWR3JM8xy8ON4pqDRhYWVMQqOAyODVr5myq+yg@mail.gmail.com","subject":"Re: [PATCH] tests: fix autostash","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-08T13:19:52Z","receivedAt":"2013-06-08T13:19:52Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Felipe Contreras wrote:\n> Yes you do. The rest of the tests expect that the previous rebase has\n> been aborted.\n>\n> In fact, all the tests depend on the previous test finishing\n> correctly, which is not the way tests should be written.\n\nHow else am I supposed to write them?  If there is a stale state from\nthe previous test, there isn't too much I can do.  Or should I be\ncleaning up state at the beginning of each test, instead of at the\nend?\n\n> Doing 'rm -rf $dotest' is even worst than 'git rebase --abort',\n> because it relies on the implementation of 'git rebase', which might\n> need to remove more files than $dotest.\n\nHuh?  Tests aren't allowed to rely on how a command is implemented?\n\n  $ git grep test_path t\n\nOfcourse they're implementation details.  Even in this very test, I\ncheck $dotest/autostash plenty of times.\n\nHave you read rr/rebase-autostash?  The whole idea is to inject\n$dotest/autostash and teach various scripts about how their\nassumptions about $dotest have changed.\n\n> This wouldn't be a problem if the tests were implemented correctly,\n> but they are not, so 'git rebase --abort' is the only sane option.\n\nThen show me how to do it correctly.\n"},{"id":"219823","messageId":"CAMP44s1p-VDMLNhoym5fH3XAn8CWqu2yqGq0mb+55qHOq-r_ww@mail.gmail.com","threadId":"34060","inReplyTo":"CALkWK0=qVXL=DtJzYRGFLQQh3_-cGC_t_zpaiTL9KVKN4--mVg@mail.gmail.com","subject":"Re: [PATCH] tests: fix autostash","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-08T14:01:05Z","receivedAt":"2013-06-08T14:01:05Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Jun 8, 2013 at 8:19 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:\n> Felipe Contreras wrote:\n\n>> Doing 'rm -rf $dotest' is even worst than 'git rebase --abort',\n>> because it relies on the implementation of 'git rebase', which might\n>> need to remove more files than $dotest.\n>\n> Huh?  Tests aren't allowed to rely on how a command is implemented?\n>\n>   $ git grep test_path t\n>\n> Ofcourse they're implementation details.  Even in this very test, I\n> check $dotest/autostash plenty of times.\n\nThe more the test relies on implementation details, the worst.\n\n> Have you read rr/rebase-autostash?  The whole idea is to inject\n> $dotest/autostash and teach various scripts about how their\n> assumptions about $dotest have changed.\n>\n>> This wouldn't be a problem if the tests were implemented correctly,\n>> but they are not, so 'git rebase --abort' is the only sane option.\n>\n> Then show me how to do it correctly.\n\nSomething like this.\n\ndiff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\nindex 6ba449b..b96a56a 100755\n--- a/t/t3420-rebase-autostash.sh\n+++ b/t/t3420-rebase-autostash.sh\n@@ -33,54 +33,56 @@ test_expect_success setup '\n \tgit commit -m \"related commit\"\n '\n\n+setup_tmp () {\n+\tgit clone . tmp &&\n+\tcd tmp &&\n+\tgit fetch -u origin \"refs/heads/*:refs/heads/*\" &&\n+\ttest_config rebase.autostash true &&\n+\tgit checkout -b rebased-feature-branch feature-branch\n+}\n+\n testrebase() {\n \ttype=$1\n \tdotest=$2\n\n \ttest_expect_success \"rebase$type: dirty worktree, non-conflicting rebase\" '\n-\t\ttest_config rebase.autostash true &&\n-\t\tgit reset --hard &&\n-\t\tgit checkout -b rebased-feature-branch feature-branch &&\n-\t\ttest_when_finished git branch -D rebased-feature-branch &&\n+\t\ttest_when_finished \"rm -rf tmp\" &&\n+\t\t(\n+\t\tsetup_tmp &&\n \t\techo dirty >>file3 &&\n \t\tgit rebase$type unrelated-onto-branch &&\n \t\tgrep unrelated file4 &&\n-\t\tgrep dirty file3 &&\n-\t\tgit checkout feature-branch\n+\t\tgrep dirty file3\n+\t\t)\n \t'\n\n \ttest_expect_success \"rebase$type: dirty index, non-conflicting rebase\" '\n-\t\ttest_config rebase.autostash true &&\n-\t\tgit reset --hard &&\n-\t\tgit checkout -b rebased-feature-branch feature-branch &&\n-\t\ttest_when_finished git branch -D rebased-feature-branch &&\n+\t\ttest_when_finished \"rm -rf tmp\" &&\n+\t\t(\n+\t\tsetup_tmp &&\n \t\techo dirty >>file3 &&\n \t\tgit add file3 &&\n \t\tgit rebase$type unrelated-onto-branch &&\n \t\tgrep unrelated file4 &&\n-\t\tgrep dirty file3 &&\n-\t\tgit checkout feature-branch\n+\t\tgrep dirty file3\n+\t\t)\n \t'\n\n \ttest_expect_success \"rebase$type: conflicting rebase\" '\n-\t\ttest_config rebase.autostash true &&\n-\t\tgit reset --hard &&\n-\t\tgit checkout -b rebased-feature-branch feature-branch &&\n-\t\ttest_when_finished git branch -D rebased-feature-branch &&\n+\t\ttest_when_finished \"rm -rf tmp\" &&\n+\t\t(\n+\t\tsetup_tmp &&\n \t\techo dirty >>file3 &&\n \t\ttest_must_fail git rebase$type related-onto-branch &&\n \t\ttest_path_is_file $dotest/autostash &&\n-\t\tfalse ! grep dirty file3 &&\n-\t\trm -rf $dotest &&\n-\t\tgit reset --hard &&\n-\t\tgit checkout feature-branch\n+\t\t! grep dirty file3\n+\t\t)\n \t'\n\n \ttest_expect_success \"rebase$type: --continue\" '\n-\t\ttest_config rebase.autostash true &&\n-\t\tgit reset --hard &&\n-\t\tgit checkout -b rebased-feature-branch feature-branch &&\n-\t\ttest_when_finished git branch -D rebased-feature-branch &&\n+\t\ttest_when_finished \"rm -rf tmp\" &&\n+\t\t(\n+\t\tsetup_tmp &&\n \t\techo dirty >>file3 &&\n \t\ttest_must_fail git rebase$type related-onto-branch &&\n \t\ttest_path_is_file $dotest/autostash &&\n@@ -89,45 +91,43 @@ testrebase() {\n \t\tgit add file2 &&\n \t\tgit rebase --continue &&\n \t\ttest_path_is_missing $dotest/autostash &&\n-\t\tgrep dirty file3 &&\n-\t\tgit checkout feature-branch\n+\t\tgrep dirty file3\n+\t\t)\n \t'\n\n \ttest_expect_success \"rebase$type: --skip\" '\n-\t\ttest_config rebase.autostash true &&\n+\t\ttest_when_finished \"rm -rf tmp\" &&\n+\t\t(\n+\t\tsetup_tmp &&\n \t\tgit reset --hard &&\n-\t\tgit checkout -b rebased-feature-branch feature-branch &&\n-\t\ttest_when_finished git branch -D rebased-feature-branch &&\n \t\techo dirty >>file3 &&\n \t\ttest_must_fail git rebase$type related-onto-branch &&\n \t\ttest_path_is_file $dotest/autostash &&\n \t\t! grep dirty file3 &&\n \t\tgit rebase --skip &&\n \t\ttest_path_is_missing $dotest/autostash &&\n-\t\tgrep dirty file3 &&\n-\t\tgit checkout feature-branch\n+\t\tgrep dirty file3\n+\t\t)\n \t'\n\n \ttest_expect_success \"rebase$type: --abort\" '\n-\t\ttest_config rebase.autostash true &&\n-\t\tgit reset --hard &&\n-\t\tgit checkout -b rebased-feature-branch feature-branch &&\n-\t\ttest_when_finished git branch -D rebased-feature-branch &&\n+\t\ttest_when_finished \"rm -rf tmp\" &&\n+\t\t(\n+\t\tsetup_tmp &&\n \t\techo dirty >>file3 &&\n \t\ttest_must_fail git rebase$type related-onto-branch &&\n \t\ttest_path_is_file $dotest/autostash &&\n \t\t! grep dirty file3 &&\n \t\tgit rebase --abort &&\n \t\ttest_path_is_missing $dotest/autostash &&\n-\t\tgrep dirty file3 &&\n-\t\tgit checkout feature-branch\n+\t\tgrep dirty file3\n+\t\t)\n \t'\n\n \ttest_expect_success \"rebase$type: non-conflicting rebase, conflicting stash\" '\n-\t\ttest_config rebase.autostash true &&\n-\t\tgit reset --hard &&\n-\t\tgit checkout -b rebased-feature-branch feature-branch &&\n-\t\ttest_when_finished git branch -D rebased-feature-branch &&\n+\t\ttest_when_finished \"rm -rf tmp\" &&\n+\t\t(\n+\t\tsetup_tmp &&\n \t\techo dirty >file4 &&\n \t\tgit add file4 &&\n \t\tgit rebase$type unrelated-onto-branch &&\n@@ -138,6 +138,7 @@ testrebase() {\n \t\tgit checkout feature-branch &&\n \t\tgit stash pop &&\n \t\tgrep dirty file4\n+\t\t)\n \t'\n }\n\n-- \nFelipe Contreras\n"},{"id":"219826","messageId":"CALkWK0mBnG7houMKHDorz3oSm=R7u4Va1s=AepSM0CPY6Gy4cw@mail.gmail.com","threadId":"34060","inReplyTo":"CAMP44s1p-VDMLNhoym5fH3XAn8CWqu2yqGq0mb+55qHOq-r_ww@mail.gmail.com","subject":"Re: [PATCH] tests: fix autostash","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-08T14:04:51Z","receivedAt":"2013-06-08T14:04:51Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Felipe Contreras wrote:\n>> Ofcourse they're implementation details.  Even in this very test, I\n>> check $dotest/autostash plenty of times.\n>\n> The more the test relies on implementation details, the worst.\n\nI'm not convinced about this.\n\n>> Then show me how to do it correctly.\n>\n> Something like this.\n\nYeah, this is definitely better.  Can you submit this patch?\n"},{"id":"219829","messageId":"CAMP44s0MZ34J=C0r_V8MN-nYrDbi7buy5vOKtHU_jKRFvyGbXw@mail.gmail.com","threadId":"34060","inReplyTo":"CALkWK0mBnG7houMKHDorz3oSm=R7u4Va1s=AepSM0CPY6Gy4cw@mail.gmail.com","subject":"Re: [PATCH] tests: fix autostash","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-08T14:13:10Z","receivedAt":"2013-06-08T14:13:10Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Jun 8, 2013 at 9:04 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:\n> Felipe Contreras wrote:\n>>> Ofcourse they're implementation details.  Even in this very test, I\n>>> check $dotest/autostash plenty of times.\n>>\n>> The more the test relies on implementation details, the worst.\n>\n> I'm not convinced about this.\n\nThere's even a model called test-driven development, where you start\ndeveloping the tests even before there's any implementation. There's\nalso black-box testing.\n\nThere's reasons for that.\n\n>>> Then show me how to do it correctly.\n>>\n>> Something like this.\n>\n> Yeah, this is definitely better.  Can you submit this patch?\n\nMaybe, I don't know when.\n\n-- \nFelipe Contreras\n"},{"id":"219830","messageId":"CALWbr2x4xGjYXa30vpbK4Q2WAVfmzuz1jNWFyGKSPO0To4P-0w@mail.gmail.com","threadId":"34060","inReplyTo":"CALkWK0mBnG7houMKHDorz3oSm=R7u4Va1s=AepSM0CPY6Gy4cw@mail.gmail.com","subject":"Re: [PATCH] tests: fix autostash","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-06-08T14:13:59Z","receivedAt":"2013-06-08T14:13:59Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"On Sat, Jun 8, 2013 at 4:04 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:\n> Felipe Contreras wrote:\n>>> Ofcourse they're implementation details.  Even in this very test, I\n>>> check $dotest/autostash plenty of times.\n>>\n>> The more the test relies on implementation details, the worst.\n>\n> I'm not convinced about this.\n\nMy understanding of these tests is that they make sure new/better\nimplementations don't break the user experience/defined behavior. If\nthe test relies on the implementation, then they lose most of their\ninterest.\n"},{"id":"219834","messageId":"CALkWK0k6Ut_uiV753cAQLNt7UsCuhMUViEBO5V-Fp7NoCY---A@mail.gmail.com","threadId":"34060","inReplyTo":"CALWbr2x4xGjYXa30vpbK4Q2WAVfmzuz1jNWFyGKSPO0To4P-0w@mail.gmail.com","subject":"Re: [PATCH] tests: fix autostash","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-08T15:35:11Z","receivedAt":"2013-06-08T15:35:11Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Antoine Pelisse wrote:\n>>> The more the test relies on implementation details, the worst.\n>>\n>> I'm not convinced about this.\n>\n> My understanding of these tests is that they make sure new/better\n> implementations don't break the user experience/defined behavior. If\n> the test relies on the implementation, then they lose most of their\n> interest.\n\nMakes sense, thanks.\n"},{"id":"219839","messageId":"20130608165611.GB3109@elie.Belkin","threadId":"34060","inReplyTo":"CALkWK0=qVXL=DtJzYRGFLQQh3_-cGC_t_zpaiTL9KVKN4--mVg@mail.gmail.com","subject":"Re: [PATCH] tests: fix autostash","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-06-08T16:56:11Z","receivedAt":"2013-06-08T16:56:11Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> How else am I supposed to write them?  If there is a stale state from\n> the previous test, there isn't too much I can do.  Or should I be\n> cleaning up state at the beginning of each test, instead of at the\n> end?\n\nThat's one strategy.  \"test_when_finished\" to restore the set-up\nstate is another.\n\nMaking tests skippable unless labelled otherwise is currently an\naspirational goal rather than a practical one.  Hopefully some day\nwe'll get there and the test harness can start checking it. :)  It\nmakes reorganizing test scripts, for example by reordering tests, much\neasier.\n\nThanks,\nJonathan\n"}]}