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

Lighthouse ci action #1401

Merged
merged 60 commits into from
Nov 3, 2020
Merged

Conversation

tunetheweb
Copy link
Member

@tunetheweb tunetheweb commented Nov 3, 2020

Fixes #1178

Adds Automatic Lighthouse checks to our Test Website action to test for regressions as good changes.

Note we disable a few checks:

  • Time-based performance checks are disabled as too unreliable
  • Optimisations we don't currently perform (e.g. minifying CSS and JS and removing unused CSS and JS)
  • Turns off items that don't work on dev server (e.g. Canonical is incorrect, Compression is not used, Crawling is disabled...etc.).

This means we should only get real failures.

I also added a weekly run against production action. This looks at Lighthouse scores - though again not Performance due to variability as really want to avoid introducing noisy checks that create false alerts sometimes.

As well as avoiding #1216 (which is where this whole thing came from!) it's also identified another couple of issues meaning we have less than perfect scores:

  • The Spanish CSS chapter has an empty link
  • Links with lints don't have enough spacing on mobile

I think this shows the value of automating these checks!

Monitoring performance needs some more thought. We could do several runs to average, and set thresholds, or leverage the Page Speed Insights API to get CrUX data or something similar. Anyway think this PR has enough value even without explicit performance monitoring.

Interested to hear your thoughts!

@tunetheweb tunetheweb added development Building the Almanac tech stack SEO SEO related accessibility Accessibility related labels Nov 3, 2020
@rviscomi rviscomi added this to the 2020 Platform Development milestone Nov 3, 2020
@rviscomi rviscomi removed their request for review November 3, 2020 16:51
@rviscomi
Copy link
Member

rviscomi commented Nov 3, 2020

Thanks @bazzadp for the PR. Excited about LH integration. I'll defer to @ibnesayeed's review.

@tunetheweb tunetheweb requested a review from ibnesayeed November 3, 2020 18:39
Copy link
Contributor

@ibnesayeed ibnesayeed left a comment

Choose a reason for hiding this comment

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

I am okay with the other changes except the last one where I think using jq for building and formatting the config file would be better than manually doing it, which is brittle as it introduces tight coupling between the config file format and the shell script.

@tunetheweb tunetheweb requested a review from ibnesayeed November 3, 2020 20:46
Copy link
Contributor

@ibnesayeed ibnesayeed left a comment

Choose a reason for hiding this comment

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

LGTM!

However, I wish there was a way in lighthouse to supply list of URIs separately, without mixing it into the config file. Please see if there are any configuration options that can point to a file for list of URIs. That said, this PR can be merged, and if a better approach is found, we can make necessary changes later.

@tunetheweb tunetheweb merged commit e9206bc into HTTPArchive:main Nov 3, 2020
@tunetheweb tunetheweb deleted the lighthouse-ci-action branch February 6, 2021 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility Accessibility related development Building the Almanac tech stack SEO SEO related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Lighthouse testing
3 participants