Skip to content

Prototype of common test suite format and runner for Mypy #436

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions test-data/check-basic.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# Basic tests.

- case: Empty file
files:
- content: |

- case: Assignment and var def
files:
- content: |
a = None # type: A
b = None # type: B
a = a
a = b # E: Incompatible types in assignment (expression has type "B", variable has type "A")
class A: pass
class B: pass

- case: Constructor and assignment
files:
- content: |
class A:
def __init__(self): pass
class B:
def __init__(self): pass
x = None # type: A
x = A()
x = B() # E: Incompatible types in assignment (expression has type "B", variable has type "A")

- case: Invalid argument count
files:
- content: |
import typing
def f(x, y) -> None: pass
f(object()) # E: Too few arguments for "f"
f(object(), object(), object()) # E: Too many arguments for "f"
17 changes: 17 additions & 0 deletions test-data/check-modules.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Type checker test cases dealing with modules and imports.

- case: Access imported definitions
files:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to omit files: line? It looks redundant, but I have never worked with YAML.

Copy link
Member Author

@vlasovskikh vlasovskikh May 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ilevkivskyi No, it's not possible to omit it here. In a sense, YAML is just a simpler way to write JSON objects, which are, from the Python's point of view, just dictionaries.

So, this YAML file:

# Type checker test cases dealing with modules and imports.

- case: Access imported definitions
  files:
    - content: |
        ...
    - path: m.py
      content: |
        ...

is equivalent to the following Python dict:

[{'case': 'Access imported definitions',
  'files': [{'content': '...\n'}, {'content': '...', 'path': 'm.py'}]}]

(You can convert a YAML string to a Python dict / list using yaml.load(yaml_string).)

So we need the files attribute, as it becomes the 'files' key in our test case dict.

- content: |
import m
import typing
m.f() # E: Too few arguments for "f"
m.f(object()) # E: Argument 1 to "f" has incompatible type "object"; expected "A"
m.x = object() # E: Incompatible types in assignment (expression has type "object", variable has type "A")
m.f(m.A())
m.x = m.A()
- path: m.py
content: |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to somehow make this less verbose? Mypy tests just use single line [file m.py].

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ilevkivskyi I'm afraid not. path and content become the keys of the resulting dict: {'path': 'm.py', 'content': '...'}. I've made the file path main.py default so you don't have to type path for every test. But when you need a custom path, you have to provide it as an attribute.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I see. Still, YAML looks quite flexible, let's see what others will say.

class A: pass
def f(a: A) -> None: pass
x = A()
89 changes: 89 additions & 0 deletions tests/mypy_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import glob
import os
import re
from subprocess import Popen, PIPE
from typing import Text, List, Iterable, Tuple, Any

import pytest
import yaml


DEFAULT_PATH = 'main.py'
ENCODING = 'utf-8'


def load_test_cases(root: Text) -> Iterable[Tuple[Text, Text, List[dict]]]:
paths = glob.glob(os.path.join(root, '*.yml'))
assert paths, 'No test data found in %s' % os.path.abspath(root)

for path in paths:
with open(path, 'rb') as fd:
raw_data = yaml.load(fd)
for case in raw_data:
_, filename = os.path.split(path)
name = case.get('case')
assert name, 'Unnamed case: %r' % case
files = case.get('files', [])
unique = {file.get('path', DEFAULT_PATH) for file in files}
assert len(files) == len(unique), 'Duplicate files in %s' % name
yield filename, name, files


def inline_errors(errors: List[Text], files: List[dict]) -> List[dict]:
contents = {
file.get('path', DEFAULT_PATH): strip_errors(file.get('content', ''))
for file in files}

for error in errors:
index, message, path = parse_error_line(error)
content = contents.get(path)
assert content, 'Cannot find contents of %s' % path
lines = content.splitlines(keepends=True)
assert 0 <= index < len(lines)
line = lines[index]
updated = '%s# E: %s\n' % (line.rstrip('\n'), message)
lines[index] = updated
contents[path] = ''.join(lines)

results = [file.copy() for file in files]
for result in results:
result['content'] = contents.get(result.get('path', DEFAULT_PATH), '')
return results


def strip_errors(s: Text) -> Text:
return re.sub(r'#\s*E:.*', r'', s)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we split on ' # ' (i.e. requiring at least one space on each side of '#')? See python/mypy#3417 -- the reason is that we want to allow error messages containing URLs of the form http://host/path#fragment to deep-link directly to a specific section of the docs (like https://github.com/python/mypy/pull/3411/files#diff-7739b07c7ae4561e51cbb36f31984c90R1016 -- still WIP).



def parse_error_line(line: Text) -> Tuple[int, Text, Text]:
m = re.match(r'(.+):([0-9]+): (error): (.+)', line)
assert m, 'Cannot parse output line: %s' % line
path = m.group(1)
index = int(m.group(2)) - 1
message = m.group(4)
return index, message, path


@pytest.mark.parametrize('filename,case,files', load_test_cases('test-data'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to be calling load_test_cases() even if this test were not selected. Upon further inspection load_test_cases() is a generator function so the call doesn't actually do any work, but it still creeped me out. (Maybe load_test_cases() deserves a comment explaining why it should be a generator.)

def test_yaml_case(filename: Text, case: Text, files: List[dict],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pytest allows easily writing a plugin to directly parse non-Python files as pytest-native test files containing individual tests, rather than using test parametrization like this. I've done this before, and it works very well and has a number of nice properties, e.g.:

  • output when running tests has a line of dots per actual test file, not just a single line of dots referring uselessly to this file
  • test names in failure output can be exactly the name listed in the yaml file, with no repeated test_yaml_case cruft
  • you can use the obvious pytest path/to/test-basic.yaml natively, to run the tests in test-basic.yaml only, just like you would with a file containing Python tests

Sample code for this approach can be found at https://docs.pytest.org/en/latest/example/nonpython.html

tmpdir: Any) -> None:
assert filename
assert case

for file in files:
path = file.get('path', DEFAULT_PATH)
dirname, filename = os.path.split(path)
filedir = tmpdir.mkdir(dirname) if dirname else tmpdir
with filedir.join(filename).open('w', encoding=ENCODING) as fd:
fd.write(file.get('content', ''))

tmpdir.chdir()

with Popen(['mypy', '.'], stdout=PIPE, stderr=PIPE) as proc:
stdout, stderr = proc.communicate()

output = stdout + b'\n' + stderr
output_lines = output.decode(ENCODING).strip().splitlines()
actual_files = inline_errors(output_lines, files)
for expected, actual in zip(files, actual_files):
assert expected.get('content', '') == actual.get('content', '')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While pytest produces nice diff-style output when an assert fails, there's one problem: if the expected output for several files doesn't match the actual output, pytest only shows the diff for the first file with a mismatch. This seems a problem (since sometimes unexpected errors in the second file provide a clue for the unexpected errors in the first file).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it would look nice if the output for each file were concatenated with headers, e.g.:

-----x.py-----
output
-----y.py----
output

Then you could see it all at once.