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

Remove myunit #4369

Merged
merged 12 commits into from Feb 6, 2018

Conversation

Projects
None yet
5 participants
@elazarg
Contributor

elazarg commented Dec 15, 2017

This should conclude #1673. It uses unittest.TestCase as a shortcut, as suggested by @JukkaL. It is aliased as Suite.

The transition is crude, aims at functionality; I intend to clean it up in a later PRs.

  • Helper functions moved from from myunit.__init__ to test.helpers. I think some of the asserts should be changed to simple assert to take advantage of pytest's introspection.
  • BaseTestCase is inlined (and perhaps was never needed).
  • testargs fails without the change to option.py. I'm not sure why it hadn't fail before.
  • pytest fixtures are not used yet

As in previous changes, using pytest -n0 --collect-only can give you the list of tests; diffing it with the result of the same command on master shows difference of 155 test items. grep ' def test_' mypy/test/test*.py | wc -l gives 162 items, and there are 7 skipped items.

@ethanhs

This comment has been minimized.

Collaborator

ethanhs commented Dec 15, 2017

This is excellent! I have a branch that moves several of the assert functions to bare asserts, if that is desired. Also, I believe the original idea was that replacement of the test framework would be concluded when runtests.py was replaced entirely by pytest? So I don't think this quite concludes #1673. But it certainly is great progress!

@elazarg

This comment has been minimized.

Contributor

elazarg commented Dec 15, 2017

@ethanhs thanks! Yeah, maybe "conclude" is not accurate. I hope this PR does not conflict with your branch, it will be very useful.

@ethanhs

This comment has been minimized.

Collaborator

ethanhs commented Dec 15, 2017

@elazarg well, I just checked and it already has quite a few conflicts with master, so this branch can't hurt more than the conflicts that already exist. No worries :)

@ethanhs

Just a few questions and comments on the changes. Thanks again!

ROADMAP.md Outdated
@@ -19,9 +19,6 @@ more accurate.
- Continue making error messages more useful and informative.
([issue](https://github.com/python/mypy/labels/topic-usability))
- Switch completely to pytest and remove the custom testing framework.

This comment has been minimized.

@ethanhs

ethanhs Dec 16, 2017

Collaborator

I think we should keep this until we can run all the tests with just pytest

@@ -308,3 +249,52 @@ def retry_on_error(func: Callable[[], Any], max_wait: float = 1.0) -> None:
# Done enough waiting, the error seems persistent.
raise
time.sleep(wait_time)
class AssertionFailure(Exception):

This comment has been minimized.

@ethanhs

ethanhs Dec 16, 2017

Collaborator

This Exception and the assert*s below are the targets I mentioned replacing. They entirely duplicate pytest functionality.

This comment has been minimized.

@elazarg

elazarg Dec 16, 2017

Contributor

Do you want me to do anything about this in current PR? I wanted to leave these changes to the following PRs.

This comment has been minimized.

@ethanhs

ethanhs Dec 17, 2017

Collaborator

No, I don't think that is needed.

from mypy.main import process_options
class ArgSuite(Suite):
def test_coherence(self) -> None:
options = Options()
_, parsed_options = process_options([], require_targets=False)
assert_equal(options, parsed_options)
assert options.__dict__.keys() == parsed_options.__dict__.keys()

This comment has been minimized.

@ethanhs

ethanhs Dec 16, 2017

Collaborator

Could you explain why you chose this method of checking equality? assert_equal is essentially assert not a != b, I believe this could be a bit different, and is not as straightforward.

This comment has been minimized.

@elazarg

elazarg Dec 16, 2017

Contributor

I've added these checks in order to find out why it failed. Since it was helpful for me, I thought it might be better not to switch back.

@@ -67,10 +67,8 @@ def test_interface_subtyping(self) -> None:
self.assert_equivalent(self.fx.f, self.fx.f)
self.assert_not_subtype(self.fx.a, self.fx.f)
@skip

This comment has been minimized.

@ethanhs

ethanhs Dec 16, 2017

Collaborator

Perhaps still leave the TODO item comment here and for the other @skips? I sometimes grep for them from time to time.

This comment has been minimized.

@elazarg

elazarg Dec 16, 2017

Contributor

I am thinking about skip as some kind of TODO... I can leave the TODO, but you might want to grep for skips too.

pytest.ini Outdated
@@ -14,7 +14,7 @@ python_files = test*.py
# Because we provide our own collection logic, disable the default
# python collector by giving it empty patterns to search for.
python_classes =
python_classes = *Suite

This comment has been minimized.

@ethanhs

ethanhs Dec 16, 2017

Collaborator

Perhaps it would be better to explicitly check via e.g. isinstance checks in the custom collector?

This comment has been minimized.

@elazarg

elazarg Dec 16, 2017

Contributor

The custom collector is for the data-driven tests. It doesn't know about tests derived from TestCase, and I think it might be awkward to make it handle the non-TestCase, non-data-driven tests. I am still thinking about the right way to collect tests, but I prefer to work on it in following PRs.

This comment has been minimized.

@JukkaL

JukkaL Feb 1, 2018

Collaborator

The comment above this line is no longer up-to-date.

@@ -129,7 +129,7 @@ def __init__(self) -> None:
self.scripts_are_modules = False
# Config file name
self.config_file = None # type: Optional[str]
self.config_file = 'setup.cfg' # type: Optional[str]

This comment has been minimized.

@ethanhs

ethanhs Dec 17, 2017

Collaborator

Isn't this just str now? (no longer Optional[str]?)

This comment has been minimized.

@elazarg

elazarg Dec 25, 2017

Contributor

I think it isn't. Looks like it's possible to have None if there's no config file; but when running the test, there is. I'm not sure about that though; I wonder why ArgSuite doesn't fail on master.

This comment has been minimized.

@msullivan

msullivan Jan 26, 2018

Collaborator

What's the reason for this change?

This comment has been minimized.

@JukkaL

JukkaL Feb 1, 2018

Collaborator

This change doesn't seem to be related to the rest of PR. Can you remove this?

This comment has been minimized.

@elazarg

elazarg Feb 2, 2018

Contributor

It is needed because (as far as I understand) pytest uses the config file, so the default option is not None. I'm not sure this change is the right one; another option I can think of is changing this value inside the test.

This comment has been minimized.

@JukkaL

JukkaL Feb 2, 2018

Collaborator

But this is the mypy config file, which I don't expect to be directly relevant for pytest. Pytest config file should be configured somewhere else (not sure how this works exactly, though) -- and pytest already has a config file (pytest.ini) which seems to be used. Does this PR do something that requires changes to this? Or maybe I'm talking about a different config file?

This comment has been minimized.

@elazarg

elazarg Feb 2, 2018

Contributor

I think I might have confused something in my previous response (it's been a while since I tried to figure this out), but note that SHARED_CONFIG_FILES = ('setup.cfg',) is hardcoded in main.py. Somewhere in parse_config_file() there's probably a difference between how myunit and pytest work, so one the former uses None and the latter uses 'setup.cfg'.

This comment has been minimized.

@elazarg

elazarg Feb 2, 2018

Contributor

Running myunit, os.path.exists('setup.cfg') is false. Running pytest, it is true.

This comment has been minimized.

@elazarg

elazarg Feb 2, 2018

Contributor

I'm trying a different approach - simply create a new directory and chdir into it.

This comment has been minimized.

@elazarg

elazarg Feb 2, 2018

Contributor

Failed - changing directories did not work on Windows.

@elazarg

This comment has been minimized.

Contributor

elazarg commented Jan 6, 2018

I'm not sure why the conflicts don't go away.

@ethanhs

This comment has been minimized.

Collaborator

ethanhs commented Jan 6, 2018

For some reason I've had to fix merge conflicts twice for github to register it. This seems like a bug in Github. I went ahead and did the second merge, things seem to not conflict now.

elazarg added some commits Jan 6, 2018

@msullivan

This comment has been minimized.

Collaborator

msullivan commented Jan 26, 2018

This looks pretty reasonable to me. It's always nice when you can delete a lot of code. @JukkaL, is there any reason to not merge this?

@JukkaL

This comment has been minimized.

Collaborator

JukkaL commented Jan 29, 2018

I had look at this a while ago and it looked good, but then I got distracted by fine-grained incremental work and unfortunately didn't finish my review. I'll do another pass this week. Sorry about the slow turnaround -- it will great be to finally drop myunit!

@JukkaL

Looks good, mostly some minor things. Thanks for the cleanup!

The typeshed change seems unrelated.

@@ -129,7 +129,7 @@ def __init__(self) -> None:
self.scripts_are_modules = False
# Config file name
self.config_file = None # type: Optional[str]
self.config_file = 'setup.cfg' # type: Optional[str]

This comment has been minimized.

@JukkaL

JukkaL Feb 1, 2018

Collaborator

This change doesn't seem to be related to the rest of PR. Can you remove this?

def __init__(self, *, update_data: bool) -> None:
self.update_data = update_data
# Assigned from MypyDataCase.runtest
update_data = False # type: bool

This comment has been minimized.

@JukkaL

JukkaL Feb 1, 2018

Collaborator

Style nit: redundant type annotation (you can also remove the str and bool annotations above).

@@ -312,6 +255,55 @@ def retry_on_error(func: Callable[[], Any], max_wait: float = 1.0) -> None:
time.sleep(wait_time)
class AssertionFailure(Exception):

This comment has been minimized.

@JukkaL

JukkaL Feb 1, 2018

Collaborator

You could probably use the built-in exception AssertionError instead of defining a new class here.

super().__init__()
def assert_true(b: bool, msg: Optional[str] = None) -> None:

This comment has been minimized.

@JukkaL

JukkaL Feb 1, 2018

Collaborator

Add comment about assert_true and assert_false being redundant (we could just use the assert statement).

from mypy.constraints import SUPERTYPE_OF, SUBTYPE_OF, Constraint
from mypy.solve import solve_constraints
from mypy.test.typefixture import TypeFixture
from mypy.types import Type, TypeVarType, TypeVarId
class SolveSuite(Suite):
def __init__(self) -> None:
super().__init__()
def setUp(self) -> None:

This comment has been minimized.

@JukkaL

JukkaL Feb 1, 2018

Collaborator

Elsewhere we had a setup method, which is a bit confusing. This is needed because of unittest. I wonder if it make sense to use this name variant uniformly?

This comment has been minimized.

@elazarg

elazarg Feb 2, 2018

Contributor

If you insist, but I'm not sure that's desirable. The naming distinction makes it clear where it's required by unittest and where it isn't.

This comment has been minimized.

@JukkaL

JukkaL Feb 2, 2018

Collaborator

Ok no need to change this. If this becomes a problem we can reconsider.

from mypy.myunit import AssertionFailure
import pytest # type: ignore # no pytest in typeshed
from unittest import TestCase as Suite

This comment has been minimized.

@JukkaL

JukkaL Feb 1, 2018

Collaborator

Add comment about this, since this may be kind of unexpected. In the future we'd probably want to directly import TestCase in test files and inherit from it instead of the Suite alias (and maybe rename some of the other helper classes to avoid ambiguity).

pytest.ini Outdated
@@ -14,7 +14,7 @@ python_files = test*.py
# Because we provide our own collection logic, disable the default
# python collector by giving it empty patterns to search for.
python_classes =
python_classes = *Suite

This comment has been minimized.

@JukkaL

JukkaL Feb 1, 2018

Collaborator

The comment above this line is no longer up-to-date.

elazarg added some commits Feb 2, 2018

Follow review
* remove redundant type declarartions
* comment about aliasing
* s/AssertionFailure/AssertionError/g
* Fix comment in pytest.ini
@JukkaL

JukkaL approved these changes Feb 6, 2018

LGTM now! Thanks for the updates!

@JukkaL JukkaL merged commit 7ba2df4 into python:master Feb 6, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@JelleZijlstra

This comment has been minimized.

Collaborator

JelleZijlstra commented Feb 6, 2018

Hooray!

carljm added a commit to carljm/mypy that referenced this pull request Feb 14, 2018

Merge branch 'master' into import-cycle
* master: (32 commits)
  Fix some fine-grained cache/fswatcher problems (python#4560)
  Sync typeshed (python#4559)
  Add _cached suffix to test cases in fine-grained tests with cache (python#4558)
  Add back support for simplified fine-grained logging (python#4557)
  Type checking of class decorators (python#4544)
  Sync typeshed (python#4556)
  When loading from a fine-grained cache, use the real path, not the cached (python#4555)
  Switch all of the fine-grained debug logging to use manager.log (python#4550)
  Caching for fine-grained incremental mode (python#4483)
  Fix --warn-return-any for NotImplemented (python#4545)
  Remove myunit (python#4369)
  Store line numbers of imports in the cache metadata (python#4533)
  README.md: Fix a typo (python#4529)
  Enable generation and caching of fine-grained dependencies from normal runs (python#4526)
  Move argument parsing for the fine-grained flag into the main arg parsing code (python#4524)
  Don't warn about unrecognized options starting with 'x_' (python#4522)
  stubgen: don't append star arg when args list already has varargs appended (python#4518)
  Handle TypedDict in diff and deps (python#4510)
  Fix Options.__repr__ to not infinite recurse (python#4514)
  Fix some fine-grained incremental bugs with newly imported files (python#4502)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment