Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use py.test for tests #384

Merged
merged 1 commit into from Feb 21, 2018
Merged

Use py.test for tests #384

merged 1 commit into from Feb 21, 2018

Conversation

ob-stripe
Copy link
Contributor

r? @brandur-stripe
cc @stripe/api-libraries

Apologies for yet another big PR 馃槶 (although the overall line count is negative, which is always nice)

This PR replaces the unittest2 test framework with pytest, which is more modern and the current state of the art in Python.

Some advantages of using pytest:

  • assertions are written using the assert keyword (no more non-PEP8-compliant self.assertSomething methods)
    • pytest uses some advanced introspection magic to display helpful messages when an assertion fails
  • automatic stdout/stderr/log capturing
  • better fixtures system

Although many lines of code were changed, most modifications are fairly simple:

  • renamed SomethingTests classes to TestSomething. This is necessary so that pytest can discover tests automatically, and has the nice side-effect of making the test class name better match the file name
  • changed self.assertSomething calls to regular asserts
  • the request_mock instance used for stripe-mock tests is now a pytest fixture, and is injected as such into every test method that uses it
  • mock is no longer used directly, but rather through the pytest-mock plugin which adds a mocker fixture
  • tests/helper.py was moved to tests/conftest.py (this is where pytest expects to find global fixtures)

Some of the tests (notably test_api_requestor.py and test_http_client.py) required some deeper changes. In particular, any helper method that manipulates fixtures needs to be converted to a fixture itself, with the fixture method returning the "regular" helper method (e.g. here). There might be a better way to handle this, maybe by using pytest's parametrization feature.

Although all tests are passing and I believe that the PR could be merged as-is, I've tagged it as WIP as I wonder if we should wait to drop support for Python 2.6 and 3.3 beforehand, as the latest versions of pytest and its plugins no longer support those Python versions.

@@ -44,13 +44,13 @@ before_install:

install:
- pip install -U setuptools pip
- pip install unittest2 mock pycurl flake8 tox-travis coveralls
- pip install pycurl flake8 coveralls
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tox-travis was useless as we don't use tox to run tests on Travis.

pytest and any additional test modules will be installed when running python setup.py install, so no need to install them manually before.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see this cleaned out.

@@ -102,6 +102,10 @@ def __init__(self, timeout=80, session=None, **kwargs):
self._timeout = timeout
self._session = session or requests.Session()

def __del__(self):
if self._session:
self._session.close()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under Python 3, requests will sometimes return ResourceWarnings after execution (cf. psf/requests#1882). There are some tests where we explicitly catch and count warnings (to check that we're raising DeprecationWarnings) and this bug/feature was interfering with the tests.

It's apparently not great practice to close resources in destructors, but it does fix the tests and I suppose it's better than doing nothing at all.

This should not be related to pytest though and I'm not entirely sure why we never ran into this issue before.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure either, but certainly a problem :$ This looks strictly better than what we had before. Unfortunately the "real" solution is probably to implement our own close method for users to call into manually.

tox.ini Outdated
# is not agnostic when asserting that a method was called with positional vs.
# keyword arguments.
# Once we drop support for Python 3.3, we can safely remove this line.
mock_use_standalone_module = true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default, pytest-mock will use the mock module that comes with the stdlib in Python 3. However, as the comment explains, in 3.3 the assert_called_with method is not agnostic when checking for positional vs. keyword arguments, which was causing our assert_requested method to fail for some tests depending on how api_requestor.request was called.

To fix this, I added the mock pypi module to setup.py for Python<=3.3 and the configuration line above.

I'd rather drop support for 3.3 entirely though and get rid of this workaround.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation and leaving the comment in here (the code).

@brandur-stripe
Copy link
Contributor

This is great OB. Such a huge amount of work too!

LGTM. Two questions:

  1. Did you happen to check if the error messages are as good as they were before? Like if I have a assert obj.id == 'mid', does it still fail by giving me the left-hand side and right-hand side values, or does it just give me a generic "assertion failed" now?
  2. There's a section in the README on how to run tests. Can you update that to include the new instructions?

@ob-stripe
Copy link
Contributor Author

Did you happen to check if the error messages are as good as they were before? Like if I have a assert obj.id == 'mid', does it still fail by giving me the left-hand side and right-hand side values, or does it just give me a generic "assertion failed" now?

Yep! Here's how it looks like:

E       AssertionError: assert 'mid' == 'foo'
E         - mid
E         + foo

For our custom assert_requested method, pytest still attempts to give more details than just the exception that we manually raise:

E           AssertionError: Expected call: request(<ANY>, 'get', '/v1/foo', <ANY>, <ANY>)
E           Actual call: request(<stripe.api_requestor.APIRequestor object at 0x11218cad0>, 'get', '/v1/charges', {})
E
E           pytest introspection follows:
E
E           Args:
E           assert (<stripe.api_.../charges', {}) == (<ANY>, 'get',... <ANY>, <ANY>)
E             At index 2 diff: '/v1/charges' != '/v1/foo'
E             Right contains more items, first extra item: <ANY>
E             Use -v to get the full diff

Before the error message, pytest also outputs the body of the method where the failed assertion happened. Unfortunately, for our assert_requested method it will output the body of assert_requested rather than the body of the test that calls assert_requested. This is likely fixable, but it's a minor QoL thing so I punted for now.

@ob-stripe
Copy link
Contributor Author

I've updated the README as well.

@brandur-stripe
Copy link
Contributor

Thanks Olivier! Explanations sounds reasonable to me.

I just restarted one of the Travis jobs from what looked like an intermittent failure, but I'd say this is good to come in if you want to strip the WIP tag and merge it.

@ob-stripe
Copy link
Contributor Author

Actually, there's a small issue with the PR in its current state. CI tests work because Travis environments for 2.6 and 3.3 come pre-installed with a pytest version that is still compatible with these Python versions, so running python setup.py test ... will use the preinstalled pytest (3.0.3).

However, when using a clean environment, python setup.py test ... will download the latest pytest version (3.3.1 currently) which is no longer compatible with 2.6 and 3.3 (cf. https://docs.pytest.org/en/latest/changelog.html#pytest-3-3-0-2017-11-23).

We could fix this with a bit of code in setup.py (or just by freezing the pytest version to 3.2.5, the latest compatible version), but I think we should consider dropping support for these Python versions entirely. I have a few other improvements that I'd like to ship too but that require a major version bump. Happy to discuss this further internally.

@ob-stripe
Copy link
Contributor Author

Actually, after writing the above I think that freezing pytest to 3.2.5 is actually a decent short-term fix. It would let us merge this PR and work on dropping support for deprecating versions separately. I'll amend the commit with this fix and remove the WIP tag.

@ob-stripe ob-stripe changed the title [WIP] Use py.test for tests Use py.test for tests Jan 4, 2018
@stripe-ci
Copy link

@ob-stripe ob-stripe changed the title Use py.test for tests [WIP] Use py.test for tests Jan 4, 2018
@ob-stripe
Copy link
Contributor Author

馃槧 Okay, this is more complex than I expected. Something must be different in the way pytest instantiates and deletes object in 3.2.5, because now tests on Python 3.4 are failing because of the self.session.close() call in the destructor.

Going to dig a bit more. Re-tagging as WIP for now.

@ob-stripe ob-stripe changed the base branch from master to integration-v2 February 21, 2018 11:30
@ob-stripe ob-stripe force-pushed the ob-pytest branch 3 times, most recently from 667948d to a1026ad Compare February 21, 2018 12:46
@ob-stripe ob-stripe changed the title [WIP] Use py.test for tests Use py.test for tests Feb 21, 2018
@ob-stripe
Copy link
Contributor Author

ob-stripe commented Feb 21, 2018

Alright, I've updated the merge target to integration-v2 rather than master. Going to pull this in.

@ob-stripe ob-stripe merged commit 578f9a7 into integration-v2 Feb 21, 2018
@ob-stripe ob-stripe deleted the ob-pytest branch February 21, 2018 13:08
@ob-stripe ob-stripe mentioned this pull request Feb 21, 2018
7 tasks
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.

None yet

3 participants