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

feat(v2): Added Lighthouse CI to PR checks #3761

Merged
merged 26 commits into from Nov 26, 2020

Conversation

sarthakkundra
Copy link
Contributor

Motivation

Added Lighthouse CI to integrate finally and create a complete Build bot to analyse stats changed with every new pull request.

Have you read the Contributing Guidelines on pull requests?

Yes.

Test Plan

A new comment action is added.

Related PRs

#3744

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 16, 2020
@netlify
Copy link

netlify bot commented Nov 16, 2020

Deploy preview for docusaurus-2 ready!

Built without sensitive environment variables with commit b407ab8

https://deploy-preview-3761--docusaurus-2.netlify.app

@sarthakkundra sarthakkundra changed the title Lighthouse Added Lighthouse CI Nov 16, 2020
@@ -64,6 +64,8 @@ function DocSidebarItemCategory({
return isActive ? false : item.collapsed;
});

const [enableCSSStyles, setEnableCSSStyles] = useState(true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi

You probably messed up with your Git workflow, because it's not supposed to be there 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know it was 2 am IST. I will fix it today, that is why I didn't message for a review 😅

@@ -124,8 +124,7 @@ export default ({
if (metastring && codeBlockTitleRegex.test(metastring)) {
// Tested above
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
codeBlockTitle = metastring
.match(codeBlockTitleRegex)![1];
codeBlockTitle = metastring.match(codeBlockTitleRegex)![1];
Copy link
Collaborator

Choose a reason for hiding this comment

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

we already have this code formatting fixed on master

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just need to rebase, will do that when I make the other required changes. Sorry for the confusion

@slorber
Copy link
Collaborator

slorber commented Nov 17, 2020

Thanks

Do you know how I can validate this PR works as intended?

Will the comment only work after a merge?

@lex111
Copy link
Contributor

lex111 commented Nov 17, 2020

We definitely need to create our own config for Lighthouse (lighthouserc.json). At least to provide a path for PWA checks: https://v2.docusaurus.io/?offlineMode=true
An example of such a file can be found here.

@sarthakkundra
Copy link
Contributor Author

@slorber The stats will be provided on each PR. To check them for now you can go inside 'details' in the Github action and check for the Audit tab
lighthouse

@sarthakkundra
Copy link
Contributor Author

@slorber Since you gave the go ahead to upload the Lighthouse stats on a public server, I can add a comment that adds the URL to the uploaded stats just like the Netlify bot. I think that'll be good?

@sarthakkundra
Copy link
Contributor Author

@lex111 Do the score check only need to be applied to the PWA or both online and offline versions?

@lex111
Copy link
Contributor

lex111 commented Nov 17, 2020

I think checking one URL (with ?offlineMode=true) will be enough.

removed wait for Netlify action

corrected Netlify deploy urls

corrected sitename

corrected sitename
@sarthakkundra
Copy link
Contributor Author

@lex111 Netlify does not generate an offline preview mode so if I just add this PWA then it will check the live version of it and not the version that is generated with the new changes. So I don't think we should add it. If you had anything else in mind, do let me know.

As far as the config file is needed, it comes out of the box with the action and a budget.json file can be added, more on that here budget. If that's required, do let me know the exact configs of it, although the report generated by the action is pretty extensive with all the runtimes stated.

@sarthakkundra
Copy link
Contributor Author

@slorber The report generated by Lighthouse can be accessed here. To access it manually head over to the details of the Lighthouse CI action and you can find the link here.
lighthouse
Hope this clears the testing plan. Let me know if any changes are required.

Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

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

UPD: haven't seen the previous comment, if none of above can be done then LGTM 👍 from my part.


Timed out waiting for start_url (https://deploy-preview-3761--docusaurus-2.netlify.app/classic/index.html) to respond

As I wrote earlier, the URL must be used with offline mode enabled, then this error will not occur.

Also it is better for us to use the config file for Lighthouse, then we can exclude some checks that only distract attention:

  • Page is blocked from indexing
  • robots.txt is not valid
  • Document does not have a valid

These checks will never pass on Netlify deploys, so we can turn them off. As a result, we don't see real SEO metrics.

@lex111 lex111 added this to the v2.0.0-alpha.67 milestone Nov 17, 2020
@sarthakkundra
Copy link
Contributor Author

sarthakkundra commented Nov 18, 2020

@lex111 Added a lighthousesrc.json file and removed the following failing SEO audits.

  • robots-txt
  • document does not have tap targets
  • canonical

Unfortunately couldn't find an audit for page is blocked from indexing in the lighthouse config documentation. If you can find it then let me know, I'll add it.
Apart from that I think this PR is ready to be merged and will close this issue. Do let me know if anything else is required.

@lex111
Copy link
Contributor

lex111 commented Nov 18, 2020

Unfortunately couldn't find an audit for page is blocked from indexing in the lighthouse config documentation. If you can find it then let me know, I'll add it.

Maybe is-crawlable will help?

@sarthakkundra
Copy link
Contributor Author

@slorber The comment is ready I think it is just failing because you'll have to approve the action / PR. Here's a screenshot of the generated comment. I think this is what you wanted. Let me know if any changes are required.
lighthouse

@lex111
Copy link
Contributor

lex111 commented Nov 24, 2020

@sarthakkundra nice work, but can you please add offline_start_url to the skip audits list so that we get 100% score for PWA metric?

@sarthakkundra
Copy link
Contributor Author

@lex111 Done! :)

@lex111
Copy link
Contributor

lex111 commented Nov 24, 2020

@sarthakkundra thanks, but it doesn't seem to work :( And if you try to add just start-url to skip audits list?

@sarthakkundra
Copy link
Contributor Author

@lex111 Doesn't seem to work :/

@lex111
Copy link
Contributor

lex111 commented Nov 24, 2020

So, I understand, please add works-offline and offline-start-url, then everything will work for sure (I guess) and we get the long-awaited 100% for PWA.

@sarthakkundra
Copy link
Contributor Author

@lex111 Have a look

@lex111
Copy link
Contributor

lex111 commented Nov 24, 2020

Excellent! :)

@lex111 lex111 requested a review from slorber November 25, 2020 14:57
@slorber
Copy link
Collaborator

slorber commented Nov 26, 2020

Thanks, LGTM, but we'll see after merge if it works fine or not ;)

@slorber slorber changed the title Added Lighthouse CI feat(v2): Added Lighthouse CI to PR checks Nov 26, 2020
@slorber slorber added the pr: new feature This PR adds a new API or behavior. label Nov 26, 2020
@slorber slorber merged commit bd62be9 into facebook:master Nov 26, 2020
@sarthakkundra
Copy link
Contributor Author

Hahaha. Sure. Let me know if any changes are required :)

@slorber
Copy link
Collaborator

slorber commented Nov 30, 2020

@sarthakkundra it seems to work fine, FYI had to change this setting so that it works in forked repos too (#3846)

Was wondering if we can try to make the Lighthouse score more reliable. I think Netlify needs some warm up, wonder if it could get a more. predictable score by running Lighthouse twice on it (or more?), and uploading only the latest results, and uploading only the last result once the CDN caches are hotter?

Are there other settings we can use to make this score more reliable? (I don't know, maybe can we disable TTFB scores and everything that relates to network speed etc)
Can we run Lighthouse multiple times and have an average or something?

@sarthakkundra
Copy link
Contributor Author

@slorber We can definitely make Lighthouse run N number of times. Not sure about whether it uploads only the last score or the average of all.

Maybe you can open an issue and I'll look into the possible ways to make the scores better and make a PR? I'll change the number of time it runs and probably disable tests that are giving a false output / not applicable on Netlify deploys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants