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(static-server): allow hook to modify response body #9872

Merged
merged 10 commits into from
Feb 19, 2020

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Oct 21, 2019

This hook is needed to modify the URLs in the fixtures for running smoke tests on Lightrider in CI.

Googlers: see cl/275909869 / doc

@patrickhulce
Copy link
Collaborator

I'm going to dismiss my review request @brendankenny this is all you since it's a large change to be unable to see the context for :)

@brendankenny
Copy link
Member

brendankenny commented Oct 22, 2019

I'm going to dismiss my review request @brendankenny this is all you since it's a large change to be unable to see the context for :)

string -> string transformations (e.g. replace on a regex) could make it so even innocuous changes break things (let alone future refactors). It would be great to get an idea of the decision/constraint space so we could evaluate if there are other alternatives (or just what to document so future folks know which live wires not to touch).

@connorjclark
Copy link
Collaborator Author

This is just meant to change the port numbers on URLs, so the hook could be less generic and be specifically replacePort(original, new) or sumthin.

We can follow up on this after I write / we discuss a doc re: the whole integration testing story.

@connorjclark
Copy link
Collaborator Author

The first post was updated with the relevant doc (@patrickhulce, nothing has changed since you last reviewed the doc)

I don't think knowing the usage internally is necessary for reviewing this change. The idea is just that the ports must be configurable (the internal test is assigned 2 free ports, and they are random). replacePort is the stricter way to do it, the current PR is the most generic way to do it. I don't see this static test server being consumed by anyone but ourselves, so perhaps the generic solution is fine?

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.

I think we'll need a story for how to exercise the other integration parts of this that are smokerider-specific.

The generic port part LGTM, but I don't feel very confident in my ability to review the other bits.

lighthouse-core/scripts/update-report-fixtures.js Outdated Show resolved Hide resolved
lighthouse-cli/test/fixtures/static-server.js Show resolved Hide resolved
lighthouse-cli/test/fixtures/static-server.js Show resolved Hide resolved
@patrickhulce
Copy link
Collaborator

Also I've since learned it's a Goog holiday so no need to reply @connorjclark, sorry :)

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

/**
* @param {string} url
*/
function get(url) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

fwiw, we already have isomorphic-fetch in our devDeps that should make this much easier :)

const fetch = require('isomorphic-fetch')
const get = url => fetch(url).then(r => r.text())

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so fetch

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

Successfully merging this pull request may close these issues.

5 participants