MAINT: Add missing hooks tests. #653
Conversation
mattgebert
left a comment
mattgebert
left a comment
There was a problem hiding this comment.
While we're changing the test suite to make sure the hook tests run, I suggest we add --pre flag for the pre-release install in the testing CI test.yml.
* replace path separators on windows * Add drive to path for windows
rossbar
force-pushed
the
include-hook-tests-in-pkg
branch
from
fe4d9df to
417916e
Compare
Co-authored-by: Stefanie Molin <[email protected]>
rossbar
force-pushed
the
include-hook-tests-in-pkg
branch
from
157aac6 to
0f38d76
Compare
|
Apologies all - I was traveling the last couple weeks and didn't have time to get back to this. I've gone ahead and implemented @mattgebert 's and @stefmolin 's suggestions (thanks for the good ideas!) Note: I had to revert #655 in this PR as the changes in that PR materially altered the regex so that it was no longer matching file paths on windows. This wasn't caught in the CI for #655 because the hooks tests are not run in the CI (they will be after this PR). This should now be good to go, and should definitely go in before any modifications to the hooks are made to ensure that any changes are tested! |
|
No worries Ross, thanks for getting back to this! Very appreciative of all contributions.
Just bumping this ^ as well before someone with write access reviews so we can merge. |
Now that #655 is reverted, the warnings are back. Can we make sure those are addressed to not regress? 
|
Yes - I tried to figure this out but wasn't able to come up with a regex recipe that worked for the windows filesystems. If someone has a working recipe I'm happy to add it here, otherwise this should be tackled in a followup. |
There was a problem hiding this comment.
Now that #655 is reverted, the warnings are back. Can we make sure those are addressed to not regress?
Yes - I tried to figure this out but wasn't able to come up with a regex recipe that worked for the windows filesystems. If someone has a working recipe I'm happy to add it here, otherwise this should be tackled in a followup.
I'm thinking switching to re.escape() would work. Not easy to suggest changes for that here based on how the file is structured, and I don't have a Windows machine to test on, so I will just merge this for now.
Edit: This was indeed the solution. See #660.
….sep replacement Updates to make this PR compatible with the recent merge from numpy#653
….sep replacement Updates to make this PR compatible with the recent merge from numpy#653
* fix(validate.py): Considers subclass nesting when checking GL08 constructor Introduced GL08 checking for constructors (i.e. __init__) but didn't traverse the parent class heireacy when importing the __init__ parent from a module. * test(validate.py): Added a test to check nested class docstring when checking for initialisation docstring Test written due to bug missed in #592. * fix(validate.py): Allows the validator to check AST constructor docstrings compliance with parent class The abstract syntax tree doesn't provide any link to a parent class at node level, so special traversal is required to check constructor parent implementation of docstrings. * test(test_validate_hook.py,-example_module.py): Wrote new example_module tests for AST (AbstractSyntaxTree) discovery of constructor method parent class. * ci(test.yml): Added --pre option to prerelease job to ensure pre-release installation, and changed pytest invocation to use `numpydoc` instead of `.`, for consistency with `test` job In response to discussion on #591 * refactor(tests): Remove `__init__.py` module status of `tests\hooks\` to match `tests\` directory * ci(test.yml): Added explicit call to hook tests to see if included in pytest execution * test(tests\hooks\test_validate_hook.py): Changed constructor validation to use AST ancestry, fixed hook tests and added Windows support for test_utils * ci(test.yml): Added file existance check for hook tests Workflows do not seem to be running hook pytests. * ci(test.yml): Correct the workflow task name/version * ci(test.yml): Added explicit pytest call to the hooks directory * ci(test.yml): Removed file existance test, after explicit call to hooks verifies execution * fix(validate.py): switched conditional GL08 check order to avoid property class failure for Validator.name Solves Issue #638, though lacks property support for property class attributes. * test(test_validate.py): add coverage for existing / expected functionality of property and dataclass objects * test(test_validate_hook.py): modify test strings and remove unused os.sep replacement Updates to make this PR compatible with the recent merge from #653
3 participants
The
tests/hookstests are not being run on CI because the tests themselves are not included in the package. The--pyargspytest flag tells pytest to run the tests on the installed package, so since the hook tests are not included they are not run.