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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python3 #40

Open
wants to merge 14 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@buchi
Copy link
Member

commented Apr 17, 2019

Port package to Python 3.7 and Plone 5.2

MockTestCase is no longer based on plone.mocktestcase because it's no longer maintained and not compatible with Python 3. Thus I've ported most of the mock methods to this package.

Also mocker is replaced in favor of unittest.mock. This is a breaking change and may require amending existing tests based on MockTestCase.

mocker has record mode for recording expectations and a replay mode for using the mock in tests.
This is no longer the case with unittest.mock. Here's a simple example how mocker expectations can be adopted to unittest.mock.

# Mocking with mocker
mock = self.mocker.mock()  # mocker.Mock
self.expect(mock.lock()).result('already locked')
self.replay()
self.assertEqual(mock.lock(), 'already locked')
# Mocking with unittest.mock
mock = self.mock()  # unittest.mock.Mock
mock.lock.return_value = 'already locked'
self.assertEqual(mock.lock(), 'already locked')

I've also replaced forbiddenfruit and mocker in freezer as forbiddenfruit causes segmentation faults on Python 3.7. Should still work as before as I didn't have to touch the tests that much.

There are various minor changes to make the code compatible with Python 3.

@buchi

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

@jone MockTestCase currently provides a stub which has a subtle difference to a mock in that it does not assert expectations. With unittest.mock mocks do not assert expectations. You have to make assertions yourself after using the mock. So mocks behave the same as stubs.
I'm not sure how we want to deal with that. But with unittest.mock it doesn't make sense to differentiate between mocks and stubs. However I would keep all the stub methods to not have to modify existing tests.
Opinions?

@jone

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

In general, I'm a bit reluctant with mocks and stubs in tests, because it has lead to an overuse once, where we've written a lot of very bad tests. So I'd prefer that most of the current mock tests are rewritten to integration tests anyway. 😉

Since we need to change existing tests anyway with this change, I agree with your suggestion to keep the helper methods but go to unittest.mock way that it can be used as mock or stub.

buchi added some commits Apr 15, 2019

Replace forbiddenfruit and mocker in freezer
- mocker is not available for Python 3
- forbiddenfruit segfaults with Python 3
Fix GenericSetupUninstallMixin on Python 3
- Decode output from configuration comparison
- Skip quickinstaller uninstall on Plone 5.2 as it doesn't work
- Skip chooser.xml when comparing configurations
No longer base on plone.mocktestcase
It's no longer maintained and not compatible with Python 3
This is a *breaking* change and may require amending existing tests based on
``MockTestCase``.
Use less specific assertion
Error messages differ on Python 2 and 3

@buchi buchi force-pushed the python3 branch from b493ffe to c740de0 Apr 25, 2019

@buchi buchi marked this pull request as ready for review Apr 25, 2019

@buchi buchi requested a review from jone Apr 25, 2019

@jone

jone approved these changes May 23, 2019

Copy link
Member

left a comment

👍 Thanks for your work! This looks quite straight forward.

@@ -212,6 +212,6 @@ def test_commit_works_with_transactions(self):

thedate = self.layer['portal'].current_date
self.assertEquals(datetime.datetime(2015, 7, 22, 11, 45, 58), thedate)
self.assertEquals('datetime.datetime',
self.assertEquals('ftw.testing.freezer.FrozenDateTime',

This comment has been minimized.

Copy link
@jone

jone May 23, 2019

Member

Why did that change?
Is it a problem that it did change?

I remember that we had some problems at one point because e.g. type(value) == datetime.datetime was no longer True.

This comment has been minimized.

Copy link
@jone

jone May 23, 2019

Member

Discussion: we are going to ship that as it is and fix it when we run into errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.