Fix two test failures with latest mock

[koobs]

Since it's easy to forget, because currently setup.py:tests_require = mock == x, this is a task to address the two failures when running the stripe test suite with the latest version (currently 1.3.0) mock. Once solved, the dependency can then be updated to >=1.3.0, and stripe development can get back to proactively testing itself against newer versions of mock in the CI environment

=================================================== FAILURES 
___________________________________ ChargeRefundTest.test_non_recursive_save 

self = <stripe.test.resources.test_refunds.ChargeRefundTest testMethod=test_non_recursive_save>

    def test_non_recursive_save(self):
        charge = stripe.Charge.construct_from({
            'id': 'ch_nested_update',
            'customer': {
                'object': 'customer',
                'description': 'foo',
            },
            'refunds': {
                'object': 'list',
                'url': '/v1/charges/ch_foo/refunds',
                'data': [{
                    'id': 'ref_123',
                }],
            },
        }, 'api_key')

        charge.customer.description = 'bar'
        charge.refunds.has_more = True
        charge.refunds.data[0].description = 'bar'
        charge.save()

>       self.requestor_mock.request.assert_not_called()

stripe/test/resources/test_refunds.py:98:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

_mock_self = <Mock name='APIRequestor().request' id='34490831952'>

    def assert_not_called(_mock_self):
        """assert that the mock was never called.
            """
        self = _mock_self
        if self.call_count != 0:
            msg = ("Expected '%s' to not have been called. Called %s times." %
                   (self._mock_name or 'mock', self.call_count))
>           raise AssertionError(msg)
E           AssertionError: Expected 'request' to not have been called. Called 1 times.

/usr/local/lib/python2.7/site-packages/mock/mock.py:915: AssertionError

_________________________________ UpdateableAPIResourceTests.test_save_nothing 

self = <stripe.test.resources.test_updateable.UpdateableAPIResourceTests testMethod=test_save_nothing>

    def test_save_nothing(self):
        acct = MyUpdateable.construct_from({
            'id': 'myid',
            'metadata': {
                'key': 'value',
            }
        }, 'mykey')

        self.assertTrue(acct is acct.save())
>       self.requestor_mock.request.assert_not_called()

stripe/test/resources/test_updateable.py:108:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

_mock_self = <Mock name='APIRequestor().request' id='34494269168'>

    def assert_not_called(_mock_self):
        """assert that the mock was never called.
            """
        self = _mock_self
        if self.call_count != 0:
            msg = ("Expected '%s' to not have been called. Called %s times." %
                   (self._mock_name or 'mock', self.call_count))
>           raise AssertionError(msg)
E           AssertionError: Expected 'request' to not have been called. Called 1 times.

/usr/local/lib/python2.7/site-packages/mock/mock.py:915: AssertionError
=============================== 2 failed, 203 passed, 9 skipped in 45.19 seconds

[ghost]

I think, this issue can be related — #189

[koobs]

Mmm, indeed, I should have noticed that.

@alex-downtown Is that FreeBSD for #189 ?

[ghost]

Tested on OS X, but reproduces on Linux as well.

[olivierbellone]

cc @brandur @stripe/api-libraries

Hey @koobs, thanks for reporting this. I've investigated a bit and here's what I found.

As you noticed, we currently force mock == 1.0.1. This version does not define assert_not_called(). It seems calling a non-existent method on a Mock object will return another Mock object, so the two tests that use assert_not_called() do not actually assert anything!

The assert_not_called() method is defined in mock 1.1.0 and above. When upgrading mock to a version that supports assert_not_called(), the tests fail properly: the error is in the tested methods and not in the tests themselves.

For instance, in test_non_recursive_save(), calling charge.save() should not trigger a request to the API, but it does. After digging a bit, I think the problem is in StripeObject.serialize(). This method will always return a dict with a key for every sub-object and an empty dict as the value, even if the sub-object was unchanged. As a result, UpdateableAPIResource.save() will send an update request.

This seems non-trivial to fix, as we also need to be able to differentiate between a not-updated sub-object and a sub-object that we explicitly want to set to an empty dict to reset it (see UpdateableAPIResourceTests.test_array_none().

To illustrate:

>>> import stripe
>>> stripe.api_key = 'sk_test_...'
>>> ch = stripe.Charge.create(amount=400, currency='usd', customer='cus_...')
>>> ch.serialize(None)
{u'fraud_details': {}, u'source': {u'metadata': {}}, u'refunds': {}, u'metadata': {}}```
>>> # The above should be {}
>>> ch.save()
>>> # The above does send a POST update request to the API with an empty body

[brandur]

@olivierbellone Thanks for digging into this! So tests were written for the library that could never be reasonably expected to pass? Ugh.

Do you think it might be possible to update the tests to assert on what would actually happen (i.e. non-empty serializations)? Obviously not ideal because it doesn't seem to be how the library was designed to work, but such a fix might allow us to get mock upgraded and give us some breathing room to take a look at the other issue.

[kyleconroy]

So I think there are two things here. First, we should upgrade mock to `>=1.3.0', even though it causes the tests to fail. This represents where we are now, so we don't want to hide that fact anymore. We can fix the actual failing tests in another PR.

[brandur]

So I think there are two things here. First, we should upgrade mock to `>=1.3.0', even though it causes the tests to fail. This represents where we are now, so we don't want to hide that fact anymore. We can fix the actual failing tests in another PR.

Right on. It does seem like our current approach with mock versioning is just sweeping the problem under the rug.

IMO though, we should wait until someone is committed to fixing the failing tests before applying this change. Even if the tests run in their current state are wrong, having a green test suite is still useful in that it allows us to check integration against other unrelated changes.

[koobs]

@kyleconroy +1

[olivierbellone]

I created a PR (#207) to use the latest mock version and fix the failing tests. I think we can close #189 and this one once it's merged, and create another issue about the undesired behavior of the library that I described above.

[kyleconroy]

@olivierbellone 👍

[koobs]

Awesome stuff, thank you all :)