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

Test that snippet passes #63

Closed
0x143 opened this issue Oct 2, 2021 · 10 comments · Fixed by #65
Closed

Test that snippet passes #63

0x143 opened this issue Oct 2, 2021 · 10 comments · Fixed by #65

Comments

@0x143
Copy link

0x143 commented Oct 2, 2021

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
Copy link
Member

sobolevn commented Oct 2, 2021

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

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

@0x143
Copy link
Author

0x143 commented Oct 2, 2021

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
Copy link
Member

sobolevn commented Oct 2, 2021

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

@0x143
Copy link
Author

0x143 commented Oct 2, 2021

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
Copy link
Member

sobolevn commented Oct 2, 2021

@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
Copy link
Author

0x143 commented Oct 2, 2021

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
Copy link
Member

sobolevn commented Oct 2, 2021

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
Copy link
Author

0x143 commented Oct 2, 2021

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
Copy link
Contributor

zero323 commented Oct 5, 2021

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
Copy link
Contributor

zero323 commented Oct 5, 2021

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.

zero323 added a commit to zero323/pytest-mypy-plugins that referenced this issue Oct 7, 2021
This PR:

- Refactors assert_expected_matched_actual function to avoid repeated
  matching between expected and actual output
- Fixes typeddjango#63, typeddjango#64
sobolevn pushed a commit that referenced this issue Oct 8, 2021
* Refactor and fix assert_expected_matched_actual

This PR:

- Refactors assert_expected_matched_actual function to avoid repeated
  matching between expected and actual output
- Fixes #63, #64

* Reorder imports

* Add test file

* Add back  leading or trainling ... in case of matching input

* Fix and group leading / trailing ... logic

* Update test-expect-fail.yaml file

* Update tests

* Express error_message prefix in terms of actual and expected

* Drop data test and add unit tests

* Add return type annotation

* Make exact match on the error_message

* Reformat utils.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants