Re: [PATCH v5 09/12] tests/acceptance: Don't test reboot on cubieboard

[John Snow]

On 7/13/20 5:56 AM, Philippe Mathieu-Daudé wrote:
> On 7/10/20 7:06 AM, John Snow wrote:
>> cubieboard does not have a functioning reboot, it halts and QEMU does
>> not exit.
>>
>> vm.shutdown() is modified in a forthcoming patch that makes it less tolerant
>> of race conditions on shutdown; tests should consciously decide to WAIT
>> or to SHUTDOWN qemu.
>>
>> So long as this test is attempting to reboot, the correct choice would
>> be to WAIT for the VM to exit. However, since that's broken, we should
>> SHUTDOWN instead.
>>
>> SHUTDOWN is indeed what already happens when the test performs teardown,
>> however, if anyone fixes cubieboard reboot in the future, this test will
>> develop a new race condition that might be hard to debug.
>>
>> Therefore: remove the reboot test and make it obvious that the VM is
>> still running when the test concludes, where the test teardown will do
>> the right thing.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  tests/acceptance/boot_linux_console.py | 8 ++------
>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/tests/acceptance/boot_linux_console.py 
>> b/tests/acceptance/boot_linux_console.py
>> index 5867ef760c..8b8b828bc5 100644
>> --- a/tests/acceptance/boot_linux_console.py
>> +++ b/tests/acceptance/boot_linux_console.py
>> @@ -508,9 +508,7 @@ def test_arm_cubieboard_initrd(self):
>>                                                  'Allwinner sun4i/sun5i')
>>          exec_command_and_wait_for_pattern(self, 'cat /proc/iomem',
>>                                                  'system-control@1c00000')
>> -        exec_command_and_wait_for_pattern(self, 'reboot',
>> -                                                'reboot: Restarting system')
>> -        # NB: Do not issue vm.wait() here, cubieboard's reboot does not 
>> exit!
>> +        # cubieboard's reboot is not functioning; omit reboot test.
>>  
>>      def test_arm_cubieboard_sata(self):
>>          """
>> @@ -553,9 +551,7 @@ def test_arm_cubieboard_sata(self):
>>                                                  'Allwinner sun4i/sun5i')
>>          exec_command_and_wait_for_pattern(self, 'cat /proc/partitions',
>>                                                  'sda')
>> -        exec_command_and_wait_for_pattern(self, 'reboot',
>> -                                                'reboot: Restarting system')
>> -        # NB: Do not issue vm.wait() here, cubieboard's reboot does not 
>> exit!
>> +        # cubieboard's reboot is not functioning; omit reboot test.
>>  
>>      def test_arm_orangepi(self):
>>          """
>>
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Note, if I do the pull request, I might reorder this one before the
> previous one "tests/acceptance: wait() instead of shutdown() where
> appropriate".
> 

you could -- in practice it didn't seem to matter. I tested both with
and without this patch.

I was just trying to isolate each intentional semantic change as its own
commit so it could be observed/understood/debated.

--js