Skip to content

Add wbplanview unit tests and pre-commit hooks#792

Merged
maxpatiiuk merged 11 commits into
wb-upload-for-testingfrom
wbplanview-tests
Apr 16, 2021
Merged

Add wbplanview unit tests and pre-commit hooks#792
maxpatiiuk merged 11 commits into
wb-upload-for-testingfrom
wbplanview-tests

Conversation

@maxpatiiuk

Copy link
Copy Markdown
Member

Convert testmain.js to TypeScript
Add unit tests for functional wbplanview components
Add pre-commit hooks from pre-commit.com

@maxpatiiuk maxpatiiuk added impact:internal python javascript Pull requests that update Javascript code labels Apr 13, 2021
@maxpatiiuk
maxpatiiuk requested a review from benanhalt April 13, 2021 18:03
@maxpatiiuk maxpatiiuk self-assigned this Apr 13, 2021
Since `npm run test` runs TypeScript typechecker before running unit
tests, there is no need to check types once again.
Comment thread .pre-commit-config.yaml
# pre-commit run --files *.ts
#
# Or give it all files:
# pre-commit run --all-files

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a way to run it just for the output without updating any files?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't think there is such parameter.
One solution would be to run git stash && git stash apply to save a copy of the working tree into stash. Then, after running pre-commit you can recover the previous state by running git checkout stash -- .

@@ -0,0 +1,2 @@
#!/bin/bash
(cd specifyweb/frontend/js_src && npm run test)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this shell script get used anywhere?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I defined a local hook here:

entry: ./specifyweb/frontend/js_src/lib/tests/testmain.sh

All it needs is to run npm run test if any js or ts file was modified.

Since npm is a global command, rather than a local shell file, I wasn't able to get it to work nicely. So I created this local shell script that get's called by the hook.

Though, maybe there is a better way to do this.

@benanhalt

Copy link
Copy Markdown
Contributor

You can go ahead and merge this if you want. Thanks for adding the tests.

I'd like to wait before enabling the pre-commit hooks, however, until it is determined if the linting can be made less nit-picky. The way it is now it is going to pollute minor commits will all sorts of "fixes". If we make a commit to fix an issue that only changes one or two lines, but the linter changes 100s of lines, it becomes impossible to tell what changes were actually related to the fix. If it is possible to adjust the lint settings so that it mostly accepts the existing code base without much complaint, then it can be turned on.

@maxpatiiuk
maxpatiiuk merged commit 78ad5ab into wb-upload-for-testing Apr 16, 2021
@maxpatiiuk
maxpatiiuk deleted the wbplanview-tests branch April 18, 2021 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

impact:internal javascript Pull requests that update Javascript code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants