Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix/fix failing tests on windows due to relpath #8094

Conversation

@Netzeband
Copy link
Contributor

Netzeband commented Dec 5, 2019

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 drive c:, while the file which have been checked was in the repository on drive d: Thus those testcases failed due to an ValueError exception from os.path.relpath(...).

Those errors look like that

____________________________________________________________________________________________________________________________________ testReportBasic ____________________________________________________________________________________________________________________________________
[gw7] 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-reports.test:1:
SystemExit: 2
--------------------------------------------------------------------------------------------------------------------------------- Captured stdout call ----------------------------------------------------------------------------------------------------------------------------------
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "<string>", line 8, in <module>
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\execnet\gateway_base.py", line 1554, in serve
    SlaveGateway(io=io, id=id, _startcount=2).serve()
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\execnet\gateway_base.py", line 1060, in serve
    self._execpool.integrate_as_primary_thread()
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\execnet\gateway_base.py", line 267, in integrate_as_primary_thread
    self._perform_spawn(reply)
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\execnet\gateway_base.py", line 285, in _perform_spawn
    reply.run()
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\execnet\gateway_base.py", line 220, in run
    self._result = func(*args, **kwargs)
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\execnet\gateway_base.py", line 1084, in executetask
    do_exec(co, loc)  # noqa
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\xdist\remote.py", line 258, in <module>
    config.hook.pytest_cmdline_main(config=config)
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\pluggy\hooks.py", line 286, in __call__
    return self._hookexec(self, self.get_hookimpls(), kwargs)
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\pluggy\manager.py", line 93, in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\pluggy\manager.py", line 87, in <lambda>
    firstresult=hook.spec.opts.get("firstresult") if hook.spec else False,
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\pluggy\callers.py", line 187, in _multicall
    res = hook_impl.function(*args)
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\_pytest\main.py", line 239, in pytest_cmdline_main
    return wrap_session(config, _main)
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\_pytest\main.py", line 196, in wrap_session
    session.exitstatus = doit(config, session) or 0
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\_pytest\main.py", line 246, in _main
    config.hook.pytest_runtestloop(session=session)
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\pluggy\hooks.py", line 286, in __call__
    return self._hookexec(self, self.get_hookimpls(), kwargs)
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\pluggy\manager.py", line 93, in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\pluggy\manager.py", line 87, in <lambda>
    firstresult=hook.spec.opts.get("firstresult") if hook.spec else False,
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\pluggy\callers.py", line 187, in _multicall
    res = hook_impl.function(*args)
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\xdist\remote.py", line 70, in pytest_runtestloop
    self.run_one_test(torun)
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\xdist\remote.py", line 87, in run_one_test
    self.config.hook.pytest_runtest_protocol(item=item, nextitem=nextitem)
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\pluggy\hooks.py", line 286, in __call__
    return self._hookexec(self, self.get_hookimpls(), kwargs)
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\pluggy\manager.py", line 93, in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\pluggy\manager.py", line 87, in <lambda>
    firstresult=hook.spec.opts.get("firstresult") if hook.spec else False,
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\pluggy\callers.py", line 187, in _multicall
    res = hook_impl.function(*args)
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\_pytest\runner.py", line 81, in pytest_runtest_protocol
    runtestprotocol(item, nextitem=nextitem)
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\_pytest\runner.py", line 96, in runtestprotocol
    reports.append(call_and_report(item, "call", log))
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\_pytest\runner.py", line 182, in call_and_report
    call = call_runtest_hook(item, when, **kwds)
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\_pytest\runner.py", line 207, in call_runtest_hook
    lambda: ihook(item=item, **kwds), when=when, reraise=reraise
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\_pytest\runner.py", line 234, in from_call
    result = func()
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\_pytest\runner.py", line 207, in <lambda>
    lambda: ihook(item=item, **kwds), when=when, reraise=reraise
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\pluggy\hooks.py", line 286, in __call__
    return self._hookexec(self, self.get_hookimpls(), kwargs)
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\pluggy\manager.py", line 93, in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\pluggy\manager.py", line 87, in <lambda>
    firstresult=hook.spec.opts.get("firstresult") if hook.spec else False,
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\pluggy\callers.py", line 187, in _multicall
    res = hook_impl.function(*args)
  File "d:\projects\python\stars\dependencies\mypy\mypy-python\lib\site-packages\_pytest\runner.py", line 131, in pytest_runtest_call
    item.runtest()
  File "d:\Projects\Python\stars\dependencies\mypy\main\mypy\test\data.py", line 215, in runtest
    suite.run_case(self)
  File "d:\Projects\Python\stars\dependencies\mypy\main\mypy\test\testcheck.py", line 128, in run_case
    self.run_case_once(testcase)
  File "d:\Projects\Python\stars\dependencies\mypy\main\mypy\test\testcheck.py", line 195, in run_case_once
    alt_lib_path=test_temp_dir)
  File "d:\Projects\Python\stars\dependencies\mypy\main\mypy\build.py", line 167, in build
    sources, options, alt_lib_path, flush_errors, fscache, stdout, stderr, extra_plugins
  File "d:\Projects\Python\stars\dependencies\mypy\main\mypy\build.py", line 235, in _build
    graph = dispatch(sources, manager, stdout)
  File "d:\Projects\Python\stars\dependencies\mypy\main\mypy\build.py", line 2625, in dispatch
    process_graph(graph, manager)
  File "d:\Projects\Python\stars\dependencies\mypy\main\mypy\build.py", line 2934, in process_graph
    process_stale_scc(graph, scc, manager)
  File "d:\Projects\Python\stars\dependencies\mypy\main\mypy\build.py", line 3036, in process_stale_scc
    graph[id].finish_passes()
  File "d:\Projects\Python\stars\dependencies\mypy\main\mypy\build.py", line 2169, in finish_passes
    free_tree(self.tree)
  File "D:\Programming\Python\Python_3.7.4\lib\contextlib.py", line 130, in __exit__
    self.gen.throw(type, value, traceback)
  File "d:\Projects\Python\stars\dependencies\mypy\main\mypy\build.py", line 1899, in wrap_context
    yield
  File "d:\Projects\Python\stars\dependencies\mypy\main\mypy\build.py", line 2164, in finish_passes
    manager.report_file(self.tree, self.type_map(), self.options)
  File "d:\Projects\Python\stars\dependencies\mypy\main\mypy\build.py", line 789, in report_file
    self.reports.file(file, self.modules, type_map, options)
  File "d:\Projects\Python\stars\dependencies\mypy\main\mypy\report.py", line 83, in file
    reporter.on_file(tree, modules, type_map, options)
  File "d:\Projects\Python\stars\dependencies\mypy\main\mypy\report.py", line 464, in on_file
    path = os.path.relpath(tree.path)
  File "D:\Programming\Python\Python_3.7.4\lib\ntpath.py", line 562, in relpath
    path_drive, start_drive))
ValueError: path is on mount 'd:', start on mount 'C:'
Generated XML report: C:\<user-home>\AppData\Local\Temp\mypy-test-_h6ujr_a\out\index.xml
--------------------------------------------------------------------------------------------------------------------------------- Captured stderr call ----------------------------------------------------------------------------------------------------------------------------------
d:\Projects\Python\stars\dependencies\mypy\main\test-data\unit\lib-stub\typing.pyi: error: INTERNAL ERROR -- Please try using mypy master on Github:
https://mypy.rtfd.io/en/latest/common_issues.html#using-a-development-mypy-build
Please report a bug at https://github.com/python/mypy/issues
version: 0.750
d:\Projects\Python\stars\dependencies\mypy\main\test-data\unit\lib-stub\typing.pyi: : note: use --pdb to drop into pdb

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

Ivan Levkivskyi and others added 2 commits Nov 29, 2019
Ivan Levkivskyi
… points to a different drive then the current working directory.

except ValueError:
return os.path.abspath(path)

else:

This comment has been minimized.

Copy link
@Netzeband

Netzeband Dec 5, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@JukkaL

JukkaL Dec 6, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@Netzeband

Netzeband Dec 6, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@JukkaL

JukkaL Dec 6, 2019

Collaborator

Let's skip the test on Windows (but please add comment describing why).

@@ -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

This comment has been minimized.

Copy link
@Netzeband

Netzeband Dec 5, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@JukkaL

JukkaL Dec 6, 2019

Collaborator

Skipping lib-stubs is probably fine.


except ValueError:
path = tree.path

This comment has been minimized.

Copy link
@Netzeband

Netzeband Dec 5, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@JukkaL

JukkaL Dec 6, 2019

Collaborator

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

This comment has been minimized.

Copy link
@Netzeband

Netzeband Dec 7, 2019

Author Contributor

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.

@@ -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

This comment has been minimized.

Copy link
@JukkaL

JukkaL Dec 6, 2019

Collaborator

Skipping lib-stubs is probably fine.


except ValueError:
path = tree.path

This comment has been minimized.

Copy link
@JukkaL

JukkaL Dec 6, 2019

Collaborator

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

@@ -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'

This comment has been minimized.

Copy link
@JukkaL

JukkaL Dec 6, 2019

Collaborator

This change should not be here.


except ValueError:
return os.path.abspath(path)

else:

This comment has been minimized.

Copy link
@JukkaL

JukkaL Dec 6, 2019

Collaborator

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.

@Netzeband

This comment has been minimized.

Copy link
Contributor Author

Netzeband commented Dec 6, 2019

@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?

  1. How can I run just a specific test case or a specific *.test file? I did not see an option for that, neither for the test-runner python script nor for pytest itself. Maybe it is too hidden in the code?
  2. How can I mark a testcase in a test file to be skipped conditionally (for windows for example?) Is there another test, which does this? Can you point me to this test?
  3. How to avoid the conflict with the mypy/version.py file? The change here was done automatically (I guess a git-hook did it). Can I simply adapt it manually? What version number should be inside?

Thanks for your feedback and also thanks for helping me here.

@JukkaL

This comment has been minimized.

Copy link
Collaborator

JukkaL commented Dec 6, 2019

  1. You can run a specific test case by using pytest -k substring (typically pytest -n0 -k substring is faster if there are only a few tests).

  2. I don't think that there's a way to skip a test inside a .test file on Windows only. We have the -skip test name suffix, which you can use for now to skip always, and I can enable it afterwards on non-Windows platforms.

  3. You should not touch mypy/version.py in this PR. I don't know why the change got included. We don't use any git hooks that would do it. If nothing else helps, you can create the branch from scratch and leave out this change.

@@ -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

This comment has been minimized.

Copy link
@Netzeband

Netzeband Dec 7, 2019

Author Contributor

By accident I saw that the original command did not work.

Andre Netzeband
@Netzeband

This comment has been minimized.

Copy link
Contributor Author

Netzeband commented Dec 7, 2019

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,
André

@msullivan msullivan merged commit 29252f8 into python:master Dec 12, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.