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

Improve line matching behavior #77

Open
zero323 opened this issue Oct 14, 2021 · 3 comments
Open

Improve line matching behavior #77

zero323 opened this issue Oct 14, 2021 · 3 comments

Comments

@zero323
Copy link
Contributor

zero323 commented Oct 14, 2021

This is a feature request.

Summary:

Currently rows are matched by their position in the file / output.

User experience could be improved, if matching was performed by (file, line-number).

Details:

Let's assume that I have a test case like this

- case: break_following_2
  main: |
    reveal_type(1 + 1) 
    reveal_type(1.0 + 2.0) # N: Revealed type is "builtins.float"
    reveal_type("foo" + "bar") # N: Revealed type is "builtins.str"

When I run tests I see:

E   pytest_mypy_plugins.utils.TypecheckAssertionError: Invalid output: 
E   Expected:
E     main:2: note: Revealed type is "builtins.float" (diff)
E     main:3: note: Revealed type is "builtins.str" (diff)
E   Actual:
E     main:1: note: Revealed type is "builtins.int" (diff)
E     main:2: note: Revealed type is "builtins.float" (diff)
E     main:3: note: Revealed type is "builtins.str" (diff)
E   
E   Alignment of first line difference:
E     E: main:2: note: Revealed type is "builtins.float"
E     A: main:1: note: Revealed type is "builtins.int"
E

If you analyze the test case, you'll see that actual state is like this:

line actual expected match
1 Revealed type is "builtins.int"
2 Revealed type is "builtins.float" Revealed type is "builtins.float"
3 Revealed type is "builtins.str" Revealed type is "builtins.str"

however alignment message

E   Alignment of first line difference:
E     E: main:2: note: Revealed type is "builtins.float"
E     A: main:1: note: Revealed type is "builtins.int"

clearly shows that we start with comparing line 2 of expected and line 1 of actual.

This escalates to all the following lines and probably gets worse with multi-line messages (I wanted to investigate that, hence #66).

I am aware that this is consistent with behavior of the internal mypy test suite, which returns

Expected:
  main:2: note: Revealed type is "builtins.float" (diff)
  main:3: note: Revealed type is "builtins.str" (diff)
Actual:
  main:1: note: Revealed type is "builtins.int" (diff)
  main:2: note: Revealed type is "builtins.float" (diff)
  main:3: note: Revealed type is "builtins.str" (diff)

Alignment of first line difference:
  E: main:2: note: Revealed type is "builtins.float"
  A: main:1: note: Revealed type is "builtins.int"
          ^

for equivalent input, but it seems a bit counter-intuitive. While it detects presence of output mismatch, it cannot do much beyond that.

Ideally, I'd like to see something around these lines:

E   pytest_mypy_plugins.utils.TypecheckAssertionError: Invalid output: 
E   Expected:
E     (Empty)
E      ...
E   Actual:
E     main:1: note: Revealed type is "builtins.int" (diff)
E     ...

(no alignment needed for an empty line).

This should generalize to multiple interleaved blocks of matching and not matching lines, where matching blocks are indicated, but omitted.

Furthermore, errors shouldn't propagate beyond current line, in case of multline output.

Originally posted by @zero323 in #65 (comment)

@zero323
Copy link
Contributor Author

zero323 commented Oct 14, 2021

I am happy to give it a shot, if there is interest in such feature.

@sobolevn
Copy link
Member

Awesome! Let's try it. However, the end result might be even more confusing, so let's just keep an open mind 🙂

@zero323
Copy link
Contributor Author

zero323 commented Oct 14, 2021

the end result might be even more confusing, so let's just keep an open mind 🙂

Sure, I am not getting particularly attached to code :)

And even if it brings desired results, it might be still something that should be configurable, in case someone already depends on the current behavior.

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

No branches or pull requests

2 participants