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

tests(smokehouse): adopt URLSearchParams for querystring manipulation #3941

Merged
merged 5 commits into from
Dec 8, 2017

Conversation

paulirish
Copy link
Member

This is what I was thinking about for #3815.

wdyt?

Copy link
Collaborator

@kdzwinel kdzwinel left a comment

Choose a reason for hiding this comment

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

LGTM

patrickhulce
patrickhulce previously approved these changes Nov 30, 2017
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

lgtm!


function headersParam(headers) {
return headers
.map(({name, value}) => `extra_header=${name}:${encodeURI(value)}`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh nice catch we should've been using encodeURIComponent here 👍

@@ -23,7 +23,7 @@ const getFlags = require('../../cli-flags').getFlags;

describe('CLI run', function() {
it('runLighthouse completes a LH round trip', () => {
const url = 'chrome://version';
const url = 'chrome://version/';
Copy link
Collaborator

Choose a reason for hiding this comment

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

hang on, I just realized the comment around this. Are we doing something wrong here?

just tried this out in console, seems like a bad result if everything gets a trailing slash now

const URL = require('whatwg-url').URL
console.log(new URL('chrome://version').href) // 'chrome://version'

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay looks like special urls had a trailing slash for a little bit in the spec but it was changed.

web-platform-tests/wpt#4586
whatwg/url#213
jsdom/whatwg-url@9ec5618

However Chrome doesn't yet implement this change. As a result, it adds the trailing slash when it delivers the canonicalized URL over the devtools protocol. In the future this'll probably change, but for now we'll have to normalize between the spec-violating Chrome and the correct whatwg-url.

Doesn't look like there's a crbug for this, though there's a sprinkling of other issues documenting Chrome's URL failures on webplatformtests.

exciting. 🤒

@paulirish paulirish dismissed patrickhulce’s stale review December 2, 2017 00:51

big enough change you should lgtm again. :)

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants