-
Notifications
You must be signed in to change notification settings - Fork 26
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
Adding pre-commit configuration #23
base: master
Are you sure you want to change the base?
Conversation
- id: mixed-line-ending | ||
args: [--fix, lf] | ||
- id: name-tests-test | ||
args: [--django] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
args: [--django]
looks like a copy and paste error, since all the test files are named *Test.py
and not test*.py
as --django assumes.
Anyhow, the default option would not work as well. See https://github.com/pre-commit/pre-commit-hooks#name-tests-test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, as long as the naming convention of test files is fixed, the testrunner can then find them using the -p PATTERN
argument during unit test discovery. The default is test*.py
and the --django
option also matches test*.py
. That was why I chose this. But the default name-tests-test
can also be used by just providing the -p *_test.py
argument to the unit test command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would have to potentially rename the tests once this decision is made.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
I believe that before enabling precommit
test themself should be changed.
As majority of tests encompass the whole learning procedure, few dozen times. And that can be quite time consuming, especially for stochastic learning. That is on a TODO list for some time now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can keep the procedure tests, those can be useful, but separate them from the fast unit tests.
Then you can have both: quick checks on commit and extensive functionality tests when you feel like it or when you make a big change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea.
It will be like that then. Now "only" thing left to do is to write unit tests 👯
Hi there,
after issue #22 I thought an easy way of preventing pushing such errors to pypi would be to automatically run the unittests on every commit.
This can either be done using CI/CD using a GitHub ci-cd pipline which could be run after every commit by GitHub in the cloud.
Alternatively, they can be run locally before every commit. A great tool to do this easily is pre-commit.
It allows to install pre-commit hooks into your local git infrastructure to run any number of commands before any commit.
If any of them fail, the commit will be aborted and the issue can be resolved locally.
This PR adds a basic pre-commit configuration including checking:
python -m unittest discover
.To make them discoverable, I added a dunder-init file in tests.
However, at the moment the unittests use relative file paths that break when running them from the project root folder.
Additionally, they are pretty slow. If unittests should be run on every commit, the tests would have to be faster than they currently are, or alternatively be curated to only include fast ones.
Please check this out before accepting this PR!
Fast setup for pre-commit:
Install it by using
pip install pre-commit
.Then type
pre-commit install
to install the hooks them into the local git repository.Run the hooks on the entire project by using
pre-commit run -a
.By default, commits will only run on changed files instead of the entire project.
You can manually run the hooks on only changed files using
pre-commit run
.