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.

Sign up for GitHub

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

refactor performance experiment server #1189

Merged
merged 4 commits into from
Jan 4, 2017
Merged

refactor performance experiment server #1189

merged 4 commits into from
Jan 4, 2017

Conversation

weiwei-lin
Copy link
Contributor

Main changes:

  1. Saving results as JSON instead of HTML. This can ease results comparison in Project Performance Experiment (Perf-X) Project Performance Experiment #1143.
  2. Save flags. Perf-X will need this to rerun the lighthouse (with some extra configurations).
  3. Save file and construct report page URL in server.js instead of bin.ts. This hopefully can make bin.ts more readable and reduce coupling.

brendankenny
brendankenny reviewed Dec 21, 2016
View reviewed changes
lighthouse-cli/bin.ts
opn(`http://localhost:${port}/reports/${filename}`);
});
});
return performanceXServer.hostExperiment(flags, results).then(opn);
Copy link
Member

@brendankenny brendankenny Dec 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I'll have to wait for the morning to take a full look, but love this already :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks :)

@weiwei-lin
refactor server.js again
4cdb8c1 
server no longer save any file to disk\nserver now can be closed by ctrl-c properly\nimproved code simplicity\n
paulirish
paulirish approved these changes Jan 4, 2017
View reviewed changes
Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

this is great, and a huge improvement.

Small nit, but otherwise, let's land this!

(we'll have to sort out the conflicts between this and your other)

lighthouse-cli/performance-experiment/server.js
* @return {!Promise<string>} Promise that resolves when server is closed
*/
let lhResults;
function hostExperiment(lighthouseParams, results) {
Copy link
Member

@paulirish paulirish Jan 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

right now this seems more like serveReport or serveAndOpenReport. Once experiments are in place, then I think we could rename the entry method's name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Already renamed it to serveAndOpenReport.

@weiwei-lin weiwei-lin merged commit 2e601cc into GoogleChrome:master Jan 4, 2017
@weiwei-lin weiwei-lin deleted the refactor-perf-x-server branch January 4, 2017 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brendankenny brendankenny brendankenny left review comments

@paulirish paulirish paulirish approved these changes

@samuelli samuelli Awaiting requested review from samuelli

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@weiwei-lin @paulirish @brendankenny