diff mbox

[1/2] tests: Resort check-qtest entries in Makefile.include

Message ID 1468490266-3468-2-git-send-email-thuth@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Thomas Huth July 14, 2016, 9:57 a.m. UTC
The rather random list of check-qtest-xxx entries caused some
confusion in the past, where to use "=" and where to use "+="
(see commits 0ccac16f59462b8e2b9afbc1 and 1f5c1cfbaec0792cd2e5da
for example).
Sorting the check-qtest-xxx entries by architecure instead and
using some empty lines inbetween should help to ease this
situation a little bit, so that it is hopefully now obvious
that new tests should be added with "+=" instead of "=".

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/Makefile.include | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

Comments

David Gibson July 15, 2016, 3:12 a.m. UTC | #1
On Thu, Jul 14, 2016 at 11:57:45AM +0200, Thomas Huth wrote:
> The rather random list of check-qtest-xxx entries caused some
> confusion in the past, where to use "=" and where to use "+="
> (see commits 0ccac16f59462b8e2b9afbc1 and 1f5c1cfbaec0792cd2e5da
> for example).
> Sorting the check-qtest-xxx entries by architecure instead and
> using some empty lines inbetween should help to ease this
> situation a little bit, so that it is hopefully now obvious
> that new tests should be added with "+=" instead of "=".
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/Makefile.include | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 2010b11..b7784d3 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -240,32 +240,40 @@ check-qtest-i386-y += tests/postcopy-test$(EXESUF)
>  check-qtest-x86_64-y += $(check-qtest-i386-y)
>  gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
>  gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
> +

Hm, seems to me like there's still potential for confusion in the
mixing of check-qtest lines and gcov-files lines.

>  check-qtest-mips-y = tests/endianness-test$(EXESUF)
>  check-qtest-mips64-y = tests/endianness-test$(EXESUF)
>  check-qtest-mips64el-y = tests/endianness-test$(EXESUF)
> +
>  check-qtest-ppc-y = tests/endianness-test$(EXESUF)
> +check-qtest-ppc-y += tests/boot-order-test$(EXESUF)
> +check-qtest-ppc-y += tests/prom-env-test$(EXESUF)
> +
>  check-qtest-ppc64-y = tests/endianness-test$(EXESUF)
> +check-qtest-ppc64-y += tests/boot-order-test$(EXESUF)
> +check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF)
> +gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c

For example, AFAICT this is the first line setting gcov-files-ppc64-y,
so shouldn't it be '=' instead of '+='?

> +check-qtest-ppc64-y += tests/prom-env-test$(EXESUF)
> +
>  check-qtest-sh4-y = tests/endianness-test$(EXESUF)
>  check-qtest-sh4eb-y = tests/endianness-test$(EXESUF)

Should be another blank line between sh4 and sh4eb, no?

> +
> +check-qtest-sparc-y = tests/prom-env-test$(EXESUF)
> +#check-qtest-sparc-y += tests/m48t59-test$(EXESUF)
> +#gcov-files-sparc-y = hw/timer/m48t59.c

You've added a comment here, haven't you?  Which changes something
real, not just order.

> +
>  check-qtest-sparc64-y = tests/endianness-test$(EXESUF)
> -#check-qtest-sparc-y = tests/m48t59-test$(EXESUF)
>  #check-qtest-sparc64-y += tests/m48t59-test$(EXESUF)
> -gcov-files-sparc-y += hw/timer/m48t59.c
> -gcov-files-sparc64-y += hw/timer/m48t59.c
> +#gcov-files-sparc64-y += hw/timer/m48t59.c

And here?

> +#Disabled for now, triggers a TCG bug on 32-bit hosts
> +#check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
> +
>  check-qtest-arm-y = tests/tmp105-test$(EXESUF)
>  check-qtest-arm-y += tests/ds1338-test$(EXESUF)
>  gcov-files-arm-y += hw/misc/tmp105.c
>  check-qtest-arm-y += tests/virtio-blk-test$(EXESUF)
>  gcov-files-arm-y += arm-softmmu/hw/block/virtio-blk.c
> -check-qtest-ppc-y += tests/boot-order-test$(EXESUF)
> -check-qtest-ppc64-y += tests/boot-order-test$(EXESUF)
> -check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF)
> -gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c
> -check-qtest-ppc-y += tests/prom-env-test$(EXESUF)
> -check-qtest-ppc64-y += tests/prom-env-test$(EXESUF)
> -check-qtest-sparc-y += tests/prom-env-test$(EXESUF)
> -#Disabled for now, triggers a TCG bug on 32-bit hosts
> -#check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
> +
>  check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
>  check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)

And there should be a blank betweem microblazeel and xtensaeb too, no?
Thomas Huth July 15, 2016, 6:46 a.m. UTC | #2
On 15.07.2016 05:12, David Gibson wrote:
> On Thu, Jul 14, 2016 at 11:57:45AM +0200, Thomas Huth wrote:
>> The rather random list of check-qtest-xxx entries caused some
>> confusion in the past, where to use "=" and where to use "+="
>> (see commits 0ccac16f59462b8e2b9afbc1 and 1f5c1cfbaec0792cd2e5da
>> for example).
>> Sorting the check-qtest-xxx entries by architecure instead and
>> using some empty lines inbetween should help to ease this
>> situation a little bit, so that it is hopefully now obvious
>> that new tests should be added with "+=" instead of "=".
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  tests/Makefile.include | 32 ++++++++++++++++++++------------
>>  1 file changed, 20 insertions(+), 12 deletions(-)
>>
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index 2010b11..b7784d3 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -240,32 +240,40 @@ check-qtest-i386-y += tests/postcopy-test$(EXESUF)
>>  check-qtest-x86_64-y += $(check-qtest-i386-y)
>>  gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
>>  gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
>> +
> 
> Hm, seems to me like there's still potential for confusion in the
> mixing of check-qtest lines and gcov-files lines.
> 
>>  check-qtest-mips-y = tests/endianness-test$(EXESUF)
>>  check-qtest-mips64-y = tests/endianness-test$(EXESUF)
>>  check-qtest-mips64el-y = tests/endianness-test$(EXESUF)
>> +
>>  check-qtest-ppc-y = tests/endianness-test$(EXESUF)
>> +check-qtest-ppc-y += tests/boot-order-test$(EXESUF)
>> +check-qtest-ppc-y += tests/prom-env-test$(EXESUF)
>> +
>>  check-qtest-ppc64-y = tests/endianness-test$(EXESUF)
>> +check-qtest-ppc64-y += tests/boot-order-test$(EXESUF)
>> +check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF)
>> +gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c
> 
> For example, AFAICT this is the first line setting gcov-files-ppc64-y,
> so shouldn't it be '=' instead of '+='?

True. It currently doesn't hurt because the variable is empty initially,
but it's likely cleaner to start with "=" here, too, for the first
entry... I'll change it in v2.

>> +check-qtest-ppc64-y += tests/prom-env-test$(EXESUF)
>> +
>>  check-qtest-sh4-y = tests/endianness-test$(EXESUF)
>>  check-qtest-sh4eb-y = tests/endianness-test$(EXESUF)
> 
> Should be another blank line between sh4 and sh4eb, no?

I didn't mind it here yet since there is currently only one test for
these architectures, but yes, it's likely cleaner to also add a space
here right from the start, too.

>> +
>> +check-qtest-sparc-y = tests/prom-env-test$(EXESUF)
>> +#check-qtest-sparc-y += tests/m48t59-test$(EXESUF)
>> +#gcov-files-sparc-y = hw/timer/m48t59.c
> 
> You've added a comment here, haven't you?  Which changes something
> real, not just order.

Yeah, I've commented it out since the test is disabled, ... I guess I
should either leave it uncommented or write a proper sentence about this
in the commit description...

>> +
>>  check-qtest-sparc64-y = tests/endianness-test$(EXESUF)
>> -#check-qtest-sparc-y = tests/m48t59-test$(EXESUF)
>>  #check-qtest-sparc64-y += tests/m48t59-test$(EXESUF)
>> -gcov-files-sparc-y += hw/timer/m48t59.c
>> -gcov-files-sparc64-y += hw/timer/m48t59.c
>> +#gcov-files-sparc64-y += hw/timer/m48t59.c
> 
> And here?
> 
>> +#Disabled for now, triggers a TCG bug on 32-bit hosts
>> +#check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
>> +
>>  check-qtest-arm-y = tests/tmp105-test$(EXESUF)
>>  check-qtest-arm-y += tests/ds1338-test$(EXESUF)
>>  gcov-files-arm-y += hw/misc/tmp105.c
>>  check-qtest-arm-y += tests/virtio-blk-test$(EXESUF)
>>  gcov-files-arm-y += arm-softmmu/hw/block/virtio-blk.c
>> -check-qtest-ppc-y += tests/boot-order-test$(EXESUF)
>> -check-qtest-ppc64-y += tests/boot-order-test$(EXESUF)
>> -check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF)
>> -gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c
>> -check-qtest-ppc-y += tests/prom-env-test$(EXESUF)
>> -check-qtest-ppc64-y += tests/prom-env-test$(EXESUF)
>> -check-qtest-sparc-y += tests/prom-env-test$(EXESUF)
>> -#Disabled for now, triggers a TCG bug on 32-bit hosts
>> -#check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
>> +
>>  check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
>>  check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
> 
> And there should be a blank betweem microblazeel and xtensaeb too, no?

Yes, I'll change in v2.

Thanks for the review!

 Thomas
David Gibson July 15, 2016, 7:14 a.m. UTC | #3
On Fri, Jul 15, 2016 at 08:46:47AM +0200, Thomas Huth wrote:
> On 15.07.2016 05:12, David Gibson wrote:
> > On Thu, Jul 14, 2016 at 11:57:45AM +0200, Thomas Huth wrote:
> >> The rather random list of check-qtest-xxx entries caused some
> >> confusion in the past, where to use "=" and where to use "+="
> >> (see commits 0ccac16f59462b8e2b9afbc1 and 1f5c1cfbaec0792cd2e5da
> >> for example).
> >> Sorting the check-qtest-xxx entries by architecure instead and
> >> using some empty lines inbetween should help to ease this
> >> situation a little bit, so that it is hopefully now obvious
> >> that new tests should be added with "+=" instead of "=".
> >>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >> ---
> >>  tests/Makefile.include | 32 ++++++++++++++++++++------------
> >>  1 file changed, 20 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/tests/Makefile.include b/tests/Makefile.include
> >> index 2010b11..b7784d3 100644
> >> --- a/tests/Makefile.include
> >> +++ b/tests/Makefile.include
> >> @@ -240,32 +240,40 @@ check-qtest-i386-y += tests/postcopy-test$(EXESUF)
> >>  check-qtest-x86_64-y += $(check-qtest-i386-y)
> >>  gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
> >>  gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
> >> +
> > 
> > Hm, seems to me like there's still potential for confusion in the
> > mixing of check-qtest lines and gcov-files lines.
> > 
> >>  check-qtest-mips-y = tests/endianness-test$(EXESUF)
> >>  check-qtest-mips64-y = tests/endianness-test$(EXESUF)
> >>  check-qtest-mips64el-y = tests/endianness-test$(EXESUF)
> >> +
> >>  check-qtest-ppc-y = tests/endianness-test$(EXESUF)
> >> +check-qtest-ppc-y += tests/boot-order-test$(EXESUF)
> >> +check-qtest-ppc-y += tests/prom-env-test$(EXESUF)
> >> +
> >>  check-qtest-ppc64-y = tests/endianness-test$(EXESUF)
> >> +check-qtest-ppc64-y += tests/boot-order-test$(EXESUF)
> >> +check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF)
> >> +gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c
> > 
> > For example, AFAICT this is the first line setting gcov-files-ppc64-y,
> > so shouldn't it be '=' instead of '+='?
> 
> True. It currently doesn't hurt because the variable is empty initially,
> but it's likely cleaner to start with "=" here, too, for the first
> entry... I'll change it in v2.

Hm, given that += on a previously undeclared variable treats it as
empty, it seems like it would be safer to just use += everywhere.  I
don't believe there are any cases where we're trying to remove tests

> 
> >> +check-qtest-ppc64-y += tests/prom-env-test$(EXESUF)
> >> +
> >>  check-qtest-sh4-y = tests/endianness-test$(EXESUF)
> >>  check-qtest-sh4eb-y = tests/endianness-test$(EXESUF)
> > 
> > Should be another blank line between sh4 and sh4eb, no?
> 
> I didn't mind it here yet since there is currently only one test for
> these architectures, but yes, it's likely cleaner to also add a space
> here right from the start, too.
> 
> >> +
> >> +check-qtest-sparc-y = tests/prom-env-test$(EXESUF)
> >> +#check-qtest-sparc-y += tests/m48t59-test$(EXESUF)
> >> +#gcov-files-sparc-y = hw/timer/m48t59.c
> > 
> > You've added a comment here, haven't you?  Which changes something
> > real, not just order.
> 
> Yeah, I've commented it out since the test is disabled, ... I guess I
> should either leave it uncommented or write a proper sentence about this
> in the commit description...

Ah.. yes, I see.  I hadn't connected the coverage to the fact the
corresponding test is disabled.  Yeah, I think that needs a mention in
the description.

> 
> >> +
> >>  check-qtest-sparc64-y = tests/endianness-test$(EXESUF)
> >> -#check-qtest-sparc-y = tests/m48t59-test$(EXESUF)
> >>  #check-qtest-sparc64-y += tests/m48t59-test$(EXESUF)
> >> -gcov-files-sparc-y += hw/timer/m48t59.c
> >> -gcov-files-sparc64-y += hw/timer/m48t59.c
> >> +#gcov-files-sparc64-y += hw/timer/m48t59.c
> > 
> > And here?
> > 
> >> +#Disabled for now, triggers a TCG bug on 32-bit hosts
> >> +#check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
> >> +
> >>  check-qtest-arm-y = tests/tmp105-test$(EXESUF)
> >>  check-qtest-arm-y += tests/ds1338-test$(EXESUF)
> >>  gcov-files-arm-y += hw/misc/tmp105.c
> >>  check-qtest-arm-y += tests/virtio-blk-test$(EXESUF)
> >>  gcov-files-arm-y += arm-softmmu/hw/block/virtio-blk.c
> >> -check-qtest-ppc-y += tests/boot-order-test$(EXESUF)
> >> -check-qtest-ppc64-y += tests/boot-order-test$(EXESUF)
> >> -check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF)
> >> -gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c
> >> -check-qtest-ppc-y += tests/prom-env-test$(EXESUF)
> >> -check-qtest-ppc64-y += tests/prom-env-test$(EXESUF)
> >> -check-qtest-sparc-y += tests/prom-env-test$(EXESUF)
> >> -#Disabled for now, triggers a TCG bug on 32-bit hosts
> >> -#check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
> >> +
> >>  check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
> >>  check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
> > 
> > And there should be a blank betweem microblazeel and xtensaeb too, no?
> 
> Yes, I'll change in v2.
> 
> Thanks for the review!
> 
>  Thomas
> 
> 
>
diff mbox

Patch

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 2010b11..b7784d3 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -240,32 +240,40 @@  check-qtest-i386-y += tests/postcopy-test$(EXESUF)
 check-qtest-x86_64-y += $(check-qtest-i386-y)
 gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
 gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
+
 check-qtest-mips-y = tests/endianness-test$(EXESUF)
 check-qtest-mips64-y = tests/endianness-test$(EXESUF)
 check-qtest-mips64el-y = tests/endianness-test$(EXESUF)
+
 check-qtest-ppc-y = tests/endianness-test$(EXESUF)
+check-qtest-ppc-y += tests/boot-order-test$(EXESUF)
+check-qtest-ppc-y += tests/prom-env-test$(EXESUF)
+
 check-qtest-ppc64-y = tests/endianness-test$(EXESUF)
+check-qtest-ppc64-y += tests/boot-order-test$(EXESUF)
+check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF)
+gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c
+check-qtest-ppc64-y += tests/prom-env-test$(EXESUF)
+
 check-qtest-sh4-y = tests/endianness-test$(EXESUF)
 check-qtest-sh4eb-y = tests/endianness-test$(EXESUF)
+
+check-qtest-sparc-y = tests/prom-env-test$(EXESUF)
+#check-qtest-sparc-y += tests/m48t59-test$(EXESUF)
+#gcov-files-sparc-y = hw/timer/m48t59.c
+
 check-qtest-sparc64-y = tests/endianness-test$(EXESUF)
-#check-qtest-sparc-y = tests/m48t59-test$(EXESUF)
 #check-qtest-sparc64-y += tests/m48t59-test$(EXESUF)
-gcov-files-sparc-y += hw/timer/m48t59.c
-gcov-files-sparc64-y += hw/timer/m48t59.c
+#gcov-files-sparc64-y += hw/timer/m48t59.c
+#Disabled for now, triggers a TCG bug on 32-bit hosts
+#check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
+
 check-qtest-arm-y = tests/tmp105-test$(EXESUF)
 check-qtest-arm-y += tests/ds1338-test$(EXESUF)
 gcov-files-arm-y += hw/misc/tmp105.c
 check-qtest-arm-y += tests/virtio-blk-test$(EXESUF)
 gcov-files-arm-y += arm-softmmu/hw/block/virtio-blk.c
-check-qtest-ppc-y += tests/boot-order-test$(EXESUF)
-check-qtest-ppc64-y += tests/boot-order-test$(EXESUF)
-check-qtest-ppc64-y += tests/spapr-phb-test$(EXESUF)
-gcov-files-ppc64-y += ppc64-softmmu/hw/ppc/spapr_pci.c
-check-qtest-ppc-y += tests/prom-env-test$(EXESUF)
-check-qtest-ppc64-y += tests/prom-env-test$(EXESUF)
-check-qtest-sparc-y += tests/prom-env-test$(EXESUF)
-#Disabled for now, triggers a TCG bug on 32-bit hosts
-#check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
+
 check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
 check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)