Run Lighthouse against a local server per push #216
Conversation
|
Since there are more files, I am also putting the artifacts into a sub-folder with the build number, so they are a little easier to keep together, which i have found to be important when iterating on these kinds of things. |
|
Hey all - I just want to flag that I'm getting concerned about the amount of extra complexity that we are starting to add to the repository. I think that adding lighthouse would certainly help, and that we should be working towards improving the HTML that the theme uses. That said, can we guarantee that we'll be able to maintain this machinery when it breaks, when it needs to be improved, etc? Somebody feel free to tell me "these changes really aren't that big, and they're simple enough that they shouldn't break" - I must admit that it all looks Greek to me, so I'm not in a good position to judge their complexity, but I wanted to voice this concern to make sure we can discuss. I don't want this to come across as me shooting down this work - I think it's awesome and I really appreciate @bollwyvl putting in the effort to make these improvements. I am open to being and hope to be convinced that I'm being overly conservative here... signed, |
|
If they do break, it's just a comment away from getting them off the
critical path, but, like a linter, if these checks aren't enforced, they
will be ignored. They could also be isolated in a pr-only workflow file to
prevent hiccups when shipping.
In my biased opinion, they are simple enough and shouldn't break, once we
figure out a predictable workflow. Pinning to known action versions, and
using the relatively well-known tornado opinion, reduces risk vs a more
involved solution e.g. more nodejs tooling, or heavyweight selenium testing.
If it catches one accessibility regression, it will be worth it... e.g.
once it hits 💯 on accessibility, it should not be allowed to regress... which
the reports indeed point out is just the beginning, as other things like
keyboard accessibility and logical page order are harder to assess
automatically.
My intent here, selfishly, is being able to make the case for work usage
that this theme is checking the boxes for performance, security and
accessibility. A more community-focused intent: given the number of
important sites that use this theme, small regressions could have a
significant impact on differently-abled users, while improvements can be
shared equally.
|
|
Thanks for framing it that way. IMO:
Are worthwhile goals to increase complexity for. I just want to make sure our decisions to do so are made diligently 👍 |
|
Yeah, @choldgraf, I certainly share your general concern. But I also want to ensure that the people who know more about this web stuff, like @bollwyvl, can do their thing ;), as the theme will benefit from that as well. But it's certainly a trade-off to keep being careful about. Now about the actual PR: @bollwyvl, so we now have an additional "audit" check, that has a check for the minimum scores ("lighthouse-check: Scores passed minimum requirement ✅"), and has the actual html reports as artifact. |
I'm sure they could be. Actually using circle seems like too great of a dependency between the two providers, however. Unlike the demo site, these are flat files, so they could be posted to a gist... weirdly i don't see a handy-dandy action for it, but I'm pretty sure cURL could do it. Otherwise there's netlify, etc... |
|
OK. The PR is ready to merge from your side? |
Indeed, if only to get the broken one pointed at circle out of there! Did a little looking into netlify... it is free for open source, but doesn't really have shared auth, so it might need a bot account to own the credentials. Probably similar for the gist approach. Binder of course, would work... but you wouldn't want to build a whole, brand new docker container to view 3+ html files... but having an nbgitpuller bring in a gist would probably be fine. Apparently there's a way to get the raw JSON back out, as well, so maybe a notebook that loads those into a dataframe would be nice, as well. |
|
OK, let's see how it goes now ;) |
|
Huzzah!
…On Thu, Jul 9, 2020, 07:53 Joris Van den Bossche ***@***.***> wrote:
OK, let's see how it goes now ;)
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#216 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAALCREBPXT3DT6LTKLK6GLR2WVSXANCNFSM4OIO6CBQ>
.
|
This moves the lighthouse audit into the main, PR-visible workflow, and runs the audit against
localhost, as served by a local tornado server with some basic stuff configured: gzip, basically-infinite caching.Since it can actually be iterated on, I've gone ahead and also added the threshold check (with a little wiggle room), and do a check against a random smattering of pages, though it would probably make sense to hit all of them at some future point.
I think this is getting pretty close!