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

Jump to bottom

Fix ChromeDriver acceptInsecureCerts capability tests #19584

Merged
merged 2 commits into from Oct 10, 2019
Merged

Fix ChromeDriver acceptInsecureCerts capability tests #19584

merged 2 commits into from Oct 10, 2019

Conversation

JohnChen0
Copy link
Contributor

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
Fix ChromeDriver acceptInsecureCerts capability tests
fce48ea 
PR web-platform-tests#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
Copy link
Contributor Author

@Hexcles PTAL

Hexcles
Hexcles reviewed Oct 9, 2019
View reviewed changes
Copy link
Member

@Hexcles Hexcles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@JohnChen0 @Hexcles
Fix typo
376dda1 
Co-Authored-By: Robert Ma <bob1211@gmail.com>
@JohnChen0
Copy link
Contributor Author

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 October 9, 2019 19:58
@mjzffr mjzffr removed their assignment Oct 9, 2019
@andreastt
Copy link
Member

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.

andreastt
andreastt approved these changes Oct 10, 2019
View reviewed changes
Copy link
Member

@andreastt andreastt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
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
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
@Hexcles Hexcles deleted the cert-cap branch October 10, 2019 20:20
@andreastt
Copy link
Member

Ah, thanks for the explanation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hexcles Hexcles Hexcles left review comments

@andreastt andreastt andreastt approved these changes

@AutomatedTester AutomatedTester Awaiting requested review from AutomatedTester

@shs96c shs96c Awaiting requested review from shs96c

@whimboo whimboo Awaiting requested review from whimboo

Assignees

@Hexcles Hexcles

Labels
webdriver wg-testing_browser
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@JohnChen0 @andreastt @Hexcles @mjzffr @wpt-pr-bot