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

devops: adds test action; makes run-all-checks exit with proper status code #150

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mattxwang
Copy link
Collaborator

This PR does two things:

  • sets up an action to run run-all-checks.sh after building the site
  • adjusts run-all-checks.sh to exit with the correct status code depending on the status of the tests

As a note, run-all-checks.sh takes a pretty non-trivial amount of time; I also think that some of these scripts could be done using rake and going directly into the Ruby tool, rather than through a shell script; but, that's out of scope for this PR!

Part of #92.

@mattxwang mattxwang requested a review from ctrueden June 4, 2021 23:10
@mattxwang mattxwang changed the title Adds test action; makes run-all-checks exit with proper status code devops: adds test action; makes run-all-checks exit with proper status code Jun 4, 2021
@mattxwang
Copy link
Collaborator Author

Notes:

  • script run time is > 13 minutes; I think this is not amazing for CI
  • currently CI fails because the script fails; we probably only want to merge this in after all those problems are done

@ctrueden
Copy link
Member

ctrueden commented Jun 6, 2021

I also think that some of these scripts could be done using rake and going directly into the Ruby tool, rather than through a shell script; but, that's out of scope for this PR!

I like shell scripts, and I don't know Ruby/rake too much. What would the technical advantage be? Performance?

script run time is > 13 minutes; I think this is not amazing for CI

I agree that this is highly suboptimal. We may need to ditch htmlproofer on CI in favor of some more-targeted-but-faster internal link checking. Any ideas for a good candidate? If not, rolling our own wouldn't be that difficult.

@ctrueden
Copy link
Member

ctrueden commented Jun 6, 2021

For now, maybe just disable the check-site-html.sh part of run-all-checks.sh? Does the script succeed in that case? If so, we could then get this merged sooner rather than later, and subsequently work toward a better link checking mechanism as part of #50.

@mattxwang
Copy link
Collaborator Author

mattxwang commented Jun 6, 2021

Just did the suggested change of commenting out check-site-html.sh (and fixed a mistake I made), and the check passes in much more reasonable time. Agree that it might be good to merge now and resolve the issues with that later! (we should squash-merge this change).

My mistake, the shell script was incorrect. Just updated it; we have two failing checks, a small one on GitHub usernames and some shenanigans with the includes check. I'm not super familiar with these, @ctrueden do you have any pointers on what I should do here?

I like shell scripts, and I don't know Ruby/rake too much. What would the technical advantage be? Performance?

I would think that we'd get significant performance benefits - afaik (not entirely sure about this) running HTMLProofer from Ruby gives you parallelism benefits (currently the shell script runs it independently on every single file, in a blocking fashion), and we skip a bunch of syscalls/context switches into shell. But, this is also not my area of expertise! We also get some type benefits (i.e. only passing in valid parameters to the object).

We may need to ditch htmlproofer on CI in favor of some more-targeted-but-faster internal link checking. Any ideas for a good candidate? If not, rolling our own wouldn't be that difficult.

I haven't had to do link-checking for a project on this scale before, but my understanding is that HTMLProofer is one of the better Ruby alternatives. Maybe we should first explore running it from Ruby + tweaking the options? If things don't work out, I can help look into alternatives.

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

Successfully merging this pull request may close these issues.

2 participants