1790699 - BiDi wdspec tests should await on asyncio.sleep

Status: RESOLVED FIXED

(Remote Protocol :: Agent, task, P1)

Description

[Julian Descottes [:jdescottes]]

We are missing await on many asyncio.sleep calls in BiDi wdspec tests:

https://searchfox.org/mozilla-central/search?q=asyncio.sleep&path=bidi&case=false&regexp=false

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Does that have any huge consequences eg. testing invalid states? I assume we should get this fixed quickly? Seems to be a quick thing to do.

Comment 2

[Julian Descottes [:jdescottes]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #1)

Does that have any huge consequences eg. testing invalid states? I assume we should get this fixed quickly? Seems to be a quick thing to do.

It's making some "safety" asserts useless an redundant. It's always in situations where we want to check that we don't get extra events, after unsubscribing for instance.

We could have missed regressions because of that, but it's unlikely. Those asserts are usually safety asserts, used in a sequence such as:

• perform an action that could trigger an event
• check that we didn't receive the event
• wait for 0.5 seconds
• check again that we didn't receive the event

We are never sure that the first check would catch regressions, that's why we have the second one, in case the event gets emitted asynchronously.

But other than this yes it's a very simple fix, 1 point material. Only downside is that it will make those tests a bit slower.

Comment 3

[Julian Descottes [:jdescottes]]

let's address this, it's a tiny issue.

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1790699 - [wdspec] bidi tests should always await on asyncio.sleep

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d5f5979479b
[wdspec] bidi tests should always await on asyncio.sleep r=webdriver-reviewers,whimboo

Comment 6

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/35969 for changes under testing/web-platform/tests

Comment 7

[Andreea Pavel [:apavel]]

Backed out for SM bustage at PerfSpewer.cpp
Push with failure: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=2d5f5979479b19e009548aa52cc59bd6d36c6280&selectedTaskRun=euIKe88aQ8-_ewJYQBl7Ug.0
Failure log: https://treeherder.mozilla.org/logviewer?job_id=390943088&repo=autoland&lineNumber=29821
Backout: https://hg.mozilla.org/integration/autoland/rev/2b89c5d345a812e6e9b37ac4db84603474645c2d

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4bb8be194099
[wdspec] bidi tests should always await on asyncio.sleep CLOSED TREE

Comment 9

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 10

[Andreea Pavel [:apavel]]

(In reply to Andreea Pavel [:apavel] from comment #7)

Backed out for SM bustage at PerfSpewer.cpp
Push with failure: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=2d5f5979479b19e009548aa52cc59bd6d36c6280&selectedTaskRun=euIKe88aQ8-_ewJYQBl7Ug.0
Failure log: https://treeherder.mozilla.org/logviewer?job_id=390943088&repo=autoland&lineNumber=29821
Backout: https://hg.mozilla.org/integration/autoland/rev/2b89c5d345a812e6e9b37ac4db84603474645c2d

Hi Julian, I apologize for the confusion, I relanded your changes here

Comment 11

[Julian Descottes [:jdescottes]]

No worries, thanks for re-landing Andreea!

Comment 12

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/4bb8be194099

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by jgraham