Re: [PATCH 0/N] test-suite improvement - PASS/FAIL: initial patch- fix

[Tomas Dohnalek <tdohnale at redhat dot com>]

Hi Marek,
thank you for your input.

Dne 10.9.2012 22:21, Marek Polacek napsal(a):
> On Mon, Sep 10, 2012 at 01:04:48PM -0400, Tomas Dohnalek wrote:
> > -do-tests-clean:
> > +tests-summary = $(common-objpfx)tests.sum
> > +tests-summary-clean:
> > +	rm -f $(tests-summary)
> > +
> You probably want to use `-rm' here.

True.

> > diff --git a/Rules b/Rules
> > index 17d938e..5a569dd 100644
> > --- a/Rules
> > +++ b/Rules
> > @@ -127,6 +127,8 @@ binaries-shared-tests = $(filter-out $(binaries-pie) $(binaries-static), \
> >   				     $(binaries-all-tests))
> >   binaries-shared-notests = $(filter-out $(binaries-pie) $(binaries-static), \
> >   				       $(binaries-all-notests))
> > +test-name = $(subdir)/$(*F)
> Can't we use just $(@F) here?

That will produce "PASS: tst-empty.out" instead of nicer and more explaining "PASS: csu/tst-empty". 
I think, that there is a need to distinguish tests from different directories.

> > +test_rc=$1
> > +test_name=$2
> > +
> > +if [ $# -gt 2 ]; then
> > +    output=$3
> > +else
> > +    output=/dev/stdout
> > +fi
> Is this hunk really needed?  That is, couldn't we just drop this and
> then in Makefiles just do
> $(evaluate-test) > $(tests-summary)
> Anyway, it looks like we always output into $(tests-summary)...

Ok, it is more straightforward, I tried to write it more universally, but there probably wont be no 
need for that functionality.

> > +
> > +if [ ${test_rc} -eq 0 ]; then
> > +    result="PASS"
> > +else
> > +    result="FAIL"
> > +fi
> Use 2 spaces.  In the future we probably will want to have
> SKIP/UNSUPPORTED option too, I suspect.
>
> > +echo "${result}: ${test_name}" >> ${output}
> > +exit ${test_rc}
> There's no need to use ${var}.  Just $var is good enough.  Thanks,

I prefer to use it this way, but if $var is glibc convention, I have no problem with that.

> Furthermore, I'm not sure about parallel make...

True, but Joseph S. Myers proposal seems to be dealing with parallelism, I will make new patch.

> 	Marek

Thanks
Tomas

--
Tomas Dohnalek
QE Tools Intern
Red Hat