-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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: add CI test to confirm sample_v2.json is up to date #4956
Conversation
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.
LGTM! some nits
#!/usr/bin/env bash | ||
|
||
## | ||
# @license Copyright 2017 Google Inc. All Rights Reserved. |
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.
2018 ;)
@@ -0,0 +1,26 @@ | |||
#!/usr/bin/env node |
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.
wanna kebab-case this file?
@@ -0,0 +1,49 @@ | |||
#!/usr/bin/env bash |
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.
wanna kebab-case this sucker?
@@ -36,6 +36,7 @@ script: | |||
- yarn unit:silentcoverage | |||
- yarn type-check | |||
- yarn closure | |||
- yarn diff:sample-json |
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.
let's throw it into yarn test
too for local snazziness :)
might need to revert the file to original state though?
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.
might need to revert the file to original state though?
For CI: no changes, so no worries, right? right.
For local: yes, working copy could have changes already. In this case, it seems like you'd want to get reported on the fact that your working copy, while changed, is still not where it needs to be. But it seems like you wouldn't want a failure when you've already bumped the JSON.
if [ $retVal -eq 0 ]; then | ||
colorText "✅ PASS. No change in LHR." "$green" | ||
else | ||
colorText "❌ FAIL. LHR has changed." "$red" |
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.
should this include something like "make sure to run update:sample-json
and check in the result?"
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.
great idea.
*/ | ||
'use strict'; | ||
|
||
/** @fileoverview Read in a LHR JSON file, remove whatever shouldn't be compared, write it back. */ |
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.
rename file (cleanup-LHR-for-diff.js
?) maybe? Unclear what the cleanup is for
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.
sg done
from the failing tests (this test :), I think you need to rebase? |
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.
LGTM!
from #4951 (comment)
is this what ya'll were thinking?
Currently changes within
LHR.timing
are ignored. I could also see us ignore changes toauditResult.helpText
to make kayce's PRs easier.