Refactor test discovery by elazarg · Pull Request #3973 · python/mypy

elazarg · 2017-09-20T01:09:22Z

Test discovery logic is duplicated over all the different test suites. This PR refactors this logic to the plugin method collect(), leaving only the choice of configuration in the classes. This refactoring should enable further optimizations in the discovery of specific tests.

The argument perform to parse_test_cases() is (already) unused, so it is removed. Same goes for include_path.

The core of the changes can be tested by diffing the result of py.test --collect-only -n0 with the main branch. The only differences should be the execution time and the name of the suite PythonCmdlineSuite which is called PythonEvaluationSuite on master.

TestCase is split into BaseTestCase to avoid sharing of fields and methods that's not really shared with myunit tests, such as run(). I believe this change need not last long.

JukkaL

Negative lines of code is great -- thanks! This removes some pretty pointless boilerplate. I have just a bunch of style/documentation related comments.

JukkaL · 2017-11-30T15:31:22Z

mypy/myunit/__init__.py

-    def __init__(self, name: str, suite: 'Optional[Suite]' = None,
-                 func: Optional[Callable[[], None]] = None) -> None:
-        self.func = func
+class ProtoTestCase:


I'd suggest renaming this to BaseTestCase, since Proto sounds like a protocol but this is a regular class. Also add a docstring. At least describe how these are run, since this class doesn't seem to have a hint about that.

JukkaL · 2017-11-30T15:34:01Z

mypy/myunit/__init__.py

        self.tmpdir = None


+class TestCase(ProtoTestCase):


Add docstring. In particular, how is this different from the base class?

JukkaL · 2017-11-30T15:35:46Z

mypy/test/data.py



-class DataDrivenTestCase(TestCase):
+class DataDrivenTestCase(ProtoTestCase):


This is kind of confusing, since there seems to be no longer anything in this class about how an individual test case is run. This at least needs a docstring.

JukkaL · 2017-11-30T15:37:46Z

mypy/test/data.py

 def pytest_pycollect_makeitem(collector: Any, name: str, obj: Any) -> Any:
    if not isinstance(obj, type) or not issubclass(obj, DataSuite):
        return None
+    if obj is DataSuite:


Add comment here -- something like this perhaps: only classes derived from DataSuite contain test cases, not the DataSuite class.

JukkaL · 2017-11-30T15:38:53Z

mypy/test/data.py

    def collect(self) -> Iterator['MypyDataCase']:
-        for case in self.obj.cases():
-            yield MypyDataCase(case.name, self, case)
+        cls = self.obj


Add comment -- what is self.obj here and where does it come from? This apparently requires understanding some pytest magic, which many readers likely won't be too familiar with, including me.

JukkaL · 2017-11-30T15:39:18Z

mypy/test/data.py

    return MypyDataSuite(name, parent=collector)


 class MypyDataSuite(pytest.Class):  # type: ignore  # inheriting from Any


Could you add docstring here as well while you are at it and probably have more context on this than I have?

JukkaL · 2017-11-30T15:42:05Z

mypy/test/data.py



 class DataSuite:
+    files = None  # type: typing.ClassVar[List[str]]


I'd rather not have the ClassVar annotations here and elsewhere -- they add noise and distract from the main intent of the code. It would be okay to just add a comment here mentioning that all of these should be treated as class variables.

JukkaL · 2017-11-30T15:47:09Z

mypy/test/data.py

+    optional_out = False  # type: typing.ClassVar[bool]
+    native_sep = False  # type: typing.ClassVar[bool]
+    require_stable = False  # type: typing.ClassVar[bool]
+    require_incremental = False  # type: typing.ClassVar[bool]


require_stable and require_incremental are too specialized to be included here as configuration attributes. It's okay to have is_incremental and has_stable_flags as utility functions in this module (I don't think that they need to be associated with any class), but I'd prefer if these would be explicitly handled using asserts in run_case methods, similar to how these were implemented previously.

These are also used as filters in the test selection, and the asserts only validate it. I will add a filter method then.

JukkaL · 2017-11-30T15:49:38Z

mypy/test/testpythoneval.py

-                    test_python_evaluation, test_temp_dir, True)
-        return c
+    files = ['pythoneval.test', 'python2eval.test']  # type: typing.ClassVar[List[str]]
+    if sys.version_info.major == 3 and sys.version_info.minor >= 4:


3.3 is no longer supported for running mypy so this test can be dropped.

…covery

elazarg · 2017-12-01T14:23:08Z

I think I've addressed the comments except the parts about configuration options. I hope to address the rest tomorrow.

elazarg · 2017-12-02T19:13:42Z

@JukkaL I believe this is ready.

JukkaL

Thanks for the updates! Looks mostly good now, just a few minor things left.

JukkaL · 2017-12-12T14:31:13Z

mypy/myunit/__init__.py

+class BaseTestCase:
+    """Common base class for _MyUnitTestCase and DataDrivenTestCase.
+
+    Handles temporary folder creation and deletion"""


Style nit: In multi-line docstrings add the trailing """ on a separate line like this:

class BaseTestCase: """Common base class for _MyUnitTestCase and DataDrivenTestCase. Handles temporary folder creation and deletion. """ ...

JukkaL · 2017-12-12T14:32:07Z

mypy/myunit/__init__.py

+
+    def setup(self) -> None:
+        super().setup()
+        self.suite.set_up()


Inconsistent naming: setup vs set_up?

JukkaL · 2017-12-12T14:35:02Z

mypy/test/data.py

+                              obj: object) -> 'Optional[Any]':
+    """Called by pytest on each object in modules configured in conftest.py files.
+
+    collector is pytest.Collector, returns Optional[pytest.Class]"""


Style nit: """ should be on a separate line in multi-line docstrings.

JukkaL · 2017-12-12T14:36:42Z

mypy/test/data.py

+        # Only classes derived from DataSuite contain test cases, not the DataSuite class itself
+        if issubclass(obj, DataSuite) and obj is not DataSuite:
+            # Non-None result means this obj is a test case
+            # result.collect will be called, with self.obj being obj


Here it's a little unclear what result.collect refers to. Maybe rephrase as "The collect method of the returned MypyDataSuite instance will be called later, with self.obj being obj."

JukkaL · 2017-12-12T14:39:09Z

mypy/test/testcheck.py

-            c += parse_test_cases(os.path.join(test_data_prefix, f),
-                                  None, test_temp_dir, True)
-        return c
+    files = typecheck_files  # type: List[str]


All of the variable type annotations for in DataSuite subclasses now seem redundant. Can you remove them?

JukkaL · 2017-12-12T14:40:57Z

mypy/test/testdmypy.py

    def run_case(self, testcase: DataDrivenTestCase) -> None:
-        assert self.is_incremental(testcase), "Testcase is not incremental"
-        assert self.has_stable_flags(testcase), "Testcase has varying flags"
+        assert has_stable_flags(testcase), "Testcase is not incremental"


The message should be "Testcase has varying flags" (the messages seem to have been switched on these two lines).

JukkaL · 2017-12-12T14:41:13Z

mypy/test/testdmypy.py

-        assert self.is_incremental(testcase), "Testcase is not incremental"
-        assert self.has_stable_flags(testcase), "Testcase has varying flags"
+        assert has_stable_flags(testcase), "Testcase is not incremental"
+        assert is_incremental(testcase), "Testcase has varying flags"


Here's the other switched message.

elazarg · 2017-12-12T15:29:03Z

Thanks! Fixed.

elazarg force-pushed the refactor-test-discovery branch 7 times, most recently from d75d7ee to a107d0c Compare September 20, 2017 09:15

elazarg mentioned this pull request Sep 23, 2017

Appveyor tests are being flaky #3895

Closed

elazarg mentioned this pull request Oct 18, 2017

Add daemon mode -- highly experimental #4130

Merged

elazarg force-pushed the refactor-test-discovery branch from e76f9ef to 5df8a6c Compare October 21, 2017 19:13

refactor test discovery

34120f6

elazarg force-pushed the refactor-test-discovery branch from 5df8a6c to 34120f6 Compare October 21, 2017 20:04

elazarg added 3 commits November 12, 2017 18:21

Merge branch 'master' into refactor-test-discovery

4a738af

Merge branch 'master' into refactor-test-discovery

900c087

import test_temp_dir

3d89f93

JukkaL reviewed Nov 30, 2017

View reviewed changes

elazarg added 6 commits December 1, 2017 15:39

remove python3.3 related check

0ce79ab

rename ProtoTestCase to BaseTestCase

cdbfe1b

cleanup and document myunit classes

73b56a6

document pytest magic

f255c4b

Merge remote-tracking branch 'upstream/master' into refactor-test-dis…

61d4e7e

…covery

uniform naming (setup/teardown); document DataDrivenTestCase

a5c6939

elazarg added 3 commits December 2, 2017 20:29

assert is_incremental and has_stable_flags

b5c1067

avoid using ClassVar

9e12fec

add filter method to DataSuite for dmypy

a8c61ba

JukkaL reviewed Dec 12, 2017

View reviewed changes

elazarg added 2 commits December 12, 2017 16:59

consistent setup/teardown; multiline docstrings

8eb0fed

fix switched messages

c26671b

elazarg added 2 commits December 12, 2017 17:07

remove annotations from DataSuite subclasses

5600859

remove missed annotation

a1d0c18

JukkaL approved these changes Dec 12, 2017

View reviewed changes

JukkaL merged commit dc52e86 into python:master Dec 12, 2017



		class DataDrivenTestCase(TestCase):
		class DataDrivenTestCase(ProtoTestCase):

		return MypyDataSuite(name, parent=collector)


		class MypyDataSuite(pytest.Class): # type: ignore # inheriting from Any



		class DataSuite:
		files = None # type: typing.ClassVar[List[str]]

Uh oh!

Conversation

elazarg commented Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JukkaL left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elazarg commented Dec 1, 2017

Uh oh!

elazarg commented Dec 2, 2017

Uh oh!

JukkaL left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elazarg commented Dec 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

elazarg commented Sep 20, 2017 •

edited

Loading