diff mbox

Do not terminate default test runs on test failure

Message ID Pine.LNX.4.64.1403131502030.7091@digraph.polyomino.org.uk
State Committed
Headers show

Commit Message

Joseph Myers March 13, 2014, 3:07 p.m. UTC
On Thu, 13 Mar 2014, Carlos O'Donell wrote:

> I would prefer you split this into two patches, a minimal patch
> to support running without `-k' and another for all the other
> changes, but I'm not opposed to one patch for this change.
> The first patch would change nothing, but add the variable,
> and the second patch changes the default and includes the summary
> changes.

I'm not clear on what you're proposing in light of Roland's and Brooks's 
comments about exit status (which require reworking the documentation 
parts of the patch anyway).

> > diff --git a/INSTALL b/INSTALL
> > index bfa692d..bcb53b8 100644
> > --- a/INSTALL
> > +++ b/INSTALL
> > @@ -154,7 +154,7 @@ will be used, and CFLAGS sets optimization options for the compiler.
> >  
> >       If you only specify `--host', `configure' will prepare for a
> >       native compile but use what you specify instead of guessing what
> > -     your system is. This is most useful to change the CPU submodel.
> > +     your system is.  This is most useful to change the CPU submodel.
> 
> OK. Split into another patch?

I've committed the regeneration below as obvious.

> > @@ -324,10 +324,20 @@ tests: $(tests-special)
> >  	$(..)scripts/merge-test-results.sh -t $(objpfx) subdir-tests.sum \
> >  	  $(sort $(subdirs) .) \
> >  	  > $(objpfx)tests.sum
> > +	@grep '^ERROR:' $(objpfx)tests.sum || true
> > +	@grep '^FAIL:' $(objpfx)tests.sum || true
> > +	@echo "Summary of test results:"
> > +	@sed 's/:.*//' < $(objpfx)tests.sum | sort | uniq -c
> > +	@if grep -q '^ERROR:' $(objpfx)tests.sum; then exit 1; fi
> >  xtests:
> >  	$(..)scripts/merge-test-results.sh -t $(objpfx) subdir-xtests.sum \
> >  	  $(sort $(subdirs)) \
> >  	  > $(objpfx)xtests.sum
> > +	@grep '^ERROR:' $(objpfx)xtests.sum || true
> > +	@grep '^FAIL:' $(objpfx)xtests.sum || true
> > +	@echo "Summary of test results for extra tests:"
> > +	@sed 's/:.*//' < $(objpfx)xtests.sum | sort | uniq -c
> > +	@if grep -q '^ERROR:' $(objpfx)xtests.sum; then exit 1; fi
> 
> OK. Split into another patch?

stop-on-test-failure=n can't work correctly in exit status terms without 
the "if grep".  And an exit with error status would be very confusing 
without some message immediately before giving the reason for the error.  
So I don't think it makes sense to add the variable without this logic 
that is needed for it to work reasonably.

> > +* Running the testsuite no longer terminates as soon as a test fails.
> > +  Instead, a file tests.sum (xtests.sum from "make xcheck") is generated,
> > +  with PASS or FAIL lines for individual tests.  A summary of the results is
> > +  printed, including a list of failing lists, and "make check" exits with
> > +  error status only if test results for a subdirectory are completely
> > +  missing, or if a test failed to compile.  "make check
> > +  stop-on-test-failure=y" may be used to keep the old behavior.
> > +
> 
> OK. Split into another patch?

If a patch does something worth mentioning in NEWS, it's best for the NEWS 
entry to go in as part of the same patch.

commit acd6e389122b39c1d4b37e739ef4e6676e706ce5
Author: Joseph Myers <joseph@codesourcery.com>
Date:   Thu Mar 13 15:01:15 2014 +0000

    Regenerate INSTALL.

Comments

Carlos O'Donell March 13, 2014, 4:54 p.m. UTC | #1
On 03/13/2014 11:07 AM, Joseph S. Myers wrote:
> On Thu, 13 Mar 2014, Carlos O'Donell wrote:
> 
>> I would prefer you split this into two patches, a minimal patch
>> to support running without `-k' and another for all the other
>> changes, but I'm not opposed to one patch for this change.
>> The first patch would change nothing, but add the variable,
>> and the second patch changes the default and includes the summary
>> changes.
> 
> I'm not clear on what you're proposing in light of Roland's and Brooks's 
> comments about exit status (which require reworking the documentation 
> parts of the patch anyway).

I reviewed this independent of Roland and Brooks' reviews. I leave it up
to you to decide what to do with the final merger of recommendations.

My suggestion is simply that you could split this into two patches, one
that adds the bits required to support running without terminating the
the test runs when one fails, and another to add the documentation and
change the default.

I suggest this only because I may wish to backport some of this to earlier
source that I support, but perhaps without the documentation or change in
defaults e.g. backport the functionality but don't expose it for users.

>>> diff --git a/INSTALL b/INSTALL
>>> index bfa692d..bcb53b8 100644
>>> --- a/INSTALL
>>> +++ b/INSTALL
>>> @@ -154,7 +154,7 @@ will be used, and CFLAGS sets optimization options for the compiler.
>>>  
>>>       If you only specify `--host', `configure' will prepare for a
>>>       native compile but use what you specify instead of guessing what
>>> -     your system is. This is most useful to change the CPU submodel.
>>> +     your system is.  This is most useful to change the CPU submodel.
>>
>> OK. Split into another patch?
> 
> I've committed the regeneration below as obvious.

Perfect.  Thanks.

>>> @@ -324,10 +324,20 @@ tests: $(tests-special)
>>>  	$(..)scripts/merge-test-results.sh -t $(objpfx) subdir-tests.sum \
>>>  	  $(sort $(subdirs) .) \
>>>  	  > $(objpfx)tests.sum
>>> +	@grep '^ERROR:' $(objpfx)tests.sum || true
>>> +	@grep '^FAIL:' $(objpfx)tests.sum || true
>>> +	@echo "Summary of test results:"
>>> +	@sed 's/:.*//' < $(objpfx)tests.sum | sort | uniq -c
>>> +	@if grep -q '^ERROR:' $(objpfx)tests.sum; then exit 1; fi
>>>  xtests:
>>>  	$(..)scripts/merge-test-results.sh -t $(objpfx) subdir-xtests.sum \
>>>  	  $(sort $(subdirs)) \
>>>  	  > $(objpfx)xtests.sum
>>> +	@grep '^ERROR:' $(objpfx)xtests.sum || true
>>> +	@grep '^FAIL:' $(objpfx)xtests.sum || true
>>> +	@echo "Summary of test results for extra tests:"
>>> +	@sed 's/:.*//' < $(objpfx)xtests.sum | sort | uniq -c
>>> +	@if grep -q '^ERROR:' $(objpfx)xtests.sum; then exit 1; fi
>>
>> OK. Split into another patch?
> 
> stop-on-test-failure=n can't work correctly in exit status terms without 
> the "if grep".  And an exit with error status would be very confusing 
> without some message immediately before giving the reason for the error.  
> So I don't think it makes sense to add the variable without this logic 
> that is needed for it to work reasonably.

That is a reasonable position to take, which is why I was asking if you
thought this should be split into a second patch.

>>> +* Running the testsuite no longer terminates as soon as a test fails.
>>> +  Instead, a file tests.sum (xtests.sum from "make xcheck") is generated,
>>> +  with PASS or FAIL lines for individual tests.  A summary of the results is
>>> +  printed, including a list of failing lists, and "make check" exits with
>>> +  error status only if test results for a subdirectory are completely
>>> +  missing, or if a test failed to compile.  "make check
>>> +  stop-on-test-failure=y" may be used to keep the old behavior.
>>> +
>>
>> OK. Split into another patch?
> 
> If a patch does something worth mentioning in NEWS, it's best for the NEWS 
> entry to go in as part of the same patch.

Agreed.

> commit acd6e389122b39c1d4b37e739ef4e6676e706ce5
> Author: Joseph Myers <joseph@codesourcery.com>
> Date:   Thu Mar 13 15:01:15 2014 +0000
> 
>     Regenerate INSTALL.
> 
> diff --git a/ChangeLog b/ChangeLog
> index 6f0a932..792fdb8 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,7 @@
> +2014-03-13  Joseph Myers  <joseph@codesourcery.com>
> +
> +	* INSTALL: Regenerated.
> +
>  2014-03-13  Will Newton  <will.newton@linaro.org>
>  
>  	* manual/setjmp.texi (System V contexts): Improve
> diff --git a/INSTALL b/INSTALL
> index bfa692d..f845940 100644
> --- a/INSTALL
> +++ b/INSTALL
> @@ -154,7 +154,7 @@ will be used, and CFLAGS sets optimization options for the compiler.
>  
>       If you only specify `--host', `configure' will prepare for a
>       native compile but use what you specify instead of guessing what
> -     your system is. This is most useful to change the CPU submodel.
> +     your system is.  This is most useful to change the CPU submodel.
>       For example, if `configure' guesses your machine as
>       `i686-pc-linux-gnu' but you want to compile a library for 586es,
>       give `--host=i586-pc-linux-gnu' or just `--host=i586-linux' and add
> 

Thanks.

Cheers,
Carlos.
diff mbox

Patch

diff --git a/ChangeLog b/ChangeLog
index 6f0a932..792fdb8 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@ 
+2014-03-13  Joseph Myers  <joseph@codesourcery.com>
+
+	* INSTALL: Regenerated.
+
 2014-03-13  Will Newton  <will.newton@linaro.org>
 
 	* manual/setjmp.texi (System V contexts): Improve
diff --git a/INSTALL b/INSTALL
index bfa692d..f845940 100644
--- a/INSTALL
+++ b/INSTALL
@@ -154,7 +154,7 @@  will be used, and CFLAGS sets optimization options for the compiler.
 
      If you only specify `--host', `configure' will prepare for a
      native compile but use what you specify instead of guessing what
-     your system is. This is most useful to change the CPU submodel.
+     your system is.  This is most useful to change the CPU submodel.
      For example, if `configure' guesses your machine as
      `i686-pc-linux-gnu' but you want to compile a library for 586es,
      give `--host=i586-pc-linux-gnu' or just `--host=i586-linux' and add