-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
plots: smoke test for happy case #2411
Conversation
get travis to retry
plots/test/smoke.sh
Outdated
node $analyze_script --ci | ||
node $clean_script | ||
|
||
echo "Finished plots smoke test without errors" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dontcha need some set -e
goodness to make this fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup.
plots/analyze.js
Outdated
@@ -45,6 +48,9 @@ function main() { | |||
GENERATED_RESULTS_PATH, | |||
`var generatedResults = ${JSON.stringify(generatedResults)}` | |||
); | |||
if (args.ci) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can do
if (process.env.CI)
and avoid adding a custom flag
plots/test/smoke.sh
Outdated
fi | ||
|
||
node $measure_script --site https://google.com/ -n 2 --disable-network-throttling --disable-cpu-throttling | ||
node $analyze_script --ci |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you want to assert anything about the analyze results?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do this is in a follow-up PR - I think many of the potential issues (e.g. lighthouse changes that break plots) would be caught without assertions.
I'm thinking about processing the output and replace non-deterministic values (e.g. timing values) and then diff it with a golden file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
marking this appropriately. my above comments. ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulirish - oy sorry for the long delay. can you take another look?
plots/analyze.js
Outdated
@@ -45,6 +48,9 @@ function main() { | |||
GENERATED_RESULTS_PATH, | |||
`var generatedResults = ${JSON.stringify(generatedResults)}` | |||
); | |||
if (args.ci) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
plots/test/smoke.sh
Outdated
fi | ||
|
||
node $measure_script --site https://google.com/ -n 2 --disable-network-throttling --disable-cpu-throttling | ||
node $analyze_script --ci |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do this is in a follow-up PR - I think many of the potential issues (e.g. lighthouse changes that break plots) would be caught without assertions.
I'm thinking about processing the output and replace non-deterministic values (e.g. timing values) and then diff it with a golden file.
Only +30s to travis times. Not bad. :) |
A simple smoke test for plots. Will add more over time.
Closes part of #1980