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

Fix ChromeDriver acceptInsecureCerts capability tests #19584

Merged
merged 2 commits into from Oct 10, 2019

Conversation

@JohnChen0
Copy link
Contributor

JohnChen0 commented Oct 8, 2019

PR #19402 added acceptInsecureCerts capability to Chrome configuration
by default. This causes failures in tests that explicitly check this
capability. Fixing by removing the default acceptInsecureCerts
capability during new_session tests.

PR #19402 added acceptInsecureCerts capability to Chrome configuration
by default. This causes failures in tests that explicitly check this
capability. Fixing by removing the default acceptInsecureCerts
capability during new_session tests.
@JohnChen0

This comment has been minimized.

Copy link
Contributor Author

JohnChen0 commented Oct 8, 2019

@Hexcles PTAL

Copy link
Member

Hexcles left a comment

Looks like Firefox always adds acceptInsecureCerts. I'm wondering why the problem only arose after #19402 .

webdriver/tests/new_session/conftest.py Outdated Show resolved Hide resolved
Fix typo
Co-Authored-By: Robert Ma <bob1211@gmail.com>
@JohnChen0

This comment has been minimized.

Copy link
Contributor Author

JohnChen0 commented Oct 9, 2019

Looks like Firefox always adds acceptInsecureCerts. I'm wondering why the problem only arose after #19402 .

For Firefox, https://github.com/web-platform-tests/wpt/blob/master/tools/wptrunner/wptrunner/browsers/firefox.py#L132 add acceptInsecureCerts only if certutil_binary is not specified. In addition, https://github.com/web-platform-tests/wpt/blob/master/tools/wpt/run.py#L215 has the code to set certutil_binary, so acceptInsecureCerts is not actually added. (I wonder if the condition at https://github.com/web-platform-tests/wpt/blob/master/tools/wpt/run.py#L205 is intentional. During my test, kwargs["ssl_type"] has value of None, which doesn't equal to "none", so the condition evaluates to true.)

@mjzffr mjzffr removed their request for review Oct 9, 2019
@mjzffr mjzffr removed their assignment Oct 9, 2019
@andreastt

This comment has been minimized.

Copy link
Member

andreastt commented Oct 10, 2019

We don’t use the browser initialisation code referenced in https://github.com/web-platform-tests/wpt/blob/master/tools/wptrunner/wptrunner/browsers/firefox.py#L132 for the wdspec type. In the wdspec case we rely entirely on what the WebDriver implementation does by default. acceptInsecureCerts is not the default in geckodriver for the wdspec tests.

Copy link
Member

andreastt left a comment

This change is fine though, it doesn’t affect geckodriver at all and solves the chromedriver case.

Do note that chromedriver should not default to have acceptInsecureCerts on by default, so maybe this should be considered a chromedriver bug?

@Hexcles

This comment has been minimized.

Copy link
Member

Hexcles commented Oct 10, 2019

Do note that chromedriver should not default to have acceptInsecureCerts on by default, so maybe this should be considered a chromedriver bug?

That was the case (https://crbug.com/chromedriver/3148), and it's been fixed, which is why I needed to change wptrunner to explicitly ignore cert errors in Chrome, but the change inadvertently affected wdspec tests.

And thanks for the explanation!

@Hexcles

This comment has been minimized.

Copy link
Member

Hexcles commented Oct 10, 2019

Taskcluster detected a slow test on Firefox:
https://tools.taskcluster.net/groups/P2tFfOfAQN-f6F0SU9bE0Q/tasks/JchWgygIQZyKjHb0TGWDCQ/runs/0/logs/public%2Flogs%2Flive.log#L16673

It's definitely unrelated to this change so I'm going to admin-merge it.

@Hexcles Hexcles merged commit 8048ad3 into web-platform-tests:master Oct 10, 2019
9 of 13 checks passed
9 of 13 checks passed
Build Failed Build Failed
Details
Build Failed Build Failed
Details
Build Failed Build Failed
Details
Taskcluster (pull_request) TaskGroup: failure
Details
Azure Pipelines Build #20191009.110 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
wpt.fyi - chrome[experimental] Chrome results
Details
wpt.fyi - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
@Hexcles Hexcles deleted the JohnChen0:cert-cap branch Oct 10, 2019
@andreastt

This comment has been minimized.

Copy link
Member

andreastt commented Oct 11, 2019

Ah, thanks for the explanation!

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.