Make sure the migration summary page has been download completely

[tinawang123]

Make sure the migration summary page has been download completely

• Related ticket: https://progress.opensuse.org/issues/64929
• Verification run:
  https://openqa.nue.suse.com/tests/4122657
  https://openqa.nue.suse.com/tests/4122992#

[lemon-suse]

LGTM。

[okurz]

I must admit this is quite unusual test code. Have you considered an approach similar to https://github.com/os-autoinst/os-autoinst-distri-opensuse/blob/master/tests/installation/installation_overview.pm#L98 ? In particular, make sure that the needles do not only look for the outer frame on the page but that the content is already completely displayed.

[tinawang123]

tent is already completely displayed.

We cannot just extend time. Let me explain my code. For first 'wait_screen_change', I need make sure the migration-target page can show up completely. As it only can show migration target but the migration summary part cannot show up in time, it cannot match the migration-target needle. Please check the failed case: https://openqa.nue.suse.com/tests/4112276#step/yast2_migration/30
For the second 'wait_screen_change', After check migration-target page, send key 'alt-n', it cannot work sometimes. So if the screen not change after send key, I would like to send again. Please check the failed case: https://openqa.nue.suse.com/tests/4112263#step/yast2_migration/22

[okurz]

I did not recommend to extend time. Actually my suggestion can save time. I'm not sure, did you check the code link I provided?

[tinawang123]

I did not recommend to extend time. Actually my suggestion can save time. I'm not sure, did you check the code link I provided?

I have checked the code link, as if it cannot match the first needle, it need send 'down' key. If use assert_screen, it will fail but not send key. I think we need wait migration summary show completely, then check needle

[okurz]

Ok, to make sure we understand, sorry if I am blunt: The current code already looks messy and I understand what you want to fix but the code you introduce looks even more convoluted and very tricky to understand in the future. Please try to write test instructions for a procedure that a human operator of the UI would also follow. Then I am sure we can also fix the code to be reliable but also clean to read. In the example I gave you the test code for the installation summary page is waiting not only for the outer frame to show but also for the inner content to be correctly displayed before trying to continue. One important point is that you only try to patch the behaviour after the test code asserts the 'yast2-migration-target' but the needle does not check that the page is already completely rendered. So what the needle should check is that not only the title is there but also the content and the buttons to continue show up. Probably it is enough to cover the "Next" button in another needle section. Then afterwards send_key_until_needlematch 'migration-target-' . get_var("VERSION"), 'down', 20; must be enough, maybe with a second parameter, e.g. send_key_until_needlematch '…', 'down', 20, 3; using a timeout for the internal check_screen of "3 s" vs. the default "1 s".

You should keep in mind: If you think you need to come up with very complicated test code and you have checked all other options of the test API then maybe the test API of os-autoinst needs to be extended. The test code should still stay maintainable for long times, not just be hotpatched to run the current products :)

[tinawang123]

I changed code with send_key_until_needlematch 'migration-target-' . get_var("VERSION"), 'down', 20, 3; Please check https://openqa.nue.suse.com/tests/4138957#step/yast2_migration/58 I think I still need wait_screen_change before 'send_key_until_needlematch'. I need make sure the migration summary page has been download completely.

[tinawang123]

My needle check includes 'migration target', 'migration summary' and 'next' button

[tinawang123]

Verified: https://openqa.nue.suse.com/tests/4143018#step/yast2_migration/21

[okurz]

Please see my comment about this. Make it at least wait_screen_change { send_key 'alt-p' }, 20;
But probably better, faster and more stable to replace with an assert_screen

[tinawang123]

I have used 'wait_screen_change(sub { send_key "alt-p" }, 20);', it same as wait_screen_change { send_key 'alt-p' }, 20;. I think assert_screen is not a good idea, I need wait screen show completely at here. And I have verified in this way. It can work now. I hope it still can work well when merged.

[okurz]

Of course. It is just that wait_screen_change { send_key 'alt-p' }, 20; is the common style and the software in this project should follow a common style.

But nevertheless, the assert_screen is ensuring that the screen shows completely here assuming that you design the needle accordingly. You are saying "it is not a good idea" but without a good reason I am convinced assert_screen is better here

[tinawang123]

If use assert_screen, I need delete all ''yast2-migration-target'' needles and add new needle for this one. I'm worried it will impact some other cases. In this way, I just need change code, after send key wait screen change. It has the same result as the delete needles but less effort. It can save many time.

[okurz]

No, it wastes test time as wait_screen_change has a timeout which introduces waiting time in every test run. You are being too careful :)

I did not ask to delete any existing needles but to include an additional assert_screen. And even if you want to change existing needles we have good tools to also show where which needle is used