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(fixtures): introduce script to update the report fixtures #4793

Merged
merged 10 commits into from
Mar 21, 2018

Conversation

paulirish
Copy link
Member

@paulirish paulirish commented Mar 17, 2018

This includes my previous two PRs. sorry. Compare link without those PRs in the mix.

Additions / changes:

  • yarn update:json: refreshes the sample_v2.json from static artifacts. Great as it's the exact same load, so minimal json changes.
  • yarn update:artifacts: needed less often, but necessary when changes/additions are made to gatherers
  • static-server is a bit more scriptable now, and this PR lays the groundwork for porting the smokehouse run-test scripts into JS (yo tests(smokehouse): run smoketests in parallel #4748!).

This does check in 3.9MB of artifacts to the repo. But I think it's worth it.

@paulirish paulirish changed the title tests(fixtures): introduce script to update the report fixtures tests: introduce script to update the report fixtures Mar 17, 2018
@paulirish paulirish changed the title tests: introduce script to update the report fixtures tests(fixtures): introduce script to update the report fixtures Mar 17, 2018
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.

maybe I'm being blinded, but not entirely what this gets us over a shell script with lighthouse -GA=./lighthouse-cli/test/sample_artifacts --output=json --output-path=./lighthouse-core/test/results/sample_v2.json 😄

@@ -0,0 +1,53 @@
/**
* @license Copyright 2016 Google Inc. All Rights Reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

2018 ;)

const cli = require('../../lighthouse-cli/run');

const {server} = require(
'../../lighthouse-cli/test/fixtures/static-server');
Copy link
Collaborator

Choose a reason for hiding this comment

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

yikes is this really over the line length!?

@paulirish
Copy link
Member Author

maybe I'm being blinded, but not entirely what this gets us over a shell script...

sure sg. for the -A side of things, I've updated the npm script to use this direct approach.

The -G side of things still needs some special handling because of the web server.

.gitignore Outdated
@@ -24,6 +24,9 @@ last-run-results.html
*.report.pretty
*.artifacts.log

!lighthouse-cli/test/sample_artifacts/*.trace.json
Copy link
Member

Choose a reason for hiding this comment

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

should these live somewhere with sample_v2.json?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure!

how about lighthouse-core/test/results/artifacts/?


(async function() {
await update();
})();
Copy link
Member

@brendankenny brendankenny Mar 19, 2018

Choose a reason for hiding this comment

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

nit: any reason to wrap update()?

package.json Outdated
@@ -48,6 +48,8 @@
"plots-smoke": "bash plots/test/smoke.sh",
"changelog": "conventional-changelog --config ./build/changelog-generator/index.js --infile changelog.md --same-file",
"type-check": "tsc -p .",
"update:artifacts": "node lighthouse-core/scripts/update-report-fixtures.js -G",
Copy link
Member

Choose a reason for hiding this comment

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

Need something more specific. Maybe update:sample-artifacts or something? (same with json below)

Copy link
Member Author

Choose a reason for hiding this comment

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

sg. updated.

@@ -20,6 +20,7 @@
"lighthouse-core/lib/dependency-graph/**/*.js",
"lighthouse-core/gather/connections/**/*.js",
"lighthouse-core/gather/gatherers/gatherer.js",
"lighthouse-core/scripts/*.js",
Copy link
Member

Choose a reason for hiding this comment

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

🎉

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!

@@ -429,3 +429,5 @@ class Runner {
}

module.exports = Runner;

Copy link
Collaborator

Choose a reason for hiding this comment

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

revert? :)

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM!

I wonder if we should start printing devtoolsLog in the style of traces so we can get better diffs on them in artifacts

@paulirish
Copy link
Member Author

I wonder if we should start printing devtoolsLog in the style of traces so we can get better diffs on them in artifacts

good idea.

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.

4 participants