ryanluker / vscode-coverage-gutters

Display test coverage generated by lcov and xml - works with many languages
https://marketplace.visualstudio.com/items?itemName=ryanluker.vscode-coverage-gutters
MIT License
460 stars 88 forks source link

Add husky pre-commit hook #314

Closed mattseddon closed 3 years ago

mattseddon commented 3 years ago

This PR adds a Husky as promised here.

The hook runs tslint --fix against all staged files before allowing a commit to be processed (if tslint cannot fix the issues in the staged file then the commit will not proceed).

I initially could not get tests to run from the command line inside of the devcontainer. There are further changes required to get that part of the hook to run and I felt that stopping here and asking the question as to whether or not it is desirable to run the tests in the pre-commit hook would be the best way to proceed.

I looked into the contributing.md and did not see any need to mention the new hook. I have however added the prepare script to the devcontainer so that the hooks are installed after it is built. Again, happy to change if you see things differently 👍🏻.

LMK if you would like to see any changes or additions to this PR 👍🏻. Happy to raise a follow up to add the tests in, if desired.

mattseddon commented 3 years ago

I have managed to get the tests running headless from the command line inside the container using the recommendations for setting up a Gitlab CI. I am pretty sure that I could get test running in the pre-commit hook for both container and non-container dev.

@ryanluker WDYT? Is there enough value in me adding the test suite into the pre-commit hook?

ryanluker commented 3 years ago

@mattseddon Thanks for the PR on the pre-commit hook! I am not sure if we want the tests to run with each commit as that might prove pretty long time wise eventually and reduce the developer feedback loop. I think the CICD run is good enough when someone opens up a PR. I also had to disable the tests for the linux CI pipeline as it had issues running in github actions 🤔. Your vscode link to the CI documentation might give us some insight into if we can re-enable linux CI testing though.

https://code.visualstudio.com/api/working-with-extensions/continuous-integration#gitlab-ci https://github.com/ryanluker/vscode-coverage-gutters/runs/2214395226?check_suite_focus=true

mattseddon commented 3 years ago

@mattseddon Thanks for the PR on the pre-commit hook! I am not sure if we want the tests to run with each commit as that might prove pretty long time wise eventually and reduce the developer feedback loop. I think the CICD run is good enough when someone opens up a PR. I also had to disable the tests for the linux CI pipeline as it had issues running in github actions 🤔. Your vscode link to the CI documentation might give us some insight into if we can re-enable linux CI testing though.

https://code.visualstudio.com/api/working-with-extensions/continuous-integration#gitlab-ci https://github.com/ryanluker/vscode-coverage-gutters/runs/2214395226?check_suite_focus=true

@ryanluker no worries, I am happy to leave tests out of the hook.

I have pinned the new dependencies as requested.

I'll look into fixing up the linux test suite (devcontainer / CI/CD pipeline) next weekend 👍🏻.

ryanluker commented 3 years ago

@mattseddon Good discussion, thanks a ton for the contributions!

ryanluker commented 3 years ago

@mattseddon Just used husky locally for a PR I am working on went well! I needed to do a clean npm install though as husky tried to run in the prepare but it wasn't installed yet 🤔 .

mattseddon commented 3 years ago

@mattseddon Just used husky locally for a PR I am working on went well! I needed to do a clean npm install though as husky tried to run in the prepare but it wasn't installed yet 🤔 .

😢 that's always the way with adding new dependencies. Sorry.

ryanluker commented 3 years ago

@mattseddon haha no need to feel sorry, I have had this happen at work a bit when adding new pre-commit type stuff so I was just wondering if it was expected.