[wdspec] add context locator tests involving iframes and shadow DOM #50047
Conversation
OrKoN
force-pushed
the
orkon/context-locator-invalid
branch
from
74ab0d7 to
785d8ac
Compare
OrKoN
force-pushed
the
orkon/context-locator-invalid
branch
from
785d8ac to
abb3ddb
Compare
wpt-pr-bot
requested review from
AutomatedTester,
jgraham,
juliandescottes,
sadym-chromium,
shs96c and
whimboo
OrKoN
changed the title
| @@ -76,7 +76,7 @@ async def test_params_locator_accessability_value_invalid_type( | |||
| ("xpath", ""), | |||
| ("innerText", ""), | |||
| ("accessibility", {}), | |||
| ("context", {}) | |||
| ("context", {"context": ""}) | |||
There was a problem hiding this comment.
Here we should have a dedicated test as well for invalid types similar to the accessibility locator.
Note that the accessibility value the line above is wrong as well and is actually an invalid type test.
| @pytest.mark.asyncio | ||
| async def test_locate_by_context_invalid_context(bidi_session, inline, top_context, domain): | ||
| iframe_url_2 = inline("<div>foo</div>", domain=domain) | ||
| iframe_url_1 = inline(f"<div><iframe src='{iframe_url_2}'></iframe></div>", domain=domain) |
There was a problem hiding this comment.
Please use the iframe fixture in those two lines. I assume we don't have to put it inside a div as well, right?
| @@ -218,3 +218,24 @@ async def test_params_start_nodes_dom_node_not_element( | |||
| locator={"type": "css", "value": "div"}, | |||
| start_nodes=[remote_reference], | |||
| ) | |||
|
|
|||
|
|
|||
| @pytest.mark.parametrize("domain", ["", "alt"], ids=["same_origin", "cross_origin"]) | |||
There was a problem hiding this comment.
For this invalid case I don't see the need to check cross origin. Is there a reason why you added it?
There was a problem hiding this comment.
I think it actually might make sense to check both depending on whether the implementation incorrectly relies on checking if the context is valid by checking its presence in the same renderer process (which is up to implementation).
There was a problem hiding this comment.
I think we should check the happy case for both same-process and OOPiF, while keep invalid arg test generic. I can hardly imagine significant regression here.
| iframe_url_1 = inline("<div id='in-iframe'>foo</div>", domain=domain) | ||
| page_url = inline(f"<iframe src='{iframe_url_1}'></iframe>") | ||
| iframe_url_1 = inline("<div>foo</div>", domain=domain) | ||
| page_url = inline(f"<iframe id='target' src='{iframe_url_1}'></iframe>") |
There was a problem hiding this comment.
Please use the iframefixture here as well.
There was a problem hiding this comment.
iframe fixture does not support custom id attributes, which is useful for assertion in this and the following tests.
| @pytest.mark.parametrize("mode", ["open", "closed"]) | ||
| @pytest.mark.asyncio | ||
| async def test_locate_by_context_in_shadow_dom(bidi_session, inline, top_context, domain, mode): | ||
| iframe_url_1 = inline(f"<div>foo</div>", domain=domain) |
There was a problem hiding this comment.
You could use the get_test_page fixture to all the setup basically in one line.
There was a problem hiding this comment.
There is no way to opt out iframe creation in get_test_page, which makes using it in this context more indirect, as there will be 2 iframes
No description provided.