Bugfix/fix failing tests on windows due to relpath #8094
Conversation
… points to a different drive then the current working directory.
mypy/build.py
Outdated
|
|
||
| except ValueError: | ||
| return os.path.abspath(path) | ||
|
|
||
| else: |
There was a problem hiding this comment.
This is probably the most critical workaround, since I think this would not work with bazel (when I read the comment above correctly). However, is bazel not Linux only? Could we simple skip the corresponding testcase on Windows systems?
There was a problem hiding this comment.
Bazel supports Windows these days, though I'd expect that nobody has tried running mypy under Bazel on Windows. Maybe it's fine to require that everything is on a single drive when using Bazel on Windows. Which test case is failing without this change? It's probably fine to skip that test case on Windows.
There was a problem hiding this comment.
The failing test is testBazelFlagIgnoresFileChanges .
This is the error:
____________________________________________________________________________________________________________________________ testBazelFlagIgnoresFileChanges ____________________________________________________________________________________________________________________________
[gw5] win32 -- Python 3.7.4 d:\projects\python\stars\dependencies\mypy\mypy-python\scripts\python.exe
data: d:\Projects\Python\stars\dependencies\mypy\main\test-data\unit\check-incremental.test:4121:
d:\Projects\Python\stars\dependencies\mypy\main\mypy\test\testcheck.py:126: in run_case
self.run_case_once(testcase, ops, step)
d:\Projects\Python\stars\dependencies\mypy\main\mypy\test\testcheck.py:227: in run_case_once
self.verify_cache(module_data, res.errors, res.manager, res.graph)
d:\Projects\Python\stars\dependencies\mypy\main\mypy\test\testcheck.py:266: in verify_cache
missing_paths = self.find_missing_cache_files(modules, manager)
d:\Projects\Python\stars\dependencies\mypy\main\mypy\test\testcheck.py:297: in find_missing_cache_files
if not build.validate_meta(meta, id, path, ignore_errors, manager):
d:\Projects\Python\stars\dependencies\mypy\main\mypy\build.py:1254: in validate_meta
path = normpath(path, manager.options)
d:\Projects\Python\stars\dependencies\mypy\main\mypy\build.py:269: in normpath
return os.path.relpath(path)
D:\Programming\Python\Python_3.7.4\lib\ntpath.py:562: in relpath
path_drive, start_drive))
E ValueError: path is on mount 'd:', start on mount 'C:'
================================================================================================================ 1 failed, 7910 passed, 236 skipped in 137.68s (0:02:17) ================================================================================================================
What would be a better solution: Fallback to absolute path or let the Exception happen and skip the test?
There was a problem hiding this comment.
Let's skip the test on Windows (but please add comment describing why).
mypy/report.py
Outdated
| @@ -123,6 +123,8 @@ def should_skip_path(path: str) -> bool: | |||
| return True | |||
| if 'stubs' in path.split('/') or 'stubs' in path.split(os.sep): | |||
| return True | |||
| if 'lib-stub' in path.split('/') or 'lib-stub' in path.split(os.sep): | |||
| return True | |||
| return False | |||
There was a problem hiding this comment.
I guess, that on a normal setup (both paths are on the same drive), the relative path would start with ..., which leads to a True in line 123. Due to my workaround the path is now absolute, so it would not work anymore. However is it semantically wrong to ignore lib-stub? What do you think?
There was a problem hiding this comment.
Skipping lib-stubs is probably fine.
|
|
||
| except ValueError: | ||
| path = tree.path | ||
|
|
There was a problem hiding this comment.
What is the semantic reason for making the path relative? When I understand the code correctly a relative path outside of the directory where the code-under-check is in, should be ignored anyway (see should_skip_path case ...)?
So a final solution could be to simply return from this function in case of a ValueError? Or to mark the resulting path as invalid and skip it with should_skip_path. What do you think?
There was a problem hiding this comment.
We use relative paths since the paths could be included in the output and we want the output to be tidy and not depend on the current working directory. I agree that if we can't determine a relative path, we can just skip and return (both here and above).
There was a problem hiding this comment.
I solved it by returning directly from the exception handler. Another alternative would be to mark the path as invalid (None or empty string for example) and skip then from the function should_skip_path(...). However this would introduce special state for path, which is not obvious, especially in cases where the function should_skip_path(...) is not used (in future). On the other hand with the current solution there are two logics to skip: the official one from should_skip_path(...) and the new one from the exception handler.
Just decide by yourself, which solution you like more. I can change it another time, if you want.
mypy/report.py
Outdated
| @@ -123,6 +123,8 @@ def should_skip_path(path: str) -> bool: | |||
| return True | |||
| if 'stubs' in path.split('/') or 'stubs' in path.split(os.sep): | |||
| return True | |||
| if 'lib-stub' in path.split('/') or 'lib-stub' in path.split(os.sep): | |||
| return True | |||
| return False | |||
There was a problem hiding this comment.
Skipping lib-stubs is probably fine.
|
|
||
| except ValueError: | ||
| path = tree.path | ||
|
|
There was a problem hiding this comment.
We use relative paths since the paths could be included in the output and we want the output to be tidy and not depend on the current working directory. I agree that if we can't determine a relative path, we can just skip and return (both here and above).
mypy/version.py
Outdated
| @@ -5,7 +5,7 @@ | |||
| # - Release versions have the form "0.NNN". | |||
| # - Dev versions have the form "0.NNN+dev" (PLUS sign to conform to PEP 440). | |||
| # - For 1.0 we'll switch back to 1.2.3 form. | |||
| __version__ = '0.750+dev' | |||
| __version__ = '0.750' | |||
There was a problem hiding this comment.
This change should not be here.
mypy/build.py
Outdated
|
|
||
| except ValueError: | ||
| return os.path.abspath(path) | ||
|
|
||
| else: |
There was a problem hiding this comment.
Bazel supports Windows these days, though I'd expect that nobody has tried running mypy under Bazel on Windows. Maybe it's fine to require that everything is on a single drive when using Bazel on Windows. Which test case is failing without this change? It's probably fine to skip that test case on Windows.
|
@JukkaL Thanks for your feedback. In the next days I will prepare another version of the pull-request, which contains your suggestions. Can you give me in the meanwhile three other hints?
Thanks for your feedback and also thanks for helping me here. |
|
| @@ -221,7 +221,7 @@ This submodule contains types for the Python standard library. | |||
|
|
|||
| Due to the way git submodules work, you'll have to do | |||
| ``` | |||
| git submodule update typeshed | |||
| git submodule update mypy/typeshed | |||
There was a problem hiding this comment.
By accident I saw that the original command did not work.
|
Thanks a lot for your feedback and assistance. I implemented everything as discussed and all tests are working now. If you have any further suggestions, just tell me. Otherwise I would be happy if you can merge it into your baseline. Best regards, |
Hello
I tried to run all testcases of a fresh cloned repository (version 0.750) on windows 10. However several testcases where failing due to a uncommon combination between temporary path and repository path.
os.path.relpath(path)works on windows only in case that the start path (os.getcwd() as default) and the given path are on the same drive. In my case the temporary directory, where the test was executed was on drivec:, while the file which have been checked was in the repository on drived:Thus those testcases failed due to an ValueError exception fromos.path.relpath(...).Those errors look like that
It is clear for me, that my fixes here are just workarounds. However I want to start a discussion about that. Maybe we can find together a better implementation?
Best regards