Adds pre-commit hook that runs automated tests & fixes lint errors#6558
Adds pre-commit hook that runs automated tests & fixes lint errors#6558Exairnous merged 1 commit intoHubs-Foundation:masterfrom
Conversation
a472f40 to
32e5b34
Compare
There was a problem hiding this comment.
I've only done a slight bit of QA testing and still have most of that left to do, but here are some things from a mostly visual inspection.
- This needs documentation that it is present, how to manually trigger it, and that
-ncan be used to bypass the hook. This should probably be added to the readme. - This appears to test everything (including untracked files) instead of only what's been staged for commit (ideally, only the staged changes should be tested/linted - for reference, the Spoke equivalent of this hook only works with the staged changes).
- The formatting changes present in this conflict with the formatting changes in #6559 and I kinda think it would be better to merge #6559 and then rebase this on top of it.
- I'm not sure, but given the hook is just a bash script I think it could potentially have issues on Windows. Has this been tested?
Should Husky be removed by this PR?
As far as I know, Husky isn't currently a dependency of this repository, so there's nothing to remove (at least I didn't find any sign of it in the package-lock files with a quick search).
Should we move this to the pre-push hook, which is run less often?
Not sure. My first thought is that if we're going to be enforcing this, then we should probably enforce it for every commit, but having it run before pushing would still be an improvement (it would probably result in a number of cleanup commits which wouldn't happen if it were required for every commit).
f02d64a to
b77ed54
Compare
…that runs automated tests. Why: There is no native way to include Git hooks in the checked-in code. All solutions require either running a command or installing a 3rd party package. As Hubs developers will almost always run `npm install` after cloning the repo, this approach requires no change in development steps. Note that developers can always prevent a hook from running by adding the flag `-n`, so git hooks just support fail-fast. Enforcement has to occur in CI.
b77ed54 to
5b18adc
Compare
Done.
Done.
Done.
We advise Windows users to use WSL |
Exairnous
left a comment
There was a problem hiding this comment.
I have finished QA testing with the latest changes and it looks good to me. Thanks. Merging.
What?
After an npm command, configures git to run hooks in .githooks. Adds pre-commit hook that runs automated tests.
Fixes lint errors from several PRs
Why?
There is no native way to include Git hooks in the checked-in code.
All solutions require either running a command or installing a 3rd party package.
As Hubs developers will almost always run
npm installafter cloning the repo, this approach requires no change in development steps.npm run testwas not run before the commits that introduced lint errors, and there was no Git pre-commit hook.How to test
npm cito activate the new hookgit addgit commit— observe that tests are run, an error message is displayed, and the commit does not completegit addgit commit— observe that tests are run and the commit does completeDocumentation of functionality
No changes to docs, this just pushes validation to the left
Limitations
Note that developers can always prevent a hook from running by adding the flag
-n, so git hooks just support fail-fast.Enforcement has to occur in CI.
The pre-commit script also checks two things that are not currently enforced by anything:
Current code follows these rules, and they appeared good checks to add.
They could be removed if/when they get in the way.
Alternatives considered
Husky is an extra dependency and doesn't appear to allow any additional functionality of use.
Open questions
Should Husky be removed by this PR?
The tests take a noticeable amount of time to run, which can be annoying when updating a commit.
Should we move this to the pre-push hook, which is run less often?