Refactor and fix assert_expected_matched_actual #65

zero323 · 2021-10-07T16:12:38Z

This PR:

Refactors assert_expected_matched_actual function to avoid repeated
matching between expected and actual output
Fixes Test that snippet passes #63
Fixes Incorrect output formatting for Expected #64

The following test file:

- case: all_mismatched
  main: |
    reveal_type(42)  # N: Revealed type is "Literal['foo']?"
    reveal_type("foo")  # N: Revealed type is "Literal[42]?"

- case: missing_message_then_match
  main: |
    reveal_type(42)
    reveal_type("foo")  # N: Revealed type is "Literal['foo']?"

- case: match_then_missing_message
  main: |
    reveal_type(42)  # N: Revealed type is "Literal[42]?"
    reveal_type("foo")

- case: missing_message
  main: |
    42 + "foo"

- case: mismatched_message_inline
  main: |
    1 + 1  # E: Unsupported operand types for + ("int" and "int")

- case: mismatched_messaged_in_out
  main: |
    1 + "foo"
  out: |
    main:1: error: Unsupported operand types for + ("int" and "int")

- case: match_then_mismatched_message
  main: |
    reveal_type(42)  # N: Revealed type is "Literal[42]?"
    reveal_type("foo")  # N: Revealed type is "builtins.int"

- case: mismatched_message_then_match
  main: |
    reveal_type("foo")  # N: Revealed type is "builtins.int"
    reveal_type(42)  # N: Revealed type is "Literal[42]?"

- case: match_between_mismatched_messages
  main: |
    reveal_type(42.0)  # N: Revealed type is "builtins.float"
    reveal_type("foo")  # N: Revealed type is "builtins.int"
    reveal_type(42)  # N: Revealed type is "Literal[42]?"

has been used to check for expected failures and gives output as shown below

test-expect-fail.yaml FFFFFFFFF                                                     [100%]

======================================== FAILURES =========================================
_____________________________________ all_mismatched ______________________________________
/home/zero323/Workspace/test-reg/failing/test-expect-fail.yaml:3: 
E   pytest_mypy_plugins.utils.TypecheckAssertionError: Invalid output: 
E   Actual:
E     main:1: note: Revealed type is "Literal[42]?" (diff)
E     main:2: note: Revealed type is "Literal['foo']?" (diff)
E   Expected:
E     main:1: note: Revealed type is "Literal['foo']?" (diff)
E     main:2: note: Revealed type is "Literal[42]?" (diff)
E   Alignment of first line difference:
E     E: ...ed type is "Literal['foo']?"
E     A: ...ed type is "Literal[42]?"
E                               ^
_______________________________ missing_message_then_match ________________________________
/home/zero323/Workspace/test-reg/failing/test-expect-fail.yaml:9: 
E   pytest_mypy_plugins.utils.TypecheckAssertionError: Invalid output: 
E   Actual:
E     main:1: note: Revealed type is "Literal[42]?" (diff)
E     main:2: note: Revealed type is "Literal['foo']?" (diff)
E   Expected:
E     main:2: note: Revealed type is "Literal['foo']?" (diff)
E   Alignment of first line difference:
E     E: main:2: note: Revealed type is "Literal['foo']?"
E     A: main:1: note: Revealed type is "Literal[42]?"
E             ^
_______________________________ match_then_missing_message ________________________________
/home/zero323/Workspace/test-reg/failing/test-expect-fail.yaml:12: 
E   pytest_mypy_plugins.utils.TypecheckAssertionError: Invalid output: 
E   Actual:
E     main:2: note: Revealed type is "Literal['foo']?" (diff)
E   Expected:
E     (empty)
_____________________________________ missing_message _____________________________________
/home/zero323/Workspace/test-reg/failing/test-expect-fail.yaml:17: 
E   pytest_mypy_plugins.utils.TypecheckAssertionError: Output is not expected: 
E   Actual:
E     main:1: error: Unsupported operand types for + ("int" and "str") (diff)
E   Expected:
E     (empty)
________________________________ mismatched_message_inline ________________________________
/home/zero323/Workspace/test-reg/failing/test-expect-fail.yaml:22: 
E   pytest_mypy_plugins.utils.TypecheckAssertionError: Invalid output: 
E   Actual:
E     (empty)
E   Expected:
E     main:1: error: Unsupported operand types for + ("int" and "int") (diff)
_______________________________ mismatched_messaged_in_out ________________________________
/home/zero323/Workspace/test-reg/failing/test-expect-fail.yaml:26: 
E   pytest_mypy_plugins.utils.TypecheckAssertionError: Invalid output: 
E   Actual:
E     main:1: error: Unsupported operand types for + ("int" and "str") (diff)
E   Expected:
E     main:1: error: Unsupported operand types for + ("int" and "int") (diff)
E   Alignment of first line difference:
E     E: ...rand types for + ("int" and "int")
E     A: ...rand types for + ("int" and "str")
E                                        ^
______________________________ match_then_mismatched_message ______________________________
/home/zero323/Workspace/test-reg/failing/test-expect-fail.yaml:33: 
E   pytest_mypy_plugins.utils.TypecheckAssertionError: Invalid output: 
E   Actual:
E     ...
E     main:2: note: Revealed type is "Literal['foo']?" (diff)
E   Expected:
E     ...
E     main:2: note: Revealed type is "builtins.int" (diff)
E   Alignment of first line difference:
E     E: ...te: Revealed type is "builtins.int"
E     A: ...te: Revealed type is "Literal['foo']?"
E                                 ^
______________________________ mismatched_message_then_match ______________________________
/home/zero323/Workspace/test-reg/failing/test-expect-fail.yaml:37: 
E   pytest_mypy_plugins.utils.TypecheckAssertionError: Invalid output: 
E   Actual:
E     main:1: note: Revealed type is "Literal['foo']?" (diff)
E     ...
E   Expected:
E     main:1: note: Revealed type is "builtins.int" (diff)
E     ...
E   Alignment of first line difference:
E     E: ...te: Revealed type is "builtins.int"
E     A: ...te: Revealed type is "Literal['foo']?"
E                                 ^
____________________________ match_between_mismatched_messages ____________________________
/home/zero323/Workspace/test-reg/failing/test-expect-fail.yaml:43: 
E   pytest_mypy_plugins.utils.TypecheckAssertionError: Invalid output: 
E   Actual:
E     ...
E     main:2: note: Revealed type is "Literal['foo']?" (diff)
E     ...
E   Expected:
E     ...
E     main:2: note: Revealed type is "builtins.int" (diff)
E     ...
E   Alignment of first line difference:
E     E: ...te: Revealed type is "builtins.int"
E     A: ...te: Revealed type is "Literal['foo']?"
E                                 ^
================================= short test summary info =================================
FAILED test-expect-fail.yaml::all_mismatched - 
FAILED test-expect-fail.yaml::missing_message_then_match - 
FAILED test-expect-fail.yaml::match_then_missing_message - 
FAILED test-expect-fail.yaml::missing_message - 
FAILED test-expect-fail.yaml::mismatched_message_inline - 
FAILED test-expect-fail.yaml::mismatched_messaged_in_out - 
FAILED test-expect-fail.yaml::match_then_mismatched_message - 
FAILED test-expect-fail.yaml::mismatched_message_then_match - 
FAILED test-expect-fail.yaml::match_between_mismatched_messages - 
==================================== 9 failed in 2.77s ====================================

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

sobolevn

Thanks a lot for the quick fix! Can you please add a testcase that was failing for you to our suite?

zero323 · 2021-10-07T16:23:46Z

Can you please add a testcase that was failing for you to our suite?

Sure, but I'll need some guidance here. All the cases covered here (refactoring aside) are suppose to fail, so we cannot simply add these to pytest_mypy_plugins/tests/. Normally , it would be something that would be marked with xfail I guess. But how would you like to proceed here?

sobolevn · 2021-10-07T16:50:06Z

I guess we need something like xfail. Probably we can start with xfail: true in the test's metadata.

zero323 · 2021-10-07T18:32:21Z

@sobolevn I am going to convert it to draft.

While bringing back leading / trailing indicator I detected some discrepancies that might require further work. Specifically I was looking at failing_case_3 (cases where there is partial match and then we expect nothing) and started to wonder if zipping lines is a good approach in the first place.

It seems like any preceding mismatch will actually brake all the following matches. Let's say I have following case:

- case: break_following
  main: |
    reveal_type(1 + 1) 
    reveal_type(1 + 1) # N: Revealed type is "builtins.int"
    reveal_type(1 + 1) # N: Revealed type is "builtins.int"
    reveal_type(1 + 1) # N: Revealed type is "builtins.int"
    reveal_type(1 + 1) # N: Revealed type is "builtins.int"

While testing, pytest-mypy-plugins 1.9.1 will give us following result:

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

which is rather counter-intuitive, given that all expectations, but the first one, are satisfied. WDYT?

zero323 · 2021-10-07T18:47:59Z

After this patch it would be

E   pytest_mypy_plugins.utils.TypecheckAssertionError: Output is not expected: 
E   Actual:
E     main:1: note: Revealed type is "builtins.int" (diff)
E     main:2: note: Revealed type is "builtins.int" (diff)
E     main:3: note: Revealed type is "builtins.int" (diff)
E     main:4: note: Revealed type is "builtins.int" (diff)
E     main:5: note: Revealed type is "builtins.int" (diff)
E   Expected:
E     main:2: note: Revealed type is "builtins.int" (diff)
E     main:3: note: Revealed type is "builtins.int" (diff)
E     main:4: note: Revealed type is "builtins.int" (diff)
E     main:5: note: Revealed type is "builtins.int" (diff)
E   Alignment of first line difference:
E     E: main:2: note: Revealed type is "builtins.int"
E     A: main:1: note: Revealed type is "builtins.int"
E

but it feels like we would actually want something around these lines:

E   pytest_mypy_plugins.utils.TypecheckAssertionError: Output is not expected: 
E   Actual:
E     main:1: note: Revealed type is "builtins.int" (diff)
E        ....
E   Expected:
E     (empty)
E       ....
E   Alignment of first line difference:
E     E: main:2: note: Revealed type is "builtins.int"
E     A: main:1: note: Revealed type is "builtins.int"
E

This also escalates, when we have matching blocks interleaved with failures (lets say we copy main block from the example break_following twice).

sobolevn · 2021-10-07T19:48:54Z

This looks correct to me: #65 (comment)

E   pytest_mypy_plugins.utils.TypecheckAssertionError: Output is not expected: 
E   Actual:
E     main:1: note: Revealed type is "builtins.int" (diff)
E     main:2: note: Revealed type is "builtins.int" (diff)
E     main:3: note: Revealed type is "builtins.int" (diff)
E     main:4: note: Revealed type is "builtins.int" (diff)
E     main:5: note: Revealed type is "builtins.int" (diff)
E   Expected:
E     main:2: note: Revealed type is "builtins.int" (diff)
E     main:3: note: Revealed type is "builtins.int" (diff)
E     main:4: note: Revealed type is "builtins.int" (diff)
E     main:5: note: Revealed type is "builtins.int" (diff)
E   Alignment of first line difference:
E     E: main:2: note: Revealed type is "builtins.int"
E     A: main:1: note: Revealed type is "builtins.int"
E

This also escalates, when we have matching blocks interleaved with failures (lets say we copy main block from the example break_following twice).

I need more examples 🙂
I don't understand this one.

zero323 · 2021-10-07T20:48:37Z

need more examples 🙂
I don't understand this one.

Sorry, let me try to explain it better.

Let's assume that I added simple patch to current master

diff --git a/pytest_mypy_plugins/utils.py b/pytest_mypy_plugins/utils.py
index dd8db72..b5f85a0 100644
--- a/pytest_mypy_plugins/utils.py
+++ b/pytest_mypy_plugins/utils.py
@@ -251,7 +251,7 @@ def assert_expected_matched_actual(expected: List[OutputMatcher], actual: List[s
             if i >= len(actual) or not expected[i].matches(actual[i]):
                 if first_diff < 0:
                     first_diff = i
-                error_message += "  {:<45} (diff)".format(expected[i])
+                error_message += "  {:<45} (diff)".format(str(expected[i]))
             else:
                 e = expected[i]
                 error_message += "  " + str(e)[:width]

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

sobolevn · 2021-10-07T21:06:59Z

@zero323 thanks a lot for this great explanation! 👍
I think that this is fine for now. You can open a new issue for it, if you want to.

Let's fix the initial bug first.

zero323 · 2021-10-07T22:21:54Z

Let's fix the initial bug first.

Fair enough :)

I've updated tests and PR description with up-to-date output.

sobolevn · 2021-10-08T08:15:03Z

@zero323 just a quick thought: maybe we can write a couple of regular python unit-tests to make sure this works?
Won't this be easier in this case? What do you think?

zero323 · 2021-10-08T09:53:57Z

@zero323 just a quick thought: maybe we can write a couple of regular python unit-tests to make sure this works? Won't this be easier in this case? What do you think?

Agreed. I was just thinking how to add tests for #66 and realized that run on YAML samples is not going to cut it. And here, we're interested as much about failure itself, as the actual output.

Two questions:

What testing style should we use? Just plain functions and depend on pytest test discovery, or unittest.TestCases?
Do you think it makes sense to decouple pure logic and test actual output, or are we fine with something like assertRaises?

sobolevn · 2021-10-08T09:56:16Z

Just plain functions and depend on pytest test discovery

Plain functions are the best! ⭐

Do you think it makes sense to decouple pure logic and test actual output

I think that we should test the output, since it is the root issue.

zero323 · 2021-10-08T15:45:42Z

pytest_mypy_plugins/tests/test_utils.py

+            """Invalid output: """,
+            """Actual:""",
+            """  main:2: note: Revealed type is "Literal['foo']?" (diff)""",
+            """Expected:""",
+            """  (empty)""",


I am not sure if that's the best way of showing the result in such case. Maybe something around these lines

Invalid output: Actual: ... main:2: note: Revealed type is "Literal['foo']?" (diff) Expected: ... (empty)

would be better?

Maybe something like this? https://github.com/wemake-services/wemake-python-styleguide/tree/master/tests/test_formatter/snapshots

I was actually thinking about the output for this specific test, where match is followed by failure, that we didn't anticipate.

If I understand you correctly, you think about redesigning the test itself. Is that right? I glanced over linked code and snapshottest examples, but I am not sure if I see the advantage here. Let me sleep on that :)

If I understand you correctly, you think about redesigning the test itself. Is that right? I glanced over linked code and snapshottest examples, but I am not sure if I see the advantage here. Let me sleep on that :)

So I took another look at this, but still don't see it (to be honest, parsing non-trivial snapshot keys hurts my brain, so I am probably biased) ‒ the arguments used here are probably to complex to be used directly, and to simple to move to files and justify the indirection (for example, not being able to just eyeball the test to understand the expectations).

sobolevn

LGTM!

zero323 · 2021-10-08T21:22:16Z

Thanks @sobolevn!

Refactor and fix assert_expected_matched_actual

ea253b8

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

sobolevn requested changes Oct 7, 2021

View reviewed changes

Reorder imports

a1b4884

zero323 added 4 commits October 7, 2021 18:59

Add test file

6cb31bc

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

16e92f6

Fix and group leading / trailing ... logic

11d6978

Update test-expect-fail.yaml file

3ac32c3

zero323 marked this pull request as draft October 7, 2021 18:04

Update tests

66fcd2e

zero323 marked this pull request as ready for review October 7, 2021 22:22

zero323 added 2 commits October 8, 2021 14:48

Express error_message prefix in terms of actual and expected

b862da8

Drop data test and add unit tests

69853b3

zero323 commented Oct 8, 2021

View reviewed changes

zero323 added 2 commits October 8, 2021 20:21

Add return type annotation

27157cb

Make exact match on the error_message

e72e329

sobolevn approved these changes Oct 8, 2021

View reviewed changes

Reformat utils.py

0c1d176

sobolevn approved these changes Oct 8, 2021

View reviewed changes

sobolevn merged commit 639263d into typeddjango:master Oct 8, 2021

zero323 deleted the assert-expected-matched-actual-fix-and-refactoring branch October 8, 2021 21:24

zero323 mentioned this pull request Oct 14, 2021

Improve line matching behavior #77

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and fix assert_expected_matched_actual #65

Refactor and fix assert_expected_matched_actual #65

zero323 commented Oct 7, 2021 •

edited

sobolevn left a comment

zero323 commented Oct 7, 2021

sobolevn commented Oct 7, 2021

zero323 commented Oct 7, 2021

zero323 commented Oct 7, 2021 •

edited

sobolevn commented Oct 7, 2021 •

edited

zero323 commented Oct 7, 2021

sobolevn commented Oct 7, 2021 •

edited

zero323 commented Oct 7, 2021

sobolevn commented Oct 8, 2021

zero323 commented Oct 8, 2021

sobolevn commented Oct 8, 2021

zero323 Oct 8, 2021 •

edited

sobolevn Oct 8, 2021

zero323 Oct 8, 2021

zero323 Oct 11, 2021

sobolevn left a comment

zero323 commented Oct 8, 2021

Refactor and fix assert_expected_matched_actual #65

Refactor and fix assert_expected_matched_actual #65

Conversation

zero323 commented Oct 7, 2021 • edited

sobolevn left a comment

Choose a reason for hiding this comment

zero323 commented Oct 7, 2021

sobolevn commented Oct 7, 2021

zero323 commented Oct 7, 2021

zero323 commented Oct 7, 2021 • edited

sobolevn commented Oct 7, 2021 • edited

zero323 commented Oct 7, 2021

sobolevn commented Oct 7, 2021 • edited

zero323 commented Oct 7, 2021

sobolevn commented Oct 8, 2021

zero323 commented Oct 8, 2021

sobolevn commented Oct 8, 2021

zero323 Oct 8, 2021 • edited

Choose a reason for hiding this comment

sobolevn Oct 8, 2021

Choose a reason for hiding this comment

zero323 Oct 8, 2021

Choose a reason for hiding this comment

zero323 Oct 11, 2021

Choose a reason for hiding this comment

sobolevn left a comment

Choose a reason for hiding this comment

zero323 commented Oct 8, 2021

zero323 commented Oct 7, 2021 •

edited

zero323 commented Oct 7, 2021 •

edited

sobolevn commented Oct 7, 2021 •

edited

sobolevn commented Oct 7, 2021 •

edited

zero323 Oct 8, 2021 •

edited