[WebDriver] Properly report error: "No such window"#37385
Conversation
Signed-off-by: Euclid Ye <[email protected]>
Signed-off-by: Euclid Ye <[email protected]>
|
cc @xiaochengh @jdm |
yezhizhen
changed the title
Signed-off-by: Euclid Ye <[email protected]>
Signed-off-by: Euclid Ye <[email protected]>
longvatrong111
left a comment
longvatrong111
left a comment
There was a problem hiding this comment.
Look good to me with some suggestions.
In addition, we will check webview still open with embedder soon since we move webdriver to servoshell, but not a big problem.
| ) -> WebDriverResult<()> { | ||
| let browsing_context_id = self.session()?.browsing_context_id; | ||
| self.verify_browsing_context_is_open(browsing_context_id)?; | ||
| let msg = EmbedderToConstellationMessage::WebDriverCommand( |
There was a problem hiding this comment.
verify_browsing_context_is_open here makes webdriver check everytime it sends a script message.
There are scenarios in which webdriver sends multiple script message, we only need to verify once, outside of this function.
There was a problem hiding this comment.
Thanks! There are many clean-ups later, this is definitely one of them.
| WebDriverCommandMsg::CheckWebViewOpen(webview_id, sender), | ||
| )) | ||
| .unwrap(); | ||
| if !receiver.recv().unwrap_or(false) { |
There was a problem hiding this comment.
Can consider using try_recv and reuse wait_for_script_response here. Benefit are:
- Can handle error like ipc channel closed.
- Avoid blocking and timeout in test.
There was a problem hiding this comment.
Can consider using try_recv
I don't think try_recv is correct here. Perhaps you mean recv_timeout?
reuse wait_for_script_response
Sure! I just further checked, this won't simplify anything.
|
@jdm after merging this PR, is webdriver test result good enough for us to enable it in the CI? |
Pull Request is not mergeable
|
@longvatrong111 I doubt it. Every intermittent failure that I saw was a panic in the script thread trying to fetch browsing context information from the constellation. You can run some try jobs with the tests enabled in your fork if you want to experiment, though! |
3 participants
For WebDriver, return "No Such Window" properly according to spec.
Testing:
./mach test-wpt -r --log-raw "D:\servo test log\all.txt" .\tests\wpt\tests\webdriver\tests\classic\ --product servodriver