add support for \ character in pytest temporary path - Closes #982
#985
Conversation
bhelgs
force-pushed
the
bad_tmp_path
branch
2 times, most recently
from
809dc4c to
4734c6b
Compare
|
🤔 |
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #985 +/- ##
==========================================
+ Coverage 95.26% 95.83% +0.57%
==========================================
Files 13 13
Lines 401 432 +31
==========================================
+ Hits 382 414 +32
+ Misses 19 18 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
bhelgs
force-pushed
the
bad_tmp_path
branch
2 times, most recently
from
e6768eb to
9681a5a
Compare
tests/test_postgresql.py
Outdated
| return tmp_path_factory.mktemp(rf"bad\path/{subdir}") | ||
|
|
||
| with mock.patch("pytest_postgresql.factories.process._tmp_dir", side_effect=bad_path): | ||
| assert postgresql_proc.running() is True |
There was a problem hiding this comment.
Hmmmm... looking at the test here. Executor at this stage is already started, it is running when you run first test with postgresql_proc.
It would be better I think to add test in test_executors, and instantiate the PostgreSQL executor there directly.
making the function _tmp_dir obsolete.
There was a problem hiding this comment.
good point. Thanks a lot! Done.
The new test has quite a bit of duplicate with the test_executor_init_with_password test, including the skipif.
It may be reasonable to refactor some more. I'm just not sure what to do yet.
There was a problem hiding this comment.
If you want I can open a new pull request to refactor.
This change damages test encapsulation, but at least I think it might resolve the skipif issue and maybe reduce test maintainance.
There was a problem hiding this comment.
I was just going to say that I did deduplicated these tests when fixing #343, but I was unable to duplicate the issue in tests, and just reverted any changes I made to tests 🤔
Refactor looks good to me, note that port_for already contains PortType definition you can import and use.
There was a problem hiding this comment.
Sorry, I was on vacation and then got a bit distracted.
I'll treat the refactor as a separate PR.
Also, I was able to reproduce and add a test for what you fixed in PR #965. So I'll rebase this PR on top of the refactor.
What I did was add (nl_NO.UTF-8) and confirm it fails without the fix you made in PR #965:
`
(Not all languages encounter the issue that you fixed)
There was a problem hiding this comment.
No worries, I do get distracted as as well.
Got three distractions distracting me around the house ;)
Not all languages encounter the issue that you fixed
I wasn't expecting that 🤦🏻
bhelgs
force-pushed
the
bad_tmp_path
branch
2 times, most recently
from
c0d04f8 to
802a667
Compare
|
I have rebased this change on top of our other recent updates. The test confirms the path can contain backslashes, "\", as well as spaces, " ". |
Description
add support for
\character in pytest temporary path - Closes #982Testing:
You can see I have broken the PR into two commits.
In the first commit the new test fails on a "good" machine (XFAIL):
The second commit adds the fix. The tests PASSED on both "good" and "bad" machines:
Chore
Chore that needs to be done:
pipenv run towncrier create [issue_number].[type].rstP.S.
I noticed flake8 is failing in the pre-commit hook.
I'll assume I should be using ruff now and the hook is just out of date..