utils: resolve_output: normpath before extracting basename #3170
Conversation
tests/unit/utils/test_utils.py
Outdated
| @@ -1,10 +1,11 @@ | |||
| import re | |||
| import os | |||
|
|
|||
| import mock | |||
There was a problem hiding this comment.
| import mock | |
| from unittest import mock |
There was a problem hiding this comment.
Are we moving from mock to unittest.mock? cc @efiop
But, it'd be good to not introduce here?
There was a problem hiding this comment.
@pared Or even better, just use mocker fixture, that would be the most correct way to go about it.
There was a problem hiding this comment.
@skshetry Even better, we are moving to pytest, so should simply use mocker fixture. Great catch!
tests/unit/utils/test_utils.py
Outdated
| ["dir", "other_dir", True, os.path.join("other_dir", "dir")], | ||
| ], | ||
| ) | ||
| def test_resolve_output(inp, out, is_dir, expected): |
There was a problem hiding this comment.
Personally, it's too hard for me to parse this matrix. I'd prefer to have it flat, for example:
with mock.patch("os.path.isdir", return_value=True):
assert resolve_output("target", "dir") == os.path.join("target", "dir")
# ... more asserts similar to above
with mock.patch("os.path.isdir", return_value=False):
assert resolve_output("target", None) == "target"
# ... more asserts similar to aboveYou don't have to change anything, I'm just stating my preference. I'm fine as is as well.
There was a problem hiding this comment.
I agree that it might be easier to read through "flat" version, but this matrix (in my opinion) makes it easier to compare particular cases. Also, If we have "flat" version, during test execution, in case of singular fail, we will only know that this test failed, while in case of parameterization we will know exactly which argument set has failed.
There was a problem hiding this comment.
I would also say that many with statements one under another kind of obscures what the test is about.
| @@ -336,7 +336,7 @@ def env2bool(var, undefined=False): | |||
| def resolve_output(inp, out): | |||
| from urllib.parse import urlparse | |||
|
|
|||
| name = os.path.basename(urlparse(inp).path) | |||
| name = os.path.basename(os.path.normpath(urlparse(inp).path)) | |||
There was a problem hiding this comment.
🤔 hard to know that it was originally wrong (before normpath). Wondering if there's a better way to handle this cases without thinking too much about correctness.
@MrOutis when I noted that the problem is with resolving the output, it came to my mind that this particular bug is not about |
❗ Have you followed the guidelines in the Contributing to DVC list?
📖 Check this box if this PR does not require documentation updates, or if it does and you have created a separate PR in dvc.org with such updates (or at least opened an issue about it in that repo). Please link below to your PR (or issue) in the dvc.org repo.
❌ Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addressed. Please review them carefully and fix those that actually improve code or fix bugs.
Thank you for the contribution - we'll try to review it as soon as possible. 🙏
Fixes #3105