Re: [Qemu-devel] [PATCH] tests: Remove (mostly) useless architecture checks

[John Snow <jsnow@redhat.com>]

On 3/3/19 9:15 AM, Thomas Huth wrote:
> On 01/03/2019 18.57, John Snow wrote:
>>
>>
>> On 3/1/19 11:16 AM, Thomas Huth wrote:
>>> These checks at the beginning of some of the tests are mostly useless:
>>> We only run the tests on x86 anyway, and g_test_message() does not
>>> print anything unless you call g_test_init() first.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>  tests/fdc-test.c      | 7 -------
>>>  tests/ide-test.c      | 7 -------
>>>  tests/ipmi-bt-test.c  | 7 -------
>>>  tests/ipmi-kcs-test.c | 7 -------
>>>  4 files changed, 28 deletions(-)
>>>
>>> diff --git a/tests/fdc-test.c b/tests/fdc-test.c
>>> index 88f1abf..31cd329 100644
>>> --- a/tests/fdc-test.c
>>> +++ b/tests/fdc-test.c
>>> @@ -548,16 +548,9 @@ static void fuzz_registers(void)
>>>  
>>>  int main(int argc, char **argv)
>>>  {
>>> -    const char *arch = qtest_get_arch();
>>>      int fd;
>>>      int ret;
>>>  
>>> -    /* Check architecture */
>>> -    if (strcmp(arch, "i386") && strcmp(arch, "x86_64")) {
>>> -        g_test_message("Skipping test for non-x86\n");
>>> -        return 0;
>>> -    }
>>> -
>>>      /* Create a temporary raw image */
>>>      fd = mkstemp(test_image);
>>>      g_assert(fd >= 0);
>>> diff --git a/tests/ide-test.c b/tests/ide-test.c
>>> index f0280e6..300d64e 100644
>>> --- a/tests/ide-test.c
>>> +++ b/tests/ide-test.c
>>> @@ -1009,16 +1009,9 @@ static void test_cdrom_dma(void)
>>>  
>>>  int main(int argc, char **argv)
>>>  {
>>> -    const char *arch = qtest_get_arch();
>>>      int fd;
>>>      int ret;
>>>  
>>> -    /* Check architecture */
>>> -    if (strcmp(arch, "i386") && strcmp(arch, "x86_64")) {
>>> -        g_test_message("Skipping test for non-x86\n");
>>> -        return 0;
>>> -    }
>>> -
>>>      /* Create temporary blkdebug instructions */
>>>      fd = mkstemp(debug_path);
>>>      g_assert(fd >= 0);
>>> diff --git a/tests/ipmi-bt-test.c b/tests/ipmi-bt-test.c
>>> index f4a81b5..fc4c83b 100644
>>> --- a/tests/ipmi-bt-test.c
>>> +++ b/tests/ipmi-bt-test.c
>>> @@ -400,15 +400,8 @@ static void open_socket(void)
>>>  
>>>  int main(int argc, char **argv)
>>>  {
>>> -    const char *arch = qtest_get_arch();
>>>      int ret;
>>>  
>>> -    /* Check architecture */
>>> -    if (strcmp(arch, "i386") && strcmp(arch, "x86_64")) {
>>> -        g_test_message("Skipping test for non-x86\n");
>>> -        return 0;
>>> -    }
>>> -
>>>      open_socket();
>>>  
>>>      /* Run the tests */
>>> diff --git a/tests/ipmi-kcs-test.c b/tests/ipmi-kcs-test.c
>>> index 178ffc1..a2354c1 100644
>>> --- a/tests/ipmi-kcs-test.c
>>> +++ b/tests/ipmi-kcs-test.c
>>> @@ -263,16 +263,9 @@ static void test_enable_irq(void)
>>>  
>>>  int main(int argc, char **argv)
>>>  {
>>> -    const char *arch = qtest_get_arch();
>>>      char *cmdline;
>>>      int ret;
>>>  
>>> -    /* Check architecture */
>>> -    if (strcmp(arch, "i386") && strcmp(arch, "x86_64")) {
>>> -        g_test_message("Skipping test for non-x86\n");
>>> -        return 0;
>>> -    }
>>> -
>>>      /* Run the tests */
>>>      g_test_init(&argc, &argv, NULL);
>>>  
>>>
>>
>> Hm, if you insist. I have no strong feelings... Do we plan to split
>> tests out by architecture eventually? Clearly x86 only tests don't
>> really need to each individually check the arch, but I'm not sure what
>> the vision is.
> 
> We could also fix the g_test_message() output by moving it after the
> g_test_init() ... I don't mind too much which way we go, but at least
> the current state is bad.
> 
> Looking at other tests, we seem to be pretty inconsistent in checking
> the architecture at the beginning. For example q35-test.c,
> pvpanic-test.c and test-x86-cpuid.c do not check for x86, while
> rtas-test.c has a check for ppc64...
> 
>> Either way, since I have no horse in the race:
>>
>> Acked-by: John Snow <jsnow@redhat.com>
> 
> Thanks!
> 
>  Thomas
> 

In that case, might as well be consistent first and we can refactor our
test suite when we have a reason to want to do that.

Thanks for the explanation!

--js