utils: resolve_output: normpath before extracting basename

[pared]

• ❗ 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

[skshetry]

Suggested change

	import mock
	from unittest import mock

[skshetry]

Are we moving from mock to unittest.mock? cc @efiop

But, it'd be good to not introduce here?

[efiop]

@pared Or even better, just use mocker fixture, that would be the most correct way to go about it.

[efiop]

@skshetry Even better, we are moving to pytest, so should simply use mocker fixture. Great catch!

[skshetry]

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 above

You don't have to change anything, I'm just stating my preference. I'm fine as is as well.

[pared]

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.

[pared]

I would also say that many with statements one under another kind of obscures what the test is about.

[ghost]

@pared , why you didn't add the test that you wrote on the issue?

[ghost]

🤔 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.

[ghost]

good catch, @pared 👍

[pared]

why you didn't add the test that you wrote on the issue?

@MrOutis when I noted that the problem is with resolving the output, it came to my mind that this particular bug is not about get, but only about resolving the output. Removing functionality worked properly, so I thought there is no need to make a functional test.