Re: [PATCH 4/5] test: improve rebase -q test
On Wed, May 29, 2013 at 11:52 AM, Junio C Hamano wrote: > Felipe Contreras writes: > >> Junio C Hamano wrote: >>> Felipe Contreras writes: >>> >>> > Let's show the output so it's clear why it failed. >>> > >>> > Signed-off-by: Felipe Contreras >>> > --- >>> > t/t3400-rebase.sh | 1 + >>> > 1 file changed, 1 insertion(+) >>> > >>> > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh >>> > index b58fa1a..fb39531 100755 >>> > --- a/t/t3400-rebase.sh >>> > +++ b/t/t3400-rebase.sh >>> > @@ -185,6 +185,7 @@ test_expect_success 'default to @{upstream} when >>> > upstream arg is missing' ' >>> > test_expect_success 'rebase -q is quiet' ' >>> >git checkout -b quiet topic && >>> >git rebase -q master >output.out 2>&1 && >>> > + cat output.out && >>> >test ! -s output.out >>> > ' >>> >>> It is one thing to avoid squelching output that naturally comes out >>> of command being tested unnecessarily, so that "./t-*.sh -v" >>> output can be used for debugging. I however am not sure if adding >>> "cat" to random places like this is a productive direction for us to >>> go in. >>> >>> A more preferrable alternative may be adding something like this to >>> test-lib.sh and call it from here and elsewhere (there are about 50 >>> places that do "test ! -s "), perhaps? >>> >>> test_must_be_an_empty_file () { >>> if test -s "$1" >>> then >>> cat "$1" >>> false >>> fi >>> } >> >> Perhaps, but I'm not interested. I'm tired of obvious fixes getting rejected >> for hypothetical "ideal" situations that we'll never reach. > > That's too bad. Addition of "cat" where there does not need one is > clearly not an obvious fix anyway. If you are an actual real user of this code; a developer that is running the test; and the test finally achieves it's designed goal of detecting a failure, you would be left scratching your head wondering what's the problem if running './test -v' doesn't show anything, even after you have added debugging code to narrow down the issue. I had to add that cat line not once, but more than two times in different lines of development. So yeah, a cat is needed, and the fact you don't see that amazes me, specially after you have reprimanded me for using 'grep -q' instead of 'grep' for this very reason. -- 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 4/5] test: improve rebase -q test
Felipe Contreras writes: > Junio C Hamano wrote: >> Felipe Contreras writes: >> >> > Let's show the output so it's clear why it failed. >> > >> > Signed-off-by: Felipe Contreras >> > --- >> > t/t3400-rebase.sh | 1 + >> > 1 file changed, 1 insertion(+) >> > >> > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh >> > index b58fa1a..fb39531 100755 >> > --- a/t/t3400-rebase.sh >> > +++ b/t/t3400-rebase.sh >> > @@ -185,6 +185,7 @@ test_expect_success 'default to @{upstream} when >> > upstream arg is missing' ' >> > test_expect_success 'rebase -q is quiet' ' >> >git checkout -b quiet topic && >> >git rebase -q master >output.out 2>&1 && >> > + cat output.out && >> >test ! -s output.out >> > ' >> >> It is one thing to avoid squelching output that naturally comes out >> of command being tested unnecessarily, so that "./t-*.sh -v" >> output can be used for debugging. I however am not sure if adding >> "cat" to random places like this is a productive direction for us to >> go in. >> >> A more preferrable alternative may be adding something like this to >> test-lib.sh and call it from here and elsewhere (there are about 50 >> places that do "test ! -s "), perhaps? >> >> test_must_be_an_empty_file () { >> if test -s "$1" >> then >> cat "$1" >> false >> fi >> } > > Perhaps, but I'm not interested. I'm tired of obvious fixes getting rejected > for hypothetical "ideal" situations that we'll never reach. That's too bad. Addition of "cat" where there does not need one is clearly not an obvious fix anyway. -- 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 4/5] test: improve rebase -q test
Junio C Hamano wrote: > Felipe Contreras writes: > > > Let's show the output so it's clear why it failed. > > > > Signed-off-by: Felipe Contreras > > --- > > t/t3400-rebase.sh | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh > > index b58fa1a..fb39531 100755 > > --- a/t/t3400-rebase.sh > > +++ b/t/t3400-rebase.sh > > @@ -185,6 +185,7 @@ test_expect_success 'default to @{upstream} when > > upstream arg is missing' ' > > test_expect_success 'rebase -q is quiet' ' > > git checkout -b quiet topic && > > git rebase -q master >output.out 2>&1 && > > + cat output.out && > > test ! -s output.out > > ' > > It is one thing to avoid squelching output that naturally comes out > of command being tested unnecessarily, so that "./t-*.sh -v" > output can be used for debugging. I however am not sure if adding > "cat" to random places like this is a productive direction for us to > go in. > > A more preferrable alternative may be adding something like this to > test-lib.sh and call it from here and elsewhere (there are about 50 > places that do "test ! -s "), perhaps? > > test_must_be_an_empty_file () { > if test -s "$1" > then > cat "$1" > false > fi > } Perhaps, but I'm not interested. I'm tired of obvious fixes getting rejected for hypothetical "ideal" situations that we'll never reach. -- 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 4/5] test: improve rebase -q test
Jonathan Nieder writes: > Junio C Hamano wrote: > >> A more preferrable alternative may be adding something like this to >> test-lib.sh and call it from here and elsewhere (there are about 50 >> places that do "test ! -s "), perhaps? >> >> test_must_be_an_empty_file () { >> if test -s "$1" >> then >> cat "$1" >> false >> fi >> } > > I generally just use the two-liner > > >empty && > test_cmp empty output > > directly in cases like this. That would work, too. -- 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 4/5] test: improve rebase -q test
Junio C Hamano wrote: > A more preferrable alternative may be adding something like this to > test-lib.sh and call it from here and elsewhere (there are about 50 > places that do "test ! -s "), perhaps? > > test_must_be_an_empty_file () { > if test -s "$1" > then > cat "$1" > false > fi > } I generally just use the two-liner >empty && test_cmp empty output directly in cases like this. 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
Re: [PATCH 4/5] test: improve rebase -q test
Felipe Contreras writes: > Let's show the output so it's clear why it failed. > > Signed-off-by: Felipe Contreras > --- > t/t3400-rebase.sh | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh > index b58fa1a..fb39531 100755 > --- a/t/t3400-rebase.sh > +++ b/t/t3400-rebase.sh > @@ -185,6 +185,7 @@ test_expect_success 'default to @{upstream} when upstream > arg is missing' ' > test_expect_success 'rebase -q is quiet' ' > git checkout -b quiet topic && > git rebase -q master >output.out 2>&1 && > + cat output.out && > test ! -s output.out > ' It is one thing to avoid squelching output that naturally comes out of command being tested unnecessarily, so that "./t-*.sh -v" output can be used for debugging. I however am not sure if adding "cat" to random places like this is a productive direction for us to go in. A more preferrable alternative may be adding something like this to test-lib.sh and call it from here and elsewhere (there are about 50 places that do "test ! -s "), perhaps? test_must_be_an_empty_file () { if test -s "$1" then cat "$1" false fi } -- 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 4/5] test: improve rebase -q test
Let's show the output so it's clear why it failed. Signed-off-by: Felipe Contreras --- t/t3400-rebase.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index b58fa1a..fb39531 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -185,6 +185,7 @@ test_expect_success 'default to @{upstream} when upstream arg is missing' ' test_expect_success 'rebase -q is quiet' ' git checkout -b quiet topic && git rebase -q master >output.out 2>&1 && + cat output.out && test ! -s output.out ' -- 1.8.3.rc3.312.g47657de -- 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