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

[Philippe Mathieu-Daudé]

On 7/13/20 5:12 PM, John Snow wrote:
> 
> 
> 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.

As both patches are correct, there is no need to debate IMO :)
I'm fine either way. The simpler the easier, and here the simpler
is to take your series as it.