-
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
misc: add lhci dogfood script to travis #9677
Conversation
scripts/dogfood-lhci.sh
Outdated
# Install LHCI | ||
npm install -g @lhci/cli | ||
# Collect our LHCI results. | ||
lhci collect --url=http://localhost:9090/lhci-report.html |
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.
these two lines plus line 39 are all that we would expect a typical user to add to their travis
scripts/dogfood-lhci.sh
Outdated
|
||
set -euox pipefail | ||
|
||
# This script requires LHCI_CANARY_SERVER_URL and LHCI_CANARY_SERVER_TOKEN variables to be set. |
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 added these to our travis settings already
fi | ||
|
||
# Generate an HTML report | ||
node "$LH_ROOT_DIR/lighthouse-cli" \ |
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.
--quiet --output=html | ||
|
||
# Start up a test server. | ||
npx http-server -p 9090 & |
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.
is using static-server too much of a pain?
or could we use python simplehttpserver?
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.
python3 -m http.server 9000
works too
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.
did not expect such controversy over it haha, this is how I would've expected documenting this to a potential lhci user who might only have node in their environment so wanted to use it like they would.
I can switch to static-server.js if we'd prefer
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.
Does that mean we'd be adding static-server and its features to our stable API?
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.
Does that mean we'd be adding static-server and its features to our stable API?
I think the intention is for this to just test out lhci on the report, not be a general solution for folks using lhci (whether or not it should be written like the general solution is another question, but I guess this has the benefit of not needing another dep or download?)
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 think the intention is for this to just test out lhci on the report, not be a general solution for folks using lhci (whether or not it should be written like the general solution is another question, but I guess this has the benefit of not needing another dep or download?)
Yeah exactly.
Does that mean we'd be adding static-server and its features to our stable API?
Nope, this script has absolutely no bearing on the API for lighthouse or lhci. It's just about how we use lhci for our internal testing.
lhci upload --serverBaseUrl="$LHCI_CANARY_SERVER_URL" --token="$LHCI_CANARY_SERVER_TOKEN" | ||
|
||
# Kill the static server from earlier. | ||
kill $! |
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.
kill $(jobs -p)
would nuke all child processes. (in case the lhci commands also did backgrounded stuff.
but they don't. so it doesnt really matter. :)
@@ -0,0 +1,50 @@ | |||
#!/bin/bash | |||
|
|||
set -euox pipefail |
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.
what's the benefit of doing this here instead of in the /bin/bash shebang? all my personal scripts look like this:
#!/bin/bash -ex
...
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.
no benefit I'm aware of, just personal habit I suppose.
is there a benefit to doing it in the shebang? :)
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 like one liners i guess
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.
oh after some googling I remembered I started this habit back at a company that used #!/usr/bin/env bash
in all our scripts where the one-liner wasn't an option, so I guess that's one benefit of this set
method is that it can be consistent across those two methods :)
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!
|
||
set -euox pipefail | ||
|
||
# This script requires LHCI_CANARY_SERVER_URL and LHCI_CANARY_SERVER_TOKEN variables to be set. |
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.
LHCI_CANARY_SERVER_TOKEN
does this mean only PRs from core contributors will get this?
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 could've sworn I replied to this already...
anyhoo, yes it's only available to core contributors at the moment, but that's not a hard requirement, we could loosen this and hardcode them if we wanted later they're not really that secret since you could find them out yourself given the URL of our server.
Summary
Very early and very rough around the...everywhere, but at least enough to bump into lots of weird behavior that needs to be discussed how it should be handled :)
EDIT: This is probably too early to merge 😢, I fixed the bug where PRs get labeled asfixed! travis docs don't seem to tell to truth or at least we seem to have different env variables set than documented and that exist over at lighthouse-ci repo, 🤷♂ oh well it works now!master
branch because of the travis-side merge of the PR build in lhci repo but it's happening on this side again now :(