Skip to content

Fix/Improve Test Driver #721

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

Merged
merged 24 commits into from
Oct 11, 2015
Merged

Fix/Improve Test Driver #721

merged 24 commits into from
Oct 11, 2015

Conversation

o11c
Copy link
Contributor

@o11c o11c commented Jul 8, 2015

The current travis.sh is broken in a number of ways. This fixes most of them.

Note: there are legitimate bugs exposed in mypy here, that's why the tests fail. The stubs do not appear to be incorrect.

@o11c
Copy link
Contributor Author

o11c commented Jul 25, 2015

Okay, I've refactored so that this can be merged before the XML one.

@o11c
Copy link
Contributor Author

o11c commented Jul 26, 2015

@JukkaL I'm really not sure if I can solve the bug this exposes. I just investigated my hypothesis and it turned out to be fruitless.

(An example of) The failing test is:
mypy -c 'import requests.packages.urllib3.connection'

Which is curious because this works and does result in an import of the connection module:
mypy -c 'import requests.packages.urllib3'

But this doesn't work:
mypy -c 'import requests.packages.urllib3, requests.packages.urllib3.connection'

This changes the error message:
mypy -c 'import requests.packages.urllib3.connectionpool, requests.packages.urllib3.connection'

And this somehow works:
mypy -c 'import requests.adapters, requests.packages.urllib3.connectionpool, requests.packages.urllib3.connection'

So all I can really say is that this is some subtle ordering problem that is beyond my skill/knowledge.

@o11c
Copy link
Contributor Author

o11c commented Jul 31, 2015

I added a list of XFAILs so at least we can get meaningful test results until the real bug is fixed.

Also managed to find a non-racy way to run tasks in parallel without os.waitid.

@o11c o11c force-pushed the driver branch 2 times, most recently from e9401c1 to f04f262 Compare August 1, 2015 03:26
@JukkaL
Copy link
Collaborator

JukkaL commented Aug 4, 2015

Thanks for the PR! This is pretty big so it might take a while but I hope to have time to review it this weekend.

@o11c
Copy link
Contributor Author

o11c commented Aug 17, 2015

Ugh, your recent pushes made there be conflicts.

@JukkaL
Copy link
Collaborator

JukkaL commented Aug 24, 2015

I've now merged your changes to the current master (haven't pushed them yet) and have been playing around with them a little. It's much better to run tests in parallel, type check all stubs and to have automatic test discovery!

There are some regressions that shouldn't be difficult to fix or work around:

  • Running individual test cases requires more work than before (python3 -m mypy.munit -m mypy.test.testfoo "*foobar*" vs python3 tests.py "*check*foobar*"). It's probably worth figuring a simpler way of running a subset of unit tests.
  • The output is way noisier than before. I don't want many pagefuls of output if things go right. By default, it should not output a lot of stuff (but it's okay to write more verbose output to a log file so that it's possible to debug if things go wrong).
  • If travis.py is the only entry point to running multiple test suites, we should probably rename it to something else as people will also want to run it locally (tests.py?).
  • Previously there was an easy way to run only inexpensive (unit) tests (tests.py). It would be nice to be still able to easily skip expensive integration tests but run everything else (at least pythoneval, which is really slow). Or maybe we could run python eval tests in parallel by splitting it into multiple chunks.

@o11c
Copy link
Contributor Author

o11c commented Aug 24, 2015

Running individual test cases requires more work than before (python3 -m mypy.munit -m mypy.test.testfoo "foobar" vs python3 tests.py "_check_foobar*"). It's probably worth figuring a simpler way of running a subset of unit tests.

mypy.myunit is not intended to be run by hand at the moment (this may change if we switch from distutils to setuptools and get easy entry_point management.

What I usually do is run ./travis.py patterns.... For example, I have my vim configured to run ./travis.py mypy.syntax to only run syntax-related tests. This has the advantage of also running the type-checker for those modules, not just the unit test modules.

If it's really worth running just a single unit test (since other than pythoneval, test modules are fast), I briefly experimented with ./travis.py modulepatterns... -- arguments but stopped since it wasn't clear what should happen if there was more than one module. Perhaps ./travis.py modulepattern:argument?

Really, mypy.myunit needs more love; it is currently a half-assed approach to testing. Of particular relevance to this issue, it ought to have discovery of its own. I have been thinking about switching to layout compatible with py.test however.

The output is way noisier than before.

Am I the only one who always runs test drivers in verbose mode? It's not like there is anything valuable in the terminal's scrollback, and the failures are nicely collected at the end. But I'm certainly not opposed to adding command-line options to travis.py to control it.

I prefer immediate output because deferred output and log files can be problematic, though I admit that I am used to working in C and C++ where memory corruption and sudden exit are more common (though they're certainly possible in python also).

If travis.py is the only entry point to running multiple test suites, we should probably rename it to something else as people will also want to run it locally (tests.py?).

I deliberately did not name it tests.py because it is not at all similar to the tests.py that existed before this patch. And I think people should draw comfort in knowing that the way they are running tests matches the way tests will be run automatically.

Bikeshed alternative: runtests.py ?

Previously there was an easy way to run only inexpensive (unit) tests

Currently, ./travis.py unit-test is my preferred way to run all unit tests, which includes pythoneval tests. Perhaps the collector should be given additional knowledge about the pythoneval tests specifically (some form of this may be needed anyway for #732), or possibly you could add negative patterns as well as positive patterns.


This PR is not an attempt to provide perfection, only an improvement. Ideally I think mypy.myunit would integrate all the abilities of travis.py and mypy.waiter but I don't have a clear design yet and I intend to work on other things first.

@o11c
Copy link
Contributor Author

o11c commented Sep 21, 2015

@JukkaL Can you please (rebase and) merge this? I have no clue what is acceptable to you, and you keep on giving painful delays.

I'm almost done with the new parser (still need to finish expression parsing and port billions of unit tests), but for lowering I really need to be able to use the updated version of mypy.

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 1, 2015

@o11c: Sorry for having being unresponsive; I've been out of the country for almost 3 weeks now and been very busy. Things will calm down next week. I may have a few hours later this week, but I can't promise anything.

@o11c
Copy link
Contributor Author

o11c commented Oct 1, 2015

Note, there is a rebased version of this at https://github.com/o11c/mypy/tree/driver-rebased but I haven't investigated the new test failures yet.

@o11c
Copy link
Contributor Author

o11c commented Oct 1, 2015

Okay, fixed.

@o11c
Copy link
Contributor Author

o11c commented Oct 1, 2015

Okay, fixed. More cyclic import problems that probably should be fixed elsewhere, but ... explicit annotations will do for now.

@o11c
Copy link
Contributor Author

o11c commented Oct 1, 2015

Rebased to fix another bug caused by upstream changes, and made the linter stop complaining.

I really think the linter is being stupid for some of those things though.

@o11c
Copy link
Contributor Author

o11c commented Oct 1, 2015

@JukkaL note that I still have to xfail 4 of the stubs, even though they are correct. Obviously there's still a cyclic import problem somewhere.

@o11c
Copy link
Contributor Author

o11c commented Oct 11, 2015

Rebased on top of #903 for easier maintenance of my other branches.

But seriously, this is embarrassing.

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 11, 2015

@o11c: This PR has taken me a lot of work to review, and because it has sometimes felt like a burden it's also been slow. I've done some reflection and tried to understand why this happened and how we can make this less likely to happen in the future. Here are my thoughts: http://www.mypy-lang.org/wiki/GoodPullRequest

Anyway, I'm going to do another round of review today and hopefully I can finally merge it.

@JukkaL JukkaL merged commit 46cd254 into python:master Oct 11, 2015
@o11c o11c deleted the driver branch October 11, 2015 19:29
@JukkaL JukkaL mentioned this pull request Oct 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants