Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete test_valid_but_unmatchable_key webdriver test #17795

Open
wants to merge 3 commits into
base: master
from

Conversation

@julianrkung
Copy link
Contributor

julianrkung commented Jul 11, 2019

This test is designed with the assumption that the first set of capabilities should be rejected during the matching capabilities algorithm at step 3 when following the "otherwise" substep due to the extension capability "foo:unmatchable".

If name is the key of an extension capability, let value be the result of trying implementation-specific steps to match on name with value. If the match is not successful, return success with data null.

However, the test incorrectly assumes that the implementation-specific steps will result in an unsuccessful match. The test therefore is no longer strictly adhering to the defined spec.

No browsers are currently passing this tests so I propose that it should be removed.

@jgraham

This comment has been minimized.

Copy link
Contributor

jgraham commented Jul 12, 2019

That's the wrong bit of spec. I think the spec is unclear here, but the relevant part is the validate capabilites algorithm. In this case the question is "is foo:unmatched the name of an extension capability"? I think the spec is unclear about whether any capability including the : matches that condition. My understanding is that the intent is in fact that only implementation-recognised extension capabilites are matched there. So I think the test is correct to the extent that it's trying to test something meaningful, but there ought to be a spec clarification to be sure.

@andreastt andreastt removed their request for review Jul 12, 2019
@JohnChen0

This comment has been minimized.

Copy link
Contributor

JohnChen0 commented Jul 12, 2019

Note that this test is also in conflict with https://github.com/web-platform-tests/wpt/blob/master/webdriver/tests/new_session/support/create.py#L38, where "test:extension" is considered to be a valid capability. If we decide to interpret the spec as intending to reject all unrecognized extension capabilities, then "test:extension" should be moved to invalid_data section.

@andreastt

This comment has been minimized.

Copy link
Member

andreastt commented Jul 12, 2019

To your last point, we can’t reject unknown extension capabilities because they may not all be meant for the endpoint node. For example, there may exist extension capabilities in the new session request that are meant for intermediary nodes, so the design of the capabilities matching is meant for them to be passed through uninterrupted. This is why we test that test:extension is not being rejected.

@JohnChen0

This comment has been minimized.

Copy link
Contributor

JohnChen0 commented Jul 12, 2019

If test:extension is allowed, then shouldn't foo:unmatchable also be allowed?

@julianrkung

This comment has been minimized.

Copy link
Contributor Author

julianrkung commented Jul 19, 2019

I also do not understand the reasoning behind test:extension being allowed if foo:unmatchable must be rejected. An explanation would be really appreciated!

@julianrkung

This comment has been minimized.

Copy link
Contributor Author

julianrkung commented Aug 1, 2019

Updates on this? Would like to not forget about this PR

@mjzffr mjzffr removed their request for review Aug 27, 2019
@mjzffr mjzffr removed their assignment Aug 27, 2019
@JohnChen0

This comment has been minimized.

Copy link
Contributor

JohnChen0 commented Oct 8, 2019

This issue was discussed during TPAC (https://www.w3.org/2019/09/19-webdriver-minutes.html#item11), and the resolution was to always accept unknown extension capabilities. So I believe this PR should be merged. Could someone with write access to the repo handle it? Thanks.

Copy link
Member

Hexcles left a comment

Although not a part of the WG, I was at the meeting and @JohnChen0 's summary matches my understanding.

I'm going to approve and merge this now to not let the PR go stale. If I missed anything, feel free to revert it.

@Hexcles Hexcles closed this Oct 22, 2019
@Hexcles Hexcles reopened this Oct 22, 2019
@Hexcles

This comment has been minimized.

Copy link
Member

Hexcles commented Oct 22, 2019

@JohnChen0 is going to send a PR to clarify the spec.

@andreastt

This comment has been minimized.

Copy link
Member

andreastt commented Oct 23, 2019

@JohnChen0 In light of w3c/webdriver#1454 (review), perhaps this should be changed to reject unknown extension capabilities instead of beng deleted?

@JohnChen0

This comment has been minimized.

Copy link
Contributor

JohnChen0 commented Oct 23, 2019

@JohnChen0 In light of w3c/webdriver#1454 (review), perhaps this should be changed to reject unknown extension capabilities instead of beng deleted?

@andreastt Please see my comment at w3c/webdriver#1454 (comment), and the TPAC minutes. My understanding is unknown extension capabilities should be accepted instead of rejected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.