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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

misc: add lhci dogfood script to travis #9677

Merged
merged 9 commits into from Sep 25, 2019
Merged

misc: add lhci dogfood script to travis #9677

merged 9 commits into from Sep 25, 2019

Conversation

patrickhulce
Copy link
Collaborator

@patrickhulce patrickhulce commented Sep 14, 2019

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 as master branch because of the travis-side merge of the PR build in lhci repo but it's happening on this side again now :( fixed! 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!

# Install LHCI
npm install -g @lhci/cli
# Collect our LHCI results.
lhci collect --url=http://localhost:9090/lhci-report.html
Copy link
Collaborator Author

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


set -euox pipefail

# This script requires LHCI_CANARY_SERVER_URL and LHCI_CANARY_SERVER_TOKEN variables to be set.
Copy link
Collaborator Author

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" \
Copy link
Member

Choose a reason for hiding this comment

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

yarn now-build will give you the same looking file in ./dist/now/english/index.html

image

--quiet --output=html

# Start up a test server.
npx http-server -p 9090 &
Copy link
Member

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?

Copy link
Collaborator

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

Copy link
Collaborator Author

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

Copy link
Collaborator

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?

Copy link
Member

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?)

Copy link
Collaborator Author

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 $!
Copy link
Member

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
Copy link
Collaborator

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

...

Copy link
Collaborator Author

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? :)

Copy link
Collaborator

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

Copy link
Collaborator Author

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 :)

@paulirish
Copy link
Member

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!


set -euox pipefail

# This script requires LHCI_CANARY_SERVER_URL and LHCI_CANARY_SERVER_TOKEN variables to be set.
Copy link
Member

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?

Copy link
Collaborator Author

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.

lighthouse-core/scripts/dogfood-lhci.sh Outdated Show resolved Hide resolved
lighthouse-core/scripts/dogfood-lhci.sh Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants