Closed Bug 1772484 Opened 8 months ago Closed 8 months ago

[wdspec] Improve tests for "stale element reference" error states

Categories

(Testing :: geckodriver, defect, P1)

Default
defect
Points:
3

Tracking

(firefox103 fixed)

RESOLVED FIXED
103 Branch
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:

Section 12 - Elements:

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.

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.

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: nobody → hskupin
Blocks: 1692468
Status: NEW → ASSIGNED
Points: --- → 3
No longer depends on: 1692468
Priority: -- → P1
Whiteboard: [webdriver:triage] → [webdriver:m4]

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.

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.

Summary: [wdspec] Cross-origin navigation tests incorrectly assume stale element references → [wdspec] Add and improve tests for "stale element reference" and "no such element" error states

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

Summary: [wdspec] Add and improve tests for "stale element reference" and "no such element" error states → [wdspec] Improve tests for "stale element reference" error states and fix cross-navigation navigation tests to expect "no such element"
Blocks: 1770420
Blocks: 1774182

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.

Summary: [wdspec] Improve tests for "stale element reference" error states and fix cross-navigation navigation tests to expect "no such element" → [wdspec] Improve tests for "stale element reference" error states
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/da94b60da49c
[marionette] Raise "stale element reference" error if a stale element is returned by executing a script. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/e6568a46cece
[marionette] Update JSDoc @throws entries for WebDriver commands. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/7acb206e3cfa
[wdspec] Fix script execution tests to correctly encode and decode payload. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/74257730a058
[wdspec] Enhance "stale element reference" tests for WebDriver classic commands. r=webdriver-reviewers,jgraham,jdescottes
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/34465 for changes under testing/web-platform/tests
Whiteboard: [webdriver:m4] → [webdriver:m4], [wptsync upstream]
Upstream PR was closed without merging
Upstream PR merged by jgraham
You need to log in before you can comment on or make changes to this bug.