-
Notifications
You must be signed in to change notification settings - Fork 263
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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" |
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: | ||
- 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: | | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ilevkivskyi I'm afraid not. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() |
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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')) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.:
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', '') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.:
Then you could see it all at once. |
There was a problem hiding this comment.
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.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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:
is equivalent to the following Python dict:
(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.