-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
apps: Fix crash with nonexisting /etc/os-release #19416
Conversation
Explicitly check that the metapackage gets installed. This makes it easier to tell apart mere "too short timeout" failures from persistent ones.
m.write("/etc/os-release", 'ID="derivative"\nID_LIKE="spicy testy"\nVERSION_ID="1"\n') | ||
b.click("#refresh") | ||
with b.wait_timeout(30): | ||
b.wait_visible(".app-list #app-1") | ||
m.execute("until test -e /usr/share/app-info/xmls/test.xml; do sleep 1; done") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about using b.wait
for the loop? Then we can control the timeout in the usual way and don't need to wait for the global timeout when this fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m.execute() has a default timeout of 2 minutes; it can be changed with timeout=
, but the default seemed quite appropriate to me. We often use this pattern in our tests, see git grep -E 'execute.*(while|until)'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and wait()
(not b.wait()
!) is more expensive, as it creates new SSH connections for each iteration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m.execute() has a default timeout of 2 minutes
Ah, I didn't think of that!
Broken out from PR #19281, as this is an embarassing crash/regression.