Add HTML validator action #579

krassowski · 2021-12-29T17:55:31Z

Adds four CI jobs:

build suite using Jekyll
validate HTML
check links (excluding absolute / links and LinkedIn links for now due to limitations of checker)
run Lighthouse audits:
- those get uploaded to temporary public storage on Google servers and can be accessed easily from the browser for 7 days

* Add HTML validator action * Update root to `_site` * Update validate.yml * Update validate.yml * Update validate.yml * Add check-links * Update validate.yml * Add lighthouse and ignores for URLs * Add lighthouse config * Update validate.yml

krassowski · 2021-12-29T18:03:48Z

The failures are intentional. I propose to merge and iterate by fixing the detected issues in subsequent PRs.

Should we set a threshold for lighthouse score?

choldgraf

This all sounds good to me, I think it's a great idea to add in some basic checks for reasonableness

choldgraf · 2021-12-29T18:36:23Z

.github/workflows/validate.yml

+    - name: Build the site in the Jekyll/builder container
+      run: |
+        docker run \
+        -v ${{ github.workspace }}:/srv/jekyll -v ${{ github.workspace }}/_site:/srv/jekyll/_site \


We have nox usable with this repository now, so we could just re-use that here via a verb like nox -s build. That might reduce some redundancy.

jupyter.github.io/noxfile.py

Lines 15 to 18 in 10cdc92

@nox.session(venv_backend='conda')

def build(session):

install_deps(session)

session.run(*"bundle exec jekyll serve liveserve".split())

We'd need to update that so that there was a build verb that didn't also do liveserve but that would be easy

choldgraf

quick idea on naming!

noxfile.py

krassowski · 2021-12-29T21:12:03Z

So I was enthusiastic about using common nox command here at first, but I am no longer convinced. The old docker-based approach was building in 51 seconds, the nox/conda takes 6 minutes 24 seconds. It is a substantial delay. I can go for mamba, cache etc but it will all increase complexity and maintenance burden - caching is hard.

Docker approach is way faster (1 minute vs 6.5 minutes)

choldgraf · 2021-12-29T21:27:32Z

Huh, that's surprising. But if it's that big a difference I agree. We could probably optimize the nox build down but that's not the best use of time in this PR in my opinion, unless you're enthusiastic to try it out

krassowski · 2021-12-29T21:31:11Z

I guess it comes down to having the docker image already available on GitHub infrastructure (the docker image is the one from the action suggested by GitHub) vs having to resolve dependencies and install them from conda - I would not expect a big difference in the actual build step.

choldgraf · 2021-12-29T21:40:36Z

Yeah for sure - maybe with mamba it'd be significantly faster? This also seems like it'd be a one time thing I'd we cached it properly. Environments all get stored in a .nox folder so maybe we could just cache that?

krassowski · 2021-12-29T21:56:06Z

Can we address the install via nox/mamba/caching in a follow-up PR? Automation is exhausting ;)

choldgraf · 2021-12-29T22:02:25Z

Totally, I'm just thinking out loud

krassowski · 2021-12-29T22:09:01Z

Currently the lighthouse result show up as:

Total size of all the files uploaded is 1165820 bytes
Finished uploading artifact lighthouse-results. Reported size is 1165820 bytes. There were 0 items that failed to upload
Uploading median LHR of http://localhost:45821/about.html...success!
Open the report at https://storage.googleapis.com/lighthouse-infrastructure.appspot.com/reports/1640813497053-27107.report.html
Uploading median LHR of http://localhost:45821/binder.html...success!
Open the report at https://storage.googleapis.com/lighthouse-infrastructure.appspot.com/reports/1640813497456-16487.report.html
Uploading median LHR of http://localhost:45821/community.html...success!
Open the report at https://storage.googleapis.com/lighthouse-infrastructure.appspot.com/reports/1640813497852-78305.report.html
Uploading median LHR of http://localhost:45821/documentation.html...success!
Open the report at https://storage.googleapis.com/lighthouse-infrastructure.appspot.com/reports/1640813498306-7357.report.html
Uploading median LHR of http://localhost:45821/hub.html...success!
Open the report at https://storage.googleapis.com/lighthouse-infrastructure.appspot.com/reports/1640813498846-65386.report.html
No GitHub token set, skipping GitHub status check.
Dumping 5 reports to disk at /home/runner/work/jupyter.github.io/jupyter.github.io/.lighthouseci...
Done writing reports to disk.

Error: 2 results for http://localhost:45821/about.html
Report: https://storage.googleapis.com/lighthouse-infrastructure.appspot.com/reports/1640813497053-27107.report.html

❌ categories.accessibility failure for minScore assertion
Expected >= 0.95, but found 0.94

❌ categories.best-practices failure for minScore assertion
Expected >= 0.95, but found 0.93
Error: 2 results for http://localhost:45821/binder.html
Report: https://storage.googleapis.com/lighthouse-infrastructure.appspot.com/reports/1640813497456-16487.report.html

❌ categories.accessibility failure for minScore assertion
Expected >= 0.95, but found 0.85

❌ categories.seo failure for minScore assertion
Expected >= 0.95, but found 0.92
Error: 1 result for http://localhost:45821/documentation.html
Report: https://storage.googleapis.com/lighthouse-infrastructure.appspot.com/reports/1640813498306-7357.report.html

❌ categories.accessibility failure for minScore assertion
Expected >= 0.95, but found 0.94

But we could go fancy and have:

This would require authorizing https://github.com/apps/lighthouse-ci ; it is tied to a private account of maintainer but also mentioned on https://github.com/GoogleChrome/lighthouse-ci/blob/main/docs/getting-started.md#github-status-checks.

Just an option - I don't have opinion on this one.

choldgraf · 2021-12-29T22:16:22Z

I'd be +1 on this personally, though also don't think we should block this PR on that if it'll require extra decision-making steps.

Is there a place where we can look at the lighthouse report itself, to quickly spot-check improvements we need to make? (e.g., similar to how the codecov reports can work?)

krassowski · 2021-12-29T22:17:48Z

Is there a place where we can look at the lighthouse report itself, to quickly spot-check improvements we need to make? (e.g., similar to how the codecov reports can work?)

The links in the logs should work, for example https://storage.googleapis.com/lighthouse-infrastructure.appspot.com/reports/1640813497053-27107.report.html for about.html highlights that the change of the links colour to orange reduced accessibility. Edit: this is temporary storage will only work for 7 days. I added note to top level comment.

choldgraf

This looks great to me - only two quick comments, and one suggestion below:

Could you add a short section to the README.md file to explain this workflow? Just a short explanation of these validation steps, and short instructions for how people can view a lighthouse report. Whatever somebody might need to understand how to make the use of this helpful CI/CD infrastructure.

.github/workflows/validate.yml

choldgraf · 2021-12-29T22:36:52Z

.github/workflows/validate.yml

+  validate:
+
+    runs-on: ubuntu-latest
+    name: Validate HTML


Could we add a comment somewhere just saying what we mean by "validate"? e.g., are we just doing some kind of basic integrity check? And would this not be captured by lighthouse already?

Some of them overlap (like alt meta tags for images, but others are not caught by Lighthouse).

By validate we mean run though W3C-sanctioned HTML integrity validator. In this case we use https://github.com/validator (https://validator.github.io/validator/) backend "Nu". It is featured on W3C: https://validator.w3.org/nu/ and https://validator.w3.org/docs/help.html#validation_basics describes what validation means to them.

then maybe we could just add a comment like # See https://validator.w3.org/docs/help.html#validation_basics for more information?

I added link to that to README in e62e6b0

choldgraf · 2021-12-29T22:41:14Z

Also just a note that I think one of the failing accessibility checks is for the new "jupyter orange" link colors that @palewire recently added in #558 :

https://storage.googleapis.com/lighthouse-infrastructure.appspot.com/reports/1640813497053-27107.report.html#accessibility

I wonder if we could darken those a bit to improve accessibility? I know that's technically no-longer "Jupyter orange", but also think we should prioritize accessibility best practices over strict adherence to design guidelines

palewire · 2021-12-29T22:56:08Z

Yes, I can darken it with a separate PR. Hmm. The default brand color isn't accessible on a white background? That might be a separate, larger problem.

palewire · 2021-12-29T22:56:37Z

Also, the fact that it was surfaced is vindication of this excellent idea for a test!

palewire · 2021-12-29T23:01:09Z

Patch: #581
Bug report: jupyter/design#67

palewire · 2021-12-29T23:45:51Z

This is super super cool, IMHO, and potentially a model for many other publishers. You want to merge it first, or try to sort out the many linty things it raises? I'm fine with a merge.

krassowski · 2021-12-29T23:50:03Z

I would say it is better to merge now and iterate. @sunnyjain15699 volunteered to address the validator failures (#526 (comment)) so I did not attempt to squash them and focused on automation work, but I did fix some link issues.

choldgraf · 2021-12-29T23:56:18Z

@krassowski you ready for a merge now? Or other stuff you wanna do?

krassowski · 2021-12-29T23:59:10Z

Ready to merge!

krassowski · 2021-12-30T00:12:51Z

Ok, I extracted the follow up tasks to new issues and will merge this one in 10 minutes if there are no further comments.

palewire · 2021-12-30T00:13:46Z

Congratulations. This is well done. I'm already thinking about how how team at latimes.com could add something like this to our build tool https://github.com/datadesk/baker-example-page-template

choldgraf · 2021-12-30T00:15:59Z

Then let's do it! Thanks @krassowski 🙂 and thanks @palewire for the reviews and ideas

Add HTML validator action (#1)

300c028

* Add HTML validator action * Update root to `_site` * Update validate.yml * Update validate.yml * Update validate.yml * Add check-links * Update validate.yml * Add lighthouse and ignores for URLs * Add lighthouse config * Update validate.yml

krassowski changed the title ~~Add HTML validator action (#1)~~ Add HTML validator action Dec 29, 2021

Hash-pin html5validator-action

52a7964

choldgraf reviewed Dec 29, 2021

View reviewed changes

Try with nox

366e00d

krassowski force-pushed the add-ci-testing branch from 3cbe312 to 366e00d Compare December 29, 2021 20:39

Set all thresholds to 0.95, correct noxfile

4dee8f5

choldgraf reviewed Dec 29, 2021

View reviewed changes

noxfile.py Outdated Show resolved Hide resolved

noxfile.py Outdated Show resolved Hide resolved

Update names as suggested in review

d7f3474

krassowski added 2 commits December 29, 2021 21:14

Add back the checkout step

1621100

Revert to building in jekyll container for speed

6b84d67

Docker approach is way faster (1 minute vs 6.5 minutes)

Maybe we don't need to copy assets?

1e77c7d

Fix some links, ignore netlify-preview from README.md

dcd0d7f

krassowski mentioned this pull request Dec 29, 2021

Broken links #580

Closed

12 tasks

choldgraf reviewed Dec 29, 2021

View reviewed changes

Fix link check, simplify naming

b501e41

palewire mentioned this pull request Dec 29, 2021

Default orange color isn't accessible on a white background jupyter/design#67

Open

krassowski added 3 commits December 29, 2021 23:12

Add documentation, use absolute links in index

e62e6b0

Fix wrong extension for img fallback

2ec8a19

Two more places where absolute links needed

0f200aa

krassowski requested review from choldgraf and palewire December 29, 2021 23:33

Fix jpg -> png in two more places

b84786c

palewire approved these changes Dec 29, 2021

View reviewed changes

This was referenced Dec 30, 2021

Enable Lighthouse CI App integration for nicer checks display? #582

Open

Try to run jekyll build in test with nox, mamba and aggressive caching #583

Open

choldgraf merged commit 485005d into jupyter:master Dec 30, 2021

krassowski deleted the add-ci-testing branch December 30, 2021 00:22

krassowski mentioned this pull request Jan 1, 2022

Add accessibility tests to PR review jupyterlab/jupyterlab#9742

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add HTML validator action #579

Add HTML validator action #579

krassowski commented Dec 29, 2021 •

edited

Loading

krassowski commented Dec 29, 2021

choldgraf left a comment

choldgraf Dec 29, 2021

choldgraf left a comment

krassowski commented Dec 29, 2021

choldgraf commented Dec 29, 2021

krassowski commented Dec 29, 2021

choldgraf commented Dec 29, 2021

krassowski commented Dec 29, 2021

choldgraf commented Dec 29, 2021

krassowski commented Dec 29, 2021

choldgraf commented Dec 29, 2021

krassowski commented Dec 29, 2021 •

edited

Loading

choldgraf left a comment

choldgraf Dec 29, 2021

krassowski Dec 29, 2021

choldgraf Dec 29, 2021

krassowski Dec 29, 2021

choldgraf commented Dec 29, 2021 •

edited

Loading

palewire commented Dec 29, 2021

palewire commented Dec 29, 2021

palewire commented Dec 29, 2021

palewire commented Dec 29, 2021

krassowski commented Dec 29, 2021

choldgraf commented Dec 29, 2021

krassowski commented Dec 29, 2021

krassowski commented Dec 30, 2021

palewire commented Dec 30, 2021 •

edited

Loading

choldgraf commented Dec 30, 2021

	@nox.session(venv_backend='conda')
	def build(session):
	install_deps(session)
	session.run(*"bundle exec jekyll serve liveserve".split())

Add HTML validator action #579

Add HTML validator action #579

Conversation

krassowski commented Dec 29, 2021 • edited Loading

krassowski commented Dec 29, 2021

choldgraf left a comment

Choose a reason for hiding this comment

choldgraf Dec 29, 2021

Choose a reason for hiding this comment

choldgraf left a comment

Choose a reason for hiding this comment

krassowski commented Dec 29, 2021

choldgraf commented Dec 29, 2021

krassowski commented Dec 29, 2021

choldgraf commented Dec 29, 2021

krassowski commented Dec 29, 2021

choldgraf commented Dec 29, 2021

krassowski commented Dec 29, 2021

choldgraf commented Dec 29, 2021

krassowski commented Dec 29, 2021 • edited Loading

choldgraf left a comment

Choose a reason for hiding this comment

choldgraf Dec 29, 2021

Choose a reason for hiding this comment

krassowski Dec 29, 2021

Choose a reason for hiding this comment

choldgraf Dec 29, 2021

Choose a reason for hiding this comment

krassowski Dec 29, 2021

Choose a reason for hiding this comment

choldgraf commented Dec 29, 2021 • edited Loading

palewire commented Dec 29, 2021

palewire commented Dec 29, 2021

palewire commented Dec 29, 2021

palewire commented Dec 29, 2021

krassowski commented Dec 29, 2021

choldgraf commented Dec 29, 2021

krassowski commented Dec 29, 2021

krassowski commented Dec 30, 2021

palewire commented Dec 30, 2021 • edited Loading

choldgraf commented Dec 30, 2021

krassowski commented Dec 29, 2021 •

edited

Loading

krassowski commented Dec 29, 2021 •

edited

Loading

choldgraf commented Dec 29, 2021 •

edited

Loading

palewire commented Dec 30, 2021 •

edited

Loading