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

CI: verify docs resources #235

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

RobinKnipe
Copy link
Collaborator

@RobinKnipe RobinKnipe commented Jun 6, 2021

Check the docs app to ensure all the links and assets resolve correctly.

There is a local check that brings up the docs in SSR mode and works with the usual runners:

  • npm test
  • make test

Extra steps have also been added to the deploy-docs GitHub action workflow to check the generated Netlify preview.

Check the docs app to ensure all the links and assets resolve correctly.
The docs app runs in SSR mode to ensure the fully rendered page contents
can be tested.
Improve the script run by `jq` to simplify the test, remove the need for
the `tee` command and the `.missing-links` local cache file.
Check the docs app preview deployed to Netlify, to ensure all the links
and assets resolve correctly.
@@ -0,0 +1,28 @@
#!/bin/bash +x
Copy link
Owner

Choose a reason for hiding this comment

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

Do we still need the +x?

Copy link
Owner

Choose a reason for hiding this comment

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

Is it better to do #! /bin/env bash these days?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question! Not really a short answer, but I think it boils down to using /bin/env is more portable but slightly less secure. I don't think either is a strong enough consideration to influence this context though.

@@ -0,0 +1,28 @@
#!/bin/bash +x

set -e
Copy link
Owner

Choose a reason for hiding this comment

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

-euo pipefail?

Copy link
Owner

Choose a reason for hiding this comment

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

Oh maybe not, you disable this later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, there's more than one thing running here, so the exit code needs to be set after clean-up, and reflect the outcome of the test process.


[ -d node_modules ] && [ -f dist/server/index.js ] || make deps build > /dev/null
set +e
npm run ssr > /dev/null &
Copy link
Owner

Choose a reason for hiding this comment

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

dev:ssr?

Or even SSR_ONLY=true npm start?

Copy link
Owner

Choose a reason for hiding this comment

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

Oh I see, you added a script for this.

@@ -6,12 +6,13 @@
"main": "dist/server/index.js",
"scripts": {
"start": "node .",
"test": "echo \"Error: no test specified\" && exit 1",
"test": "exec ./check-links.sh",
Copy link
Owner

Choose a reason for hiding this comment

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

Why the exec?

@daniel-ac-martin
Copy link
Owner

I agree with this in principle. I really want something like this.

I think it should be solved more generally though. i.e. All new apps should get this sort of basic acceptance testing out of the box.

I've been thinking of this as a subset of the functional tests (that don't yet exist!) so I was thinking of having something that runs inside of cypress.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants