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 <24376333+stefmolin@users.noreply.github.com>
rossbar
force-pushed
the
include-hook-tests-in-pkg
branch
from
157aac6 to
0f38d76
Compare
Co-authored-by: Matt Gebert <Mattheuu.Gebert@gmail.com>
|
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.
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.