Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix race condition in assert_and_click_until_screen_change #4397

Conversation

StefanBruens
Copy link
Contributor

Whenever the release causes multiple screen changes (e.g. "change in
button appearance" and "new screen after event", the check_screen may
misdetect a successful click.

A screen change is signaled by the wait_screen_change return code, which
can be used to detect events caused by the click.

See poo#31327

Signed-off-by: Stefan Brüns stefan.bruens@rwth-aachen.de

@okurz
Copy link
Member

okurz commented Feb 11, 2018

Sounds reasonable but changes like these can have a bigger impact and failures are hard to relate to that change. I strongly recommend to have multiple verification runs here. But of course anyone could do this for this PR, e.g. when you don't have a machine available for that.

Whenever the release causes multiple screen changes (e.g. "change in
button appearance" and "new screen after event", the check_screen may
misdetect a successful click.

A screen change is signaled by the wait_screen_change return code, which
can be used to detect events caused by the click.

See poo#31327

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
@StefanBruens StefanBruens force-pushed the fix_race_in_assert_and_click_until_screen_change branch from eeabc9c to 1ef7e62 Compare February 11, 2018 16:06
@StefanBruens
Copy link
Contributor Author

@okurz I would appreciate if you would do that (or poke someone else ...)

@okurz
Copy link
Member

okurz commented Feb 13, 2018

That actually is a very convincing argument ;) On it.

http://lord.arch/tests/527

@okurz
Copy link
Member

okurz commented Feb 13, 2018

Conducted more than ten runs, all fine, e.g. http://lord.arch/tests/540

@okurz okurz merged commit 1c0b5c8 into os-autoinst:master Feb 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants