Re: [PATCH 2/4] t6030: explicitly test for bisection cleanup
Hey Eric, On Wed, Jun 8, 2016 at 1:47 PM, Eric Sunshine wrote: > On Wed, Jun 8, 2016 at 4:07 AM, Pranit Bauva wrote: >> On Wed, Jun 8, 2016 at 4:51 AM, Eric Sunshine >> wrote: >>> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva wrote: diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh @@ -894,4 +894,21 @@ test_expect_success 'bisect start takes options and revs in any order' ' +test_expect_success 'git bisect reset cleans bisection state properly' ' + git bisect reset && + git bisect start && + git bisect good $HASH1 && + git bisect bad $HASH4 && + git bisect reset && + test -z "$(git for-each-ref "refs/bisect/*")" && >>> >>> I wonder if this would be more easily read as: >>> >>> git for-each-ref "refs/bisect/*" >actual && >>> test_must_be_empty actual && >> >> I just tried to imitate what the test t6030 previously had (a lot of >> occurrences). Should I still change it to your specified format? >> Should I also change the others as a side cleanup "while I am at it"? > > No, if the 'test -z "$(...)"' is already used heavily in that script, > just stick with it. As for a side cleanup, perhaps if you have time > later on, but don't let it derail your project timeline. It's not that > important. Sure! I can mark it as "to be cleaned up after GSoC" Regards, Pranit Bauva -- 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 2/4] t6030: explicitly test for bisection cleanup
On Wed, Jun 8, 2016 at 4:07 AM, Pranit Bauva wrote: > On Wed, Jun 8, 2016 at 4:51 AM, Eric Sunshine wrote: >> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva wrote: >>> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh >>> @@ -894,4 +894,21 @@ test_expect_success 'bisect start takes options and >>> revs in any order' ' >>> +test_expect_success 'git bisect reset cleans bisection state properly' ' >>> + git bisect reset && >>> + git bisect start && >>> + git bisect good $HASH1 && >>> + git bisect bad $HASH4 && >>> + git bisect reset && >>> + test -z "$(git for-each-ref "refs/bisect/*")" && >> >> I wonder if this would be more easily read as: >> >> git for-each-ref "refs/bisect/*" >actual && >> test_must_be_empty actual && > > I just tried to imitate what the test t6030 previously had (a lot of > occurrences). Should I still change it to your specified format? > Should I also change the others as a side cleanup "while I am at it"? No, if the 'test -z "$(...)"' is already used heavily in that script, just stick with it. As for a side cleanup, perhaps if you have time later on, but don't let it derail your project timeline. It's not that important. -- 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 2/4] t6030: explicitly test for bisection cleanup
Hey Eric, On Wed, Jun 8, 2016 at 4:51 AM, Eric Sunshine wrote: > On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva wrote: >> This is not an improvement in the test coverage but it helps in making >> it explicit as to what exactly would be the error as other tests are >> focussed on testing other things. > > It's not clear why you consider this as *not* improving test coverage. My mistake as I forgot to include a comment in this patch. I did it in the previous patch[1]. I manually changed the file names and saw that there were a couple of tests failing in each case. >> Signed-off-by: Pranit Bauva >> --- >> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh >> @@ -894,4 +894,21 @@ test_expect_success 'bisect start takes options and >> revs in any order' ' >> +test_expect_success 'git bisect reset cleans bisection state properly' ' >> + git bisect reset && >> + git bisect start && >> + git bisect good $HASH1 && >> + git bisect bad $HASH4 && >> + git bisect reset && >> + test -z "$(git for-each-ref "refs/bisect/*")" && > > I wonder if this would be more easily read as: > > git for-each-ref "refs/bisect/*" >actual && > test_must_be_empty actual && I just tried to imitate what the test t6030 previously had (a lot of occurrences). Should I still change it to your specified format? Should I also change the others as a side cleanup "while I am at it"? >> + ! test -s "$GIT_DIR/BISECT_EXPECTED_REV" && >> + ! test -s "$GIT_DIR/BISECT_ANCESTORS_OK" && >> + ! test -s "$GIT_DIR/BISECT_LOG" && >> + ! test -s "$GIT_DIR/BISECT_RUN" && >> + ! test -s "$GIT_DIR/BISECT_TERMS" && >> + ! test -s "$GIT_DIR/head-name" && >> + ! test -s "$GIT_DIR/BISECT_HEAD" && >> + ! test -s "$GIT_DIR/BISECT_START" > > Is it the intention that these should verify that the files don't > exist? Maybe use test_path_is_missing() instead? True. In fact it would be much more appropriate to use test_path_is_missing() as we are using remove_path() and thus there shouldn't even exist a file with file size != 0. Thanks! >> +' >> + >> test_done [1]: http://thread.gmane.org/gmane.comp.version-control.git/294520 Regards, Pranit Bauva -- 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 2/4] t6030: explicitly test for bisection cleanup
On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva wrote: > This is not an improvement in the test coverage but it helps in making > it explicit as to what exactly would be the error as other tests are > focussed on testing other things. It's not clear why you consider this as *not* improving test coverage. > Signed-off-by: Pranit Bauva > --- > diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh > @@ -894,4 +894,21 @@ test_expect_success 'bisect start takes options and revs > in any order' ' > +test_expect_success 'git bisect reset cleans bisection state properly' ' > + git bisect reset && > + git bisect start && > + git bisect good $HASH1 && > + git bisect bad $HASH4 && > + git bisect reset && > + test -z "$(git for-each-ref "refs/bisect/*")" && I wonder if this would be more easily read as: git for-each-ref "refs/bisect/*" >actual && test_must_be_empty actual && > + ! test -s "$GIT_DIR/BISECT_EXPECTED_REV" && > + ! test -s "$GIT_DIR/BISECT_ANCESTORS_OK" && > + ! test -s "$GIT_DIR/BISECT_LOG" && > + ! test -s "$GIT_DIR/BISECT_RUN" && > + ! test -s "$GIT_DIR/BISECT_TERMS" && > + ! test -s "$GIT_DIR/head-name" && > + ! test -s "$GIT_DIR/BISECT_HEAD" && > + ! test -s "$GIT_DIR/BISECT_START" Is it the intention that these should verify that the files don't exist? Maybe use test_path_is_missing() instead? > +' > + > test_done -- 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
