Re: [PATCH 17/29] t: use test_must_fail() instead of checking exit code manually
Am 26.06.2018 um 20:14 schrieb Eric Sunshine: On Tue, Jun 26, 2018 at 2:06 PM Johannes Sixt wrote: Hence, these lines should actually be p4 help client && ! p4 help nosuchcommand Thanks for the comment; you're right, of course. I'll certainly make this change if I have to re-roll for some other reason, but do you feel that this itself is worth a re-roll? Not worth a re-roll IMO. -- Hannes
Re: [PATCH 17/29] t: use test_must_fail() instead of checking exit code manually
On Tue, Jun 26, 2018 at 2:06 PM Johannes Sixt wrote: > Am 26.06.2018 um 11:21 schrieb Eric Sunshine: > >> On Tue, Jun 26, 2018 at 12:29 AM, Eric Sunshine > >> wrote: > >>> + p4 help client && > >>> + test_must_fail p4 help nosuchcommand > > [...] So, despite > > the (somewhat) misleading test title, this test doesn't care about the > > exact error code but rather cares only that "p4 help nosuchcommand" > > errors out, period. Hence, test_must_fail() again agrees with the > > spirit of the test. > > test_must_fail ensures that only "proper" failures are diagnosed as > expected; failures due to signals such as SEGV are not expected failures. > > In the test suite we expect all programs that are not our "git" to work > correctly; in particular, that they do not crash on anything that we ask > them to operate on. Under this assumption, the protection given by > test_must_fail is not needed. > > Hence, these lines should actually be > > p4 help client && > ! p4 help nosuchcommand Thanks for the comment; you're right, of course. I'll certainly make this change if I have to re-roll for some other reason, but do you feel that this itself is worth a re-roll?
Re: [PATCH 17/29] t: use test_must_fail() instead of checking exit code manually
Am 26.06.2018 um 11:21 schrieb Eric Sunshine: On Tue, Jun 26, 2018 at 4:58 AM Elijah Newren wrote: On Tue, Jun 26, 2018 at 12:29 AM, Eric Sunshine wrote: + p4 help client && + test_must_fail p4 help nosuchcommand same question? Same answer. Not shown in this patch, but just above the context lines you will find this comment in the file: # We rely on this behavior to detect for p4 move availability. which means that the test is really interested in being able to reliably detect if a sub-command is or is not available. So, despite the (somewhat) misleading test title, this test doesn't care about the exact error code but rather cares only that "p4 help nosuchcommand" errors out, period. Hence, test_must_fail() again agrees with the spirit of the test. test_must_fail ensures that only "proper" failures are diagnosed as expected; failures due to signals such as SEGV are not expected failures. In the test suite we expect all programs that are not our "git" to work correctly; in particular, that they do not crash on anything that we ask them to operate on. Under this assumption, the protection given by test_must_fail is not needed. Hence, these lines should actually be p4 help client && ! p4 help nosuchcommand -- Hannes
Re: [PATCH 17/29] t: use test_must_fail() instead of checking exit code manually
On Tue, Jun 26, 2018 at 4:58 AM Elijah Newren wrote: > On Tue, Jun 26, 2018 at 12:29 AM, Eric Sunshine > wrote: > > [...] Therefore, > > replace the manual exit code management with test_must_fail() and a > > normal &&-chain. > > > > Signed-off-by: Eric Sunshine > > --- > > diff --git a/t/t5405-send-pack-rewind.sh b/t/t5405-send-pack-rewind.sh > > @@ -25,8 +25,7 @@ test_expect_success 'non forced push should die not > > segfault' '> > - git push .. master:master > > - test $? = 1 > > + test_must_fail git push .. master:master > > test_must_fail or test_expect_code 1? A legitimate question, and the answer is that it's a judgment call based upon the spirit of the test. Although test_expect_code() would make for a faithful literal conversion, I don't think it agrees with the spirit of this test, which wants to verify that the command correctly exits with an error (in the general sense), as opposed to outright crashing (which it did at one time). test_must_fail() is tailor-made for this use case. Contrast this with patch 09/29 "t7810: use test_expect_code() instead of hand-rolled comparison". Different exit codes from git-grep have genuine different meanings, and that test is checking for a very specific exit code, for which test_expect_code() is tailor-made. > > diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh > > @@ -12,20 +12,8 @@ test_expect_success 'start p4d' ' > > test_expect_success 'p4 help unknown returns 1' ' > > ( > > cd "$cli" && > > - ( > > - p4 help client >errs 2>&1 > > - echo $? >retval > > - ) > > - echo 0 >expected && > > - test_cmp expected retval && > > - rm retval && > > - ( > > - p4 help nosuchcommand >errs 2>&1 > > - echo $? >retval > > - ) > > - echo 1 >expected && > > - test_cmp expected retval && > > - rm retval > > + p4 help client && > > + test_must_fail p4 help nosuchcommand > > same question? Same answer. Not shown in this patch, but just above the context lines you will find this comment in the file: # We rely on this behavior to detect for p4 move availability. which means that the test is really interested in being able to reliably detect if a sub-command is or is not available. So, despite the (somewhat) misleading test title, this test doesn't care about the exact error code but rather cares only that "p4 help nosuchcommand" errors out, period. Hence, test_must_fail() again agrees with the spirit of the test.
Re: [PATCH 17/29] t: use test_must_fail() instead of checking exit code manually
On Tue, Jun 26, 2018 at 12:29 AM, Eric Sunshine wrote: > These tests intentionally break the &&-chain to manually check the exit > code of invoked commands which they expect to fail, and invert that > local expected failure into a successful exit code for the test overall. > Such manual exit code manipulation predates the invention of > test_must_fail(). > > An upcoming change will teach --chain-lint to check the &&-chain inside > subshells. This sort of manual exit code checking will trip up > --chain-lint due to the intentional break in the &&-chain. Therefore, > replace the manual exit code management with test_must_fail() and a > normal &&-chain. > > Signed-off-by: Eric Sunshine > --- > t/t5405-send-pack-rewind.sh | 3 +-- > t/t9814-git-p4-rename.sh| 16 ++-- > 2 files changed, 3 insertions(+), 16 deletions(-) > > diff --git a/t/t5405-send-pack-rewind.sh b/t/t5405-send-pack-rewind.sh > index 4bda18a662..235fb7686a 100755 > --- a/t/t5405-send-pack-rewind.sh > +++ b/t/t5405-send-pack-rewind.sh > @@ -25,8 +25,7 @@ test_expect_success 'non forced push should die not > segfault' ' > > ( > cd another && > - git push .. master:master > - test $? = 1 > + test_must_fail git push .. master:master test_must_fail or test_expect_code 1? > ) > > ' > diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh > index e7e0268e98..80aac5ab16 100755 > --- a/t/t9814-git-p4-rename.sh > +++ b/t/t9814-git-p4-rename.sh > @@ -12,20 +12,8 @@ test_expect_success 'start p4d' ' > test_expect_success 'p4 help unknown returns 1' ' > ( > cd "$cli" && > - ( > - p4 help client >errs 2>&1 > - echo $? >retval > - ) > - echo 0 >expected && > - test_cmp expected retval && > - rm retval && > - ( > - p4 help nosuchcommand >errs 2>&1 > - echo $? >retval > - ) > - echo 1 >expected && > - test_cmp expected retval && > - rm retval > + p4 help client && > + test_must_fail p4 help nosuchcommand same question? > ) > ' Overall, looks much nicer.
Re: [PATCH 17/29] t: use test_must_fail() instead of checking exit code manually
On 26 June 2018 at 08:29, Eric Sunshine wrote: > These tests intentionally break the &&-chain to manually check the exit > code of invoked commands which they expect to fail, and invert that > local expected failure into a successful exit code for the test overall. > Such manual exit code manipulation predates the invention of > test_must_fail(). > > An upcoming change will teach --chain-lint to check the &&-chain inside > subshells. This sort of manual exit code checking will trip up > --chain-lint due to the intentional break in the &&-chain. Therefore, > replace the manual exit code management with test_must_fail() and a > normal &&-chain. > > Signed-off-by: Eric Sunshine > --- > t/t5405-send-pack-rewind.sh | 3 +-- > t/t9814-git-p4-rename.sh| 16 ++-- > 2 files changed, 3 insertions(+), 16 deletions(-) > > diff --git a/t/t5405-send-pack-rewind.sh b/t/t5405-send-pack-rewind.sh > index 4bda18a662..235fb7686a 100755 > --- a/t/t5405-send-pack-rewind.sh > +++ b/t/t5405-send-pack-rewind.sh > @@ -25,8 +25,7 @@ test_expect_success 'non forced push should die not > segfault' ' > > ( > cd another && > - git push .. master:master > - test $? = 1 > + test_must_fail git push .. master:master > ) > > ' > diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh > index e7e0268e98..80aac5ab16 100755 > --- a/t/t9814-git-p4-rename.sh > +++ b/t/t9814-git-p4-rename.sh > @@ -12,20 +12,8 @@ test_expect_success 'start p4d' ' > test_expect_success 'p4 help unknown returns 1' ' > ( > cd "$cli" && > - ( > - p4 help client >errs 2>&1 > - echo $? >retval > - ) > - echo 0 >expected && > - test_cmp expected retval && > - rm retval && > - ( > - p4 help nosuchcommand >errs 2>&1 > - echo $? >retval > - ) > - echo 1 >expected && > - test_cmp expected retval && > - rm retval > + p4 help client && > + test_must_fail p4 help nosuchcommand This seems a lot more sensible. It works fine in my tests. Thanks, Luke
