[wdspec] Improve tests for "stale element reference" error states
Categories
(Testing :: geckodriver, defect, P1)
Tracking
(firefox103 fixed)
| Tracking | Status | |
|---|---|---|
| firefox103 | --- | fixed |
People
(Reporter: whimboo, Assigned: whimboo)
References
Details
(Whiteboard: [webdriver:m4], [wptsync upstream])
Attachments
(4 files)
The following wdspec tests around cross-origin navigation would fail starting with my changes on bug 1692468 because they inappropriately assume a Stale Element after such a navigation.
- /webdriver/tests/back/back.py
- /webdriver/tests/forward/forward.py
- /webdriver/tests/navigate_to/navigate.py
After checking the WebDriver specification the following sections more or less make it clear that the current expectation is wrong and that both Firefox and Chrome inappropriately raise stale element error instead of a no such element error:
Each browsing context has an associated list of known elements. When the browsing context is discarded, the list of known elements is discarded along with it.
When having a cross-group navigation this is certainly the case here. The browsing context gets replaced and as such all known elements should be gone.
And here the specification explains how to get a known element and how to fail if it cannot be found:
Let element be the item in the current browsing context’s list of known elements for which the web element reference is equal to reference, if such an element exists. Otherwise return error with error code no such element.
As such my position is that the related tests need to be updated to correct check for a no such element error instead.
Adding to triage so that we can discuss it with the team next Tuesday.
|
Assignee | |
Comment 1•3 years ago
|
||
As discussed with James on Element the current WebDriver classic specification is actually referring to the WindowProxy and not the inner window, which is tight to the JSWindowActor pair. Ideally we would like to have it per inner window but there is an ongoing discussion on https://github.com/w3c/webdriver/issues/1594.
Nevertheless for these specific tests the behavior should be indeed broken and would require a fix. Everything else regarding the refactoring should be moved to bug 1692468.
Maybe we can get these tests fixed without the refactoring.
|
|
Assignee | |
Comment 2•3 years ago
|
||
We can indeed get the tests fixed without the refactoring on bug 1692468. As such I'm reversing the order of dependency.
Nevertheless I'm not just going to change these three tests but also improve the stale element reference and no such element checks for various WebDriver commands as well. This will drastically reduce the risk of any kind of regression when making the change in Marionette.
As agreed on we are moving this bug into M4 with 3 points given the amount of new tests to be added.
|
|
Assignee | |
Comment 3•3 years ago
|
||
While creating the tests I noticed a bug in the WebDriver classic specification around get known element and get known connected element. I'm going to fix it via https://github.com/w3c/webdriver/pull/1663.
|
|
Assignee | |
Comment 4•3 years ago
|
||
The number of changes that I'm working on are more than just for these 3 commands. Nearly all commands miss a proper "stale element reference" and "no such element" test. While getting these added I also found some more glitches in Marionette.
In having all these extra tests gives me a higher confidence that moving our element storage from the parent to the content process without none regressions or if then for a really rare case.
|
|
Assignee | |
Comment 5•3 years ago
|
||
As turned out the test changes for no such element will cause perma failures for non-Fission builds in CI which would be acceptable given that this is not our default configuration that gets shipped to users. But I can also see intermittent failures for some of the wdspec tests when Fission is enabled.
https://treeherder.mozilla.org/jobs?repo=try&revision=6974b22cf435e56eb6a3714de29d1327338c017c
Given that this bug was first for inappropriate "stale element reference" only and that for cross-navigation tests I would propose that we keep it for that and move anything else that adds all the new no such element checks to a follow-up bug. Most likely that follow-up one should be worked once the element store has been moved to the child process (bug 1692468).
I've pushed a new try build which hopefully should be green now:
https://treeherder.mozilla.org/jobs?repo=try&revision=51f1337afcee91e960b9307efb00dfbe414aa577
|
|
Assignee | |
Comment 6•3 years ago
|
||
As it turned out the fix for the cross-origin navigation tests we cannot do right now given that it would also start failing for Chrome. And the change would also require a WebDriver class spec change first. As such this bug should only cover the added tests and small fixes for stale element references. Given the amount of work/changes the 3 points should still be fine.
|
|
Assignee | |
Comment 7•3 years ago
|
||
|
|
Assignee | |
Comment 8•3 years ago
|
||
Depends on D149265
|
|
Assignee | |
Comment 9•3 years ago
|
||
Depends on D149266
|
|
Assignee | |
Comment 10•3 years ago
|
||
Depends on D149267
|
||
Comment 11•3 years ago
|
||
|
||
Comment 13•3 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/da94b60da49c
https://hg.mozilla.org/mozilla-central/rev/e6568a46cece
https://hg.mozilla.org/mozilla-central/rev/7acb206e3cfa
https://hg.mozilla.org/mozilla-central/rev/74257730a058
Description
•