[KED-2443] Update husky hooks

[tynandebold]

Description

We use Husky, and this PR should make it work to our advantage a little more.

Development notes

Before this PR, we would only run a pre-commit hook that would lint all our files, which is great. With this change, we'll now also run a pre-push hook that will run our CI tests locally: npm run test:ci.

When trying to push your changes, all our tests will run locally, and if anything you were working on breaks any of the tests, you'll know then and there and the push to GitHub won't happen.

This will move the failure point earlier in the development cycle. Before this change, if you didn't run all the test locally, you'd only know if they failed when CircleCI ran the jobs. This should prevent some unnecessary runs of those jobs.

You could argue this will slow down development, though I disagree. Running the tests locally when pushing takes approximately 30 seconds. Running our CircleCI jobs takes 7-10 minutes. This should actually save time.

QA notes

Checkout this branch, and then

1. Change any test file to something that will make the test fail.
2. Commit the file.
3. Try and push your change. The push should fail.

Checklist

• Read the contributing guidelines
• Opened this PR as a 'Draft Pull Request' if it is work-in-progress
• Updated the documentation to reflect the code changes
• Added new entries to the RELEASE.md file
• Added tests to cover my changes

[studioswong]

Thanks for setting this up, good thinking.

I just tested this with trying to push a change with a failed test but the husky hook didn't seem to work ( i.e I was able to push to remote with the changes of the failed test and no notification of the failed test). The branch that I was able to push was test-husky-hooks, failed CI of this branch here.

On another note, for any new practice introduced in the development workflow, we should update our contributing docs -please add a note about the pre-push hook there. ( we might not have a section set up for the husky hooks, which this would be a good chance to set that up.)

[tynandebold]

Thanks for setting this up, good thinking.

I just tested this with trying to push a change with a failed test but the husky hook didn't seem to work ( i.e I was able to push to remote with the changes of the failed test and no notification of the failed test). The branch that I was able to push was test-husky-hooks, failed CI of this branch here.

On another note, for any new practice introduced in the development workflow, we should update our contributing docs -please add a note about the pre-push hook there. ( we might not have a section set up for the husky hooks, which this would be a good chance to set that up.)

This change works for me on your branch. See here:

I checked out your branch and ran git push and i wasn't able to push because the test was failing. Can you add some more details here about what you're seeing on your end please?

Update: can confirm it's working as it should for Antony. Something odd might be happening on your end. You may need to uninstall and then reinstall husky.

[studioswong]

The commits that I ran is via VS Code source control UI.

I've tried this via command line and the husky hooks did ran, so it is down to the commit via the VSCode source control UI.

I've tested this again via the VS Code controls and was able to commit with failed tests - see below commits.

Perhaps you can try it with the VSCode source control UI?

The fact that I was indeed able to commit to remote with the VS Code UI still suggests that it is not working as it should ( as I understand that the husky hooks should work regardless of commit tools.) I am guessing we might need further config to ensure it gets enforced?

Update:
I have uninstalled and reinstalled husky and was still able to commit with a failed JS test via VSCode source control without the husky hook running before my commit.

Again, the husky hook did ran on trying to commit via command line as you can see below, which suggests that my husky setup is right, just that it's not working on VSCode source

Update 2:
further digging around suggests that this is a common problem with other source control UIs too.

I reinstalled VSCode and the commit hooks works for me now, though the error message that it gives on VSCode wasn't entirely helpful 😅

In this case I am happy to approve after adding the mention in the contribution docs, it would be worth to also mention in the docs to restart VSCode or other source control UI (such as sourcetree) to ensure the newly added hook gets ran.

[antonymilne]

I've tried this out using the VS Code UI and also PyCharm UI (which is what I normally use). Both worked correctly for me (i.e. didn't push). In VS Code I got this popup which then shows the failed tests if I press "Open git log".

Is this smart enough to run the pre-push hooks only when I've changed relevant files? I know I can skip them with --no-verify if I want to, just wondering whether it always runs the entire JS test suite or whether it can work out which (if any) tests actually need to be run.

[tynandebold]

I've tried this out using the VS Code UI and also PyCharm UI (which is what I normally use). Both worked correctly for me (i.e. didn't push). In VS Code I got this popup which then shows the failed tests if I press "Open git log".

Is this smart enough to run the pre-push hooks only when I've changed relevant files? I know I can skip them with --no-verify if I want to, just wondering whether it always runs the entire JS test suite or whether it can work out which (if any) tests actually need to be run.

The way it's setup it'll just run the whole test suite. There's probably a way for it to only run relevant changes, but I don't think that's optimal, because you could make a change in a file and it'll mess something up elsewhere, and in this case the necessary checks might not run.

And since it only takes ~24 seconds (yes I timed it haha) to run all the tests locally I think this is ok.

[tynandebold]

@studioswong you say "was still able to commit with a failed JS test via VSCode source control" but that's not when this hook is supposed to run. It's supposed to run before you push, not before committing. Also, it ran correctly for me via VSCode's terminal.

This is an interesting thought though. We could easily run this before every commit. That seemed more intrusive to me though. What do you think?

cc @AntonyMilneQB

[studioswong]

@studioswong you say "was still able to commit with a failed JS test via VSCode source control" but that's not when this hook is supposed to run. It's supposed to run before you push, not before committing. Also, it ran correctly for me via VSCode's terminal.

This is an interesting thought though. We could easily run this before every commit. That seemed more intrusive to me though. What do you think?

cc @AntonyMilneQB

yea what I meant here was 'able to commit to remote' which means push - apologies if the wording was not explicit.

I do also think that it would work better to do the hook on commit rather than push - I won't worry about the annoyance of it as there's always a way to bypass the hook anyways, and it serves the purpose to avoid failed JS tests earlier on.

[tynandebold]

@studioswong you say "was still able to commit with a failed JS test via VSCode source control" but that's not when this hook is supposed to run. It's supposed to run before you push, not before committing. Also, it ran correctly for me via VSCode's terminal.
This is an interesting thought though. We could easily run this before every commit. That seemed more intrusive to me though. What do you think?
cc @AntonyMilneQB

yea what I meant here was 'able to commit to remote' which means push - apologies if the wording was not explicit.

I do also think that it would work better to do the hook on commit rather than push - I won't worry about the annoyance of it as there's always a way to bypass the hook anyways, and it serves the purpose to avoid failed JS tests earlier on.

Cool. I've moved it to the pre-commit hook instead and updated the contributing file.

I don't think we need anything in there about restarting applications because we already have the pre-commit hook in place, and this is just adding onto it, so it should work fine.

[antonymilne]

Hmm actually I think making this a pre-commit hook does seem a bit disruptive - pre-push seems like a more sensible cadence to me. If I'm just making changes to the backend or some documentation I don't really want to wait 20s to rerun all the JS tests every time I do a commit

But given that most of the development team/work at the moment is frontend stuff, if doing this with pre-commit makes more sense for you guys then I don't mind since I can just --no-verify to skip it. Overall we want something that speeds up development for the team as a whole, so go for whatever you think that would be.

[tynandebold]

Hmm actually I think making this a pre-commit hook does seem a bit disruptive - pre-push seems like a more sensible cadence to me. If I'm just making changes to the backend or some documentation I don't really want to wait 20s to rerun all the JS tests every time I do a commit

But given that most of the development team/work at the moment is frontend stuff, if doing this with pre-commit makes more sense for you guys then I don't mind since I can just --no-verify to skip it. Overall we want something that speeds up development for the team as a whole, so go for whatever you think that would be.

This is a great point and makes me want to revert back to pre-push. This shouldn't add overhead to people who don't need it, and having it on pre-commit will probably do that. Also, as we scale the team, it will only magnify this small annoyance.

I'm going to move back to pre-push and then we can sit with it for a while and see what we think.

@AntonyMilneQB could you please approve/request changes?

[antonymilne]

This is a great point and makes me want to revert back to pre-push. This shouldn't add overhead to people who don't need it, and having it on pre-commit will probably do that. Also, as we scale the team, it will only magnify this small annoyance.

I'm going to move back to pre-push and then we can sit with it for a while and see what we think.

@AntonyMilneQB could you please approve/request changes?

Cool, sounds good.

In a similar way I was wondering previously why we don't use the Python pre-commit to enforce some linting and code formatting standards (which are already checked on CI but not locally). Lim said that is was because it just introduced another tool which confused things, and that while it was useful for backend development it slowed down frontend development (not because of longer commit times but because it often failed commits since frontend developers didn't always keep their Python environments up to date). Possibly if the balance of the team shifts we should reassess this in future too, but at the moment it's fine.

[limdauto]

+1 to pre-push. Agree pre-commit would be too disruptive. 30s is half of a tiktok video :D

[studioswong]

looks good, I am no fuss for pre-commit or pre-push hooks - having pre-commit hooks helps force commits to be more thoughtful and complete, while pre-push hooks does the job to avoid failing CI, which is what we set out to do here. We can trial this out and revisit later.

Thanks for adding the mention in the docs.

[studioswong]

Suggested change

	Lastly, we have in place a pre-commit and pre-push hook. Before committing, the pre-commit hook will lint and prettify your changed files. Before pushing those committed changes, the pre-push hook will run our full test suite. This ensures the local changes haven't caused any breakages, and if they have, you'll be notified and can remedy then and there (note: you may need to restart your code editor or source control application for these hooks to work properly).
	Lastly, we have in place a pre-commit and pre-push hook. Before committing, the pre-commit hook will lint and prettify your changed files. Before pushing those committed changes, the pre-push hook will run our JS test suite. This ensures the local changes haven't caused any breakages, and if they have, you'll be notified and can remedy then and there (note: you may need to restart your code editor or source control application for these hooks to work properly).