typeddjango / pytest-mypy-plugins

pytest plugin for testing mypy types, stubs, and plugins
https://pypi.org/project/pytest-mypy-plugins/
MIT License
102 stars 24 forks source link

Test that snippet passes #63

Closed 0x143 closed 3 years ago

0x143 commented 3 years ago

In mypy internal testing suite I can omit output to indicate that I expect snippet to pass

[case import]
from collections import namedtuple
[out]

This is very useful for basics checks.

However, I cannot get similar behaviour here. If I create a package

├── mypackage
│   └── __init__.py
├── mypy.ini
└── test
    └── test_mypackage.yaml

with

# ./mypackage/__init__.py
def bar(x: int) -> None:
    pass

and

./test/test_mypackage.yaml
- case: test_mypackage
  main: |
    from mypackage import foo

tests run without errors (pytest-mypy-plugins==1.9.1, mypy==0.910) despite incorrect import foo.

I have to put some output check

./test/test_mypackage.yaml
- case: test_mypackage
  main: |
    from mypackage import foo      
    reveal_type(True)  # N: Revealed type is "Literal[True]?"

to get an exception.

I also tried using empty out block

- case: test_mypackage
  main: |
    from mypackage import foo      
    reveal_type(True)
  out: ""

but it still silently ignores the broken import.

sobolevn commented 3 years ago

@0x143 can you try the same with empty out: mark?

Docs: https://github.com/typeddjango/pytest-mypy-plugins#3-longer-type-expectations

0x143 commented 3 years ago

Thank you for your answer @sobolevn.

I tried putting an empty out block, but to no avail. The initial import error is caught only if I have specific output expectation.

I've seen that django-stubs have some cases with no output at all and I assume these work, and I am just doing something stupid here.

sobolevn commented 3 years ago

It might be an issue with django-stubs, can you please investigate?

0x143 commented 3 years ago

It might be an issue with django-stubs, can you please investigate?

I'll be happy to, but I am not exactly sure what I am looking for. I've done some basic tests to see if django-stubs test fail on broken import, even if no explicit output check is present, and they do. Anything else I can do?

So I am really not sure what I am looking for here (any config options maybe?)

Sorry for naive questions and thank you for your support and patience @sobolevn!

sobolevn commented 3 years ago

@0x143 ok, then we need to find which follow-import strategy you use. Do you have the same mypy config for tests and dev? Does it raise error you are looking for in dev mode?

0x143 commented 3 years ago

I don't use any custom mypy configuration @sobolevn, but I don't think that import following is an issue here.

To better illustrate the problem, here is a minimal repo that shows the issue. As you see, it isnot limited to imports ‒ incorrect calls seem to be ignored as well. Also, mypy called directly with the same (empty config) on equivalent source files, detects the problems.

sobolevn commented 3 years ago

Ok, this a pretty serious bug. I don't know how long this was hidding!

Can you please fix it? PRs are more than welcome!

0x143 commented 3 years ago

Can you please fix it? PRs are more than welcom

I'll try to investigate this further and see if I can figure out the fix.

Thanks again for all your responses @sobolevn!

zero323 commented 3 years ago

It should be enough to modify assert_expected_matched_actual to check if expected size differs from actual size, shouldn't it?

index dd8db72..3448675 100644
--- a/pytest_mypy_plugins/utils.py
+++ b/pytest_mypy_plugins/utils.py
@@ -230,7 +230,7 @@ def assert_expected_matched_actual(expected: List[OutputMatcher], actual: List[s
     actual = remove_common_prefix(actual)
     error_message = ""

-    if not all(e.matches(a) for e, a in zip(expected, actual)):
+    if len(actual) != len(expected) or not all(e.matches(a) for e, a in zip(expected, actual)):
         num_skip_start = _num_skipped_prefix_lines(expected, actual)
         num_skip_end = _num_skipped_suffix_lines(expected, actual)

It should also save some work, because we can short circuit and skip actual matching.

zero323 commented 3 years ago

One could probably refactor the whole thing to just iterate over itertools.zip_longest, i.e.

from itertools import islice, zip_longest

stop =  max(len(actual), len(expected)) - num_skip_end)

for i, (e, a) in enumerate(islice(zip_longest(expected, actual)), num_skip_start, stop)):
   if a is None or e is None or not e.matches(a):
       ...  # Report failure

This way, matches would be invoked only once for each expected line.