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

Change the path for SSL configs in browser_kwargs #22253

Closed
wants to merge 1 commit into from

Conversation

KiChjang
Copy link
Contributor

@KiChjang KiChjang commented Nov 22, 2018

Fixes #22252.


This change is Reviewable

@servo-wpt-sync
Copy link
Collaborator

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#14197.

@jdm
Copy link
Member

jdm commented Nov 22, 2018

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

⌛ Trying commit fe096a2 with merge 4ddabfb...

bors-servo pushed a commit that referenced this pull request Nov 22, 2018
Change the path for SSL configs in browser_kwargs

Fixes #22252.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22253)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel-css

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Nov 22, 2018
@KiChjang
Copy link
Contributor Author

Ok, so clearly this is a platform-specific problem, and I'll need some logic to detect if the current OS is windows or not.

@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Nov 24, 2018
@KiChjang
Copy link
Contributor Author

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

⌛ Trying commit 4fc9f3b with merge 2e35e84...

bors-servo pushed a commit that referenced this pull request Nov 24, 2018
Change the path for SSL configs in browser_kwargs

Fixes #22252.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22253)
<!-- Reviewable:end -->
@servo-wpt-sync
Copy link
Collaborator

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#14197.

@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel-css

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Nov 24, 2018
@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Nov 24, 2018
@KiChjang
Copy link
Contributor Author

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

⌛ Trying commit c4fe0de with merge 0db3c16...

bors-servo pushed a commit that referenced this pull request Nov 24, 2018
Change the path for SSL configs in browser_kwargs

Fixes #22252.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22253)
<!-- Reviewable:end -->
@servo-wpt-sync
Copy link
Collaborator

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#14197.

@bors-servo
Copy link
Contributor

☀️ Test successful - linux-rel-css, linux-rel-wpt
State: approved= try=True

@KiChjang
Copy link
Contributor Author

r? @jdm

@highfive highfive assigned jdm and unassigned nox Nov 24, 2018
@jgraham
Copy link
Contributor

jgraham commented Nov 26, 2018

I"m not sure what's going on here, but this is a pretty odd-looking change. If you are passing in the correct ssl_type then the path should be set correcty. Therefore I wonder if the config isn't set up to use ssl_type of pregenerated and that's the underlying issue?

@jdm
Copy link
Member

jdm commented Dec 13, 2018

I propose that we set kwargs['ssl_type'] = 'pregenerated' in

def set_defaults(kwargs):
and see if that works without any other changes.

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Dec 13, 2018
@KiChjang
Copy link
Contributor Author

Trying that without the patch applied here results in:

The details of the failure are as follows:

TypeError: coercing to Unicode: need string or buffer, NoneType found

  File "D:\Workspace\servo\python\servo\testing_commands.py", line 376, in test_wpt
    ret = self.run_test_list_or_dispatch(kwargs["test_list"], "wpt", self._test_wpt, **kwargs)
  File "D:\Workspace\servo\python\servo\testing_commands.py", line 416, in run_test_list_or_dispatch
    return correct_function(**kwargs)
  File "D:\Workspace\servo\python\servo\testing_commands.py", line 402, in _test_wpt
    return self.wptrunner(run_file, **kwargs)
  File "D:\Workspace\servo\python\servo\testing_commands.py", line 448, in wptrunner
    return run_globals["run_tests"](**kwargs)
  File "D:\Workspace\servo\tests\wpt\run.py", line 31, in run_tests
    set_defaults(kwargs)
  File "D:\Workspace\servo\tests\wpt\run.py", line 80, in set_defaults
    wptcommandline.check_args(kwargs)
  File "D:\Workspace\servo\tests\wpt\web-platform-tests\tools\wptrunner\wptrunner\wptcommandline.py", line 493, in check_args
    require_arg(kwargs, "ca_cert_path", lambda x:os.path.exists(x))
  File "D:\Workspace\servo\tests\wpt\web-platform-tests\tools\wptrunner\wptrunner\wptcommandline.py", line 31, in require_arg
    if name not in kwargs or not value_func(kwargs[name]):
  File "D:\Workspace\servo\tests\wpt\web-platform-tests\tools\wptrunner\wptrunner\wptcommandline.py", line 493, in <lambda>
    require_arg(kwargs, "ca_cert_path", lambda x:os.path.exists(x))
  File "C:\Python27\lib\genericpath.py", line 26, in exists
    os.stat(path)

i.e. the ca_cert_path is not found in kwargs.

@KiChjang
Copy link
Contributor Author

Oh, it looks like the ca_certificate_path never existed in the first place! This patch "works" because config.ssl["pregenerated"]["ca_cert_path"] was in fact None, and that simply causes the tests to not use SSL at all.

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Dec 22, 2018
@servo-wpt-sync
Copy link
Collaborator

Error syncing changes upstream. Logs saved in error-snapshot-1545479328147.

@KiChjang
Copy link
Contributor Author

@jdm @jgraham I think this should do the trick, please have a look and see if it's the correct fix.

@KiChjang KiChjang closed this Dec 22, 2018
@KiChjang KiChjang reopened this Dec 22, 2018
@servo-wpt-sync
Copy link
Collaborator

Error syncing changes upstream. Logs saved in error-snapshot-1545509318172.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-awaiting-review There is new code that needs to be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants