Closed Bug 1499057 Opened 2 years ago Closed 2 years ago
.Session ._element _cache
46 bytes, text/x-phabricator-request
|Details | Review|
46 bytes, text/x-phabricator-request
|Details | Review|
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 , element_clear is meant > to return an error when the current browsing context (content > browser) has been discarded . > > 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. > >  > https://searchfox.org/mozilla-central/rev/ > 3a54520d8d2319a4116866371ed3d9ed2ec0cc2b/testing/web-platform/tests/ > webdriver/tests/element_clear/clear.py#49-53 >  > 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
Assignee: nobody → ato
Status: NEW → ASSIGNED
Priority: -- → P1
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 firstname.lastname@example.org: 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
Can't merge web-platform-tests PR due to failing upstream checks: Github PR https://github.com/web-platform-tests/wpt/pull/13562 * continuous-integration/travis-ci/pr (https://travis-ci.org/web-platform-tests/wpt/builds/442652614?utm_source=github_status&utm_medium=notification)
Backed out changeset 5df04b130a16 (Bug 1499057) for Wd failures in /webdriver/tests/element_send_keys/interactability.py CLOSED TREE Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=5df04b130a16afcaa917204055b8227f1ef0f411&selectedJob=206104330 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=206104330&repo=autoland&lineNumber=10515 Backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=29c0fc82e879891cd13464dc028c1d1c4da14ece
Backout by email@example.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, "==".
Pushed by firstname.lastname@example.org: 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.
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.