Closed Bug 1499057 Opened 10 months ago Closed 10 months ago

Remove webdriver.Session._element_cache

Categories

(Testing :: geckodriver, enhancement, P1)

enhancement

Tracking

(firefox64 fixed)

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: ato, Assigned: ato)

References

Details

Attachments

(2 files)

It was discovered in https://bugzilla.mozilla.org/show_bug.cgi?id=1478799
that the order in which wdspec tests are run could impact failures
to add fake web elements to the element cache of the session.

To circumvent this problem we could make it possible to construct
webdriver.Element without a webdriver.Session so that we can re-use
the abstractions for testing purposes.  It would also, obviously,
be possible to not allow this and force consumers to spell out the
web element identifier manually.
(In reply to Andreas Tolfsen ❲:ato❳ from bug 1478799 comment #18)
> With regards to test_no_browsing_context [1], element_clear is meant
> to return an error when the current browsing context (content
> browser) has been discarded [2].
> 
> This test creates a fake element to test this behaviour, because
> element_clear() takes that as input.  Because an element with the
> identifier "foo" already exists in the cache, you are seeing a
> “ValueError: Element already in cache” exception.  This is likely
> a problem with the order in which the tests are run.
> 
> The fix here is probably to make it possible to construct the Element
> class without a session, so that we can re-use the Element abstraction
> for serialising the web element.  I’ve filed
> https://bugzilla.mozilla.org/show_bug.cgi?id=1499057 about this.
> 
>   [1]
> https://searchfox.org/mozilla-central/rev/
> 3a54520d8d2319a4116866371ed3d9ed2ec0cc2b/testing/web-platform/tests/
> webdriver/tests/element_clear/clear.py#49-53
>   [2]
> https://searchfox.org/mozilla-central/rev/
> 3a54520d8d2319a4116866371ed3d9ed2ec0cc2b/testing/marionette/driver.js#2582
The problem here is actually the following...

A test creates this fake element with a unique id, which then gets added to the cache. Then the next test has the same capabilities, and as such no new session is getting created. It also means that the keep the cache from the former test. As such we see a failure that the element is already present in the cache, when trying to use the exact same element id again.

What we could do is to have a clean-up function on the `Session` class, which would let us clean-up some state like the element cache. Tests should basically no carry-over states from the previous one. And with this method we could get rid of that.
On the other side we could force to not use the same fake element id multiple times, but use a Python module like uuid to generate a unique identifier each time. This might be better.
This is about a test using the webdriver.Element abstraction to
construct a fake element so it doesn’t have to spell out the web
element identifier dictionary manually.  It can then pass the Element
instance off to element_clear() and the JSONEncoder will take care
fo the rest:
https://searchfox.org/mozilla-central/rev/c9272ef398954288525e37196eada1e5a93d93bf/testing/web-platform/tests/tools/webdriver/webdriver/protocol.py#17-18

Clearing the client’s known element cache is one way to make this
problem go away, as is allowing webdriver.Element to be constructed
without a session so that the cache isn’t touched.

However, examining the use of _element_cache I can’t really see any
sensible need for it to exist in the first place:
https://searchfox.org/mozilla-central/rev/c9272ef398954288525e37196eada1e5a93d93bf/testing/web-platform/tests/tools/webdriver/webdriver/client.py#672-687

What it does at the moment is avoid a few calls to constructing
webdriver.Element.  Since this is a client meant for testing the
WebDriver API, I propose we rip the entire thing out so that we’re
not blocked on a client-side assertion.  What we want to test here
is that the has-browsing-context-been-discarded test in the remote
end is run before is-this-a-known-web-element.
Summary: Make it possible to construct webdriver.Element without a session for testing purposes → Remove webdriver.Session._element_cache
See Also: → 1499135
Assignee: nobody → ato
Status: NEW → ASSIGNED
Priority: -- → P1
See Also: 1499135
The known web element cache in the WebDriver test client, or
webdriver.Session._element_cache, is used only to avoid constructing
new webdriver.Element instances of the same web element and serves
no practical purpose beyond that

Since this client is intended for testing purposes, we would like
to be able to construct duplicate webdriver.Element instances,
so that e.g. fake elements can be constructed and send to the remote end.
Do note that one more patch is needed to remove the hack introduced
by https://bugzilla.mozilla.org/show_bug.cgi?id=1499135.
We'd just back out bug 1499135 and mark that bug wontfix or invalid.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5df04b130a16
webdriver: remove known web element cache; r=whimboo
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13562 for changes under testing/web-platform/tests
Backout by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/29c0fc82e879
Backed out changeset 5df04b130a16 for Wd failures in /webdriver/tests/element_send_keys/interactability.py CLOSED TREE
Those failures are very suspicious. Andreas, as it looks like it needs a bit more massaging. :/
uh, silly me. That patch had landed to autoland, not m-i.
(In reply to Olli Pettay [:smaug] from comment #15)
> uh, silly me. That patch had landed to autoland, not m-i.

so my trypush was useless.
Upstream PR was closed without merging
The Python "is" operator tests object identity, but the tests should
rely on the webdriver.Element equality implementation, __eq__.
The problem here was that we were comparing web elements using the
object identity comparison operator "is", instead of the equality
comparator, "==".
Flags: needinfo?(ato)
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b5b389d5788e
webdriver: fix element comparison assertions; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/55724db90e3e
webdriver: remove known web element cache; r=whimboo
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
https://hg.mozilla.org/mozilla-central/rev/b5b389d5788e
https://hg.mozilla.org/mozilla-central/rev/55724db90e3e
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Upstream PR merged
Component: Marionette → geckodriver
Blocks: 1499485
No longer depends on: 1499485
You need to log in before you can comment on or make changes to this bug.