Move testparse to pytest

[elazarg]

IIUC, this is an item from #1673. I tried to keep this minimal.

(If this goes well I can add more)

[gvanrossum]

It seems to work, but I don't know this code very well -- do you understand it well enough to confidently say this is right?

[gvanrossum]

Since you're adding this here you should update all the classes that derive from it. Or you can follow the existing pattern and leave it out here but add such a __init__ method to ParserSuite (like all other subclasses of DataSuite do). It seems these classes all need an update_date keyword argument because of the call on line 533 in data.py (line number from master, it's in runtest() in MypyDataCase).

Note that these classes don't seem to be instantiated anywhere, I suppose they are recognized dynamically. Do you know how?

[elazarg]

I've added it here since I believe that at the end of the road no class will need it, or any other argument. So for the time being, a prototype __init__ in the base class (just for that keyword) should suffice.

[elazarg]

These classes are gathered by pytest, passed to as the obj argument to pytest_pycollect_makeitem which returns a MypyDataSuite wrapping this obj. Later the collect method is called on the MypyDataSuite instance, and our implementation iterates over self.obj.cases(), wrapping and yielding the results.

[gvanrossum]

Yeah, that explanation of how they are called makes sense.

I still think that now you're adding __init__ to DataSuite, all subclasses of it that also define __init__ should be modified to call super().__init__(update_data=update_data). Otherwise my spider-sense about inheritance is unhappy.

[elazarg]

The only class that actually defines a nontrivial __init__ is ASTMergeSuite (and I'm not sure it should, since it does not receive arguments and does not seem to have any mutable state).
May I simply remove the empty __init__ definitions?

[gvanrossum]

I think you should remove those __init__ definitions that are now a copy of DataSuite.__init__, and make the others call super().__init__(update_data=update_data). I think there may be one or two that ignore their update_data argument, please figure out if those simply don't need it or if setting it would just be ignored.

[elazarg]

Currently the only test that uses this option is testcheck, which uses update_testcase_output if it's set. The others simply ignore it.

[gvanrossum]

Does it actually matter whether this is a class method or an instance method? (I don't know -- can you tell me?)

[elazarg]

It's called without an instance (line 515 in data.py, obj is the class itself); it could have been a static method. Better yet, it could not exist at all, since the needed information here is only the file names and 2-3 options.

[elazarg]

For now it has to be a classmethod since it's this is how it's declared in DataSuite.

[gvanrossum]

Oh, I think I realize what confused me. There's a class Suite with a cases() instance method, and a class DataSuite with a cases()` class method. The two are not directly related (though I suspect that some duck typing may still be going on). Anyway, no need to change this back. :-)

[elazarg]

I don't know if that would help the review in any way, but here is an almost-complete migration of everything to pytest, plus making individual tests run in 0.5s instead of 4.

It's "almost", and it's one huge step, and it's outdated. So I'm trying* to do it step by step.
(It was also a couple of months ago so I don't remember all the details).

* (I really am :) )

[elazarg]

Maybe there's a way to automate testing-the-tests to avoid regression. As a sanity check I randomly put errors in the tests and watch them fail. This is not systematic enough though.

[elazarg]

Also, running

pytest -n0 --collect-only

gives a list of collected tests:

...
<Module 'mypy/test/testparse.py'>
  <MypyDataSuite 'ParserSuite'>
    <MypyDataCase 'testEmptyFile'>
    ...
  <MypyDataSuite 'ParseErrorSuite'>
    <MypyDataCase 'testInvalidFunction'>
    ...

[elazarg]

Or did you want here a call to super().__init__?

[gvanrossum]

I'll merge once the tests pass. Thanks for your patience!

[elazarg]

Ok. If everything goes well, please let me know how do you want to continue. There are 3 more data-driven test (testtransform, testtypegen, testsemanal), all of them are similarly straightforward. Do you want a dedicated PR for each of them or a single one?

[gvanrossum]

Thanks! I much prefer to review smaller pieces (easier to get them done between meetings :-).

[ilinum]

Thanks for doing this @elazarg! I will be so happy once we can fully switch to pytest.