tahoe-lafs/trac

modernize tests #2777

New issue

Open

opened 2016-04-12 19:28:36 +00:00 by warner · 11 comments

warner commented

2016-04-12 19:28:36 +00:00

[remove the use of mock in favor of proper test doubles]ticket:3519
switch from trial's TestCase to testtools' TestCase (probably via a base class in one of our common modules)
replace JUnit-style assertions (assertEqual(...)) with "hamcrest" style assertions (assertThat(x, Equals(y)))
switch to async def / await to flatten the control-flow nest of helper functions

I'd like to clean up our unit tests.. there are a lot of archaic idioms that have crept in (partly because I'm not up-to-date on modern practices, partly because the tests were initially written before those practices were established). Here are some things that come to mind: * [remove the use of `mock` in favor of proper test doubles]ticket:3519 * switch from trial's [TestCase](wiki/TestCase) to testtools' [TestCase](wiki/TestCase) (probably via a base class in one of our common modules) * replace JUnit-style assertions (`assertEqual(...)`) with "hamcrest" style assertions (`assertThat(x, Equals(y))`) * switch to `async def` / `await` to flatten the control-flow nest of helper functions

warner added the

labels 2016-04-12 19:28:36 +00:00

warner added this to the undecided milestone 2016-04-12 19:28:36 +00:00

warner commented

2016-04-12 19:38:18 +00:00

Author

#2465 is where we removed "mock" about 8 months ago (mostly because it wanted a modern setuptools, but we had zetuptoolz, a reason that has gone away).

meejah commented

2016-04-18 23:43:57 +00:00

It also might be fruitful to look at changing at least some of the *MixIn "helper" classes to be 'just' helper methods instead.

By which I mean that if a test-suite wants a particular helper, instead of inheriting from the MixIn and using self.{various state} idioms, it might be better to have a "create_whatever_helper" and assign it to self.whatever in the setUp of any test-suites that need them.

For example, to use the "no network" test helper, you currently:

subclass GridTestMixin
make sure you called its setUp properly
call self.set_up_grid at some point (but after the above)
access the grid state via self.g and friends (which are set in set_up_grid) or any of the dozen methods added to self by the mix-in

Instead, it might be more-clear to take the functionality of set_up_grid and make it a bare method that creates you some helper object off of which hang any interesting helper methods for the test. Something like:

change class [GridTestMixin](wiki/GridTestMixin) into class [NoNetworkGrid](wiki/NoNetworkGrid)
turn set_up_grid into a bare factory method that creates, initializes and returns a NoNetworkGrid (could be async if needed)
...keep all the other state + helper functions it already has

Then, to use this a test-suite would:

do something like self.grid = create_no_network_grid() in its setUp
access + change grid state via self.grid.*

It also might be fruitful to look at changing at least some of the *MixIn "helper" classes to be 'just' helper *methods* instead. By which I mean that if a test-suite wants a particular helper, instead of inheriting from the [MixIn](wiki/MixIn) and using `self.{various state}` idioms, it might be better to have a "create_whatever_helper" and assign it to `self.whatever` in the `setUp` of any test-suites that need them. For example, to use the "no network" test helper, you currently: - subclass `GridTestMixin` - make sure you called its `setUp` properly - call `self.set_up_grid` at some point (but after the above) - access the grid state via `self.g` and friends (which are set in `set_up_grid`) or any of the dozen methods added to `self` by the mix-in Instead, it might be more-clear to take the functionality of `set_up_grid` and make it a bare method that creates you some helper object off of which hang any interesting helper methods for the test. Something like: - change `class [GridTestMixin](wiki/GridTestMixin)` into `class [NoNetworkGrid](wiki/NoNetworkGrid)` - turn `set_up_grid` into a bare factory method that creates, initializes and returns a `NoNetworkGrid` (could be async if needed) - ...keep all the other state + helper functions it already has Then, to use this a test-suite would: - do something like `self.grid = create_no_network_grid()` in its `setUp` - access + change grid state via `self.grid.*`

warner commented

2016-04-22 16:42:18 +00:00

Author

Glyph also pointed me at newer assertion methods, like:

f = self.failureResultOf(d, *expectedExceptionTypes): passes iff the Deferred has already errbacked, with one of the given types
res = self.successResultOf(d): passes iff the Deferred has already callbacked
self.assertNoResult(d): passes iff the Deferred has not been fired yet

The general pattern is to avoid waiting for Deferreds as much as possible. You call the method that returns a Deferred (with mocks in place for everything it might wait upon), then you manually trigger those mocks. Before, during, and after the triggers, you keep inspecting the Deferred to make sure it's in the right state.

Glyph also pointed me at newer assertion methods, like: * `f = self.failureResultOf(d, *expectedExceptionTypes)`: passes iff the Deferred has already errbacked, with one of the given types * `res = self.successResultOf(d)`: passes iff the Deferred has already callbacked * `self.assertNoResult(d)`: passes iff the Deferred has not been fired yet The general pattern is to avoid waiting for Deferreds as much as possible. You call the method that returns a Deferred (with mocks in place for everything it might wait upon), then you manually trigger those mocks. Before, during, and after the triggers, you keep inspecting the Deferred to make sure it's in the right state.

warner commented

2016-04-22 16:44:59 +00:00

Author

Oh, and for reference, the signature of assertFailure (when using inlineCallbacks) is:

e = yield self.assertFailure(d, *expectedFailures): wait for d to errback, require that it errbacked with one of the given exception types, and yield the exception. Fail if d is callbacked instead of being errbacked.

Oh, and for reference, the signature of assertFailure (when using inlineCallbacks) is: * `e = yield self.assertFailure(d, *expectedFailures)`: wait for `d` to errback, require that it errbacked with one of the given exception types, and yield the exception. Fail if `d` is callbacked instead of being errbacked.

Brian Warner <warner@lothar.com> commented

2016-04-26 18:24:32 +00:00

Owner

In 84a1064/trunk:

setup.py: depend on 'mock' when using [test] extra

I think this is useful enough that we should have it available when
running tests.

refs ticket:2777

In [84a1064/trunk](/tahoe-lafs/trac/commit/84a1064b877127a1d9b291300303b6d6639ef30f): ``` setup.py: depend on 'mock' when using [test] extra I think this is useful enough that we should have it available when running tests. refs ticket:2777 ```

exarkun changed title from ~~modernize tests, use "mock"~~ to modernize tests

2020-01-13 20:03:40 +00:00

exarkun commented

2020-01-13 20:05:00 +00:00

I edited this to reverse the requirement of using mock to a requirement of not using mock. mock is a unit testing anti-pattern and it should be removed from tahoe's test suite completely.

warner commented

2020-01-15 19:58:28 +00:00

Author

I'm intrigued (and out-of-date, as usual).. do you have a pointer handy on how "testing doubles" work, vs Mock?

exarkun commented

2020-01-18 12:13:54 +00:00

Maybe https://nedbatchelder.com/blog/201206/tldw_stop_mocking_start_testing.html to start

Maybe <https://nedbatchelder.com/blog/201206/tldw_stop_mocking_start_testing.html> to start

exarkun commented

2020-01-20 13:47:37 +00:00

Also relevant

Also relevant * <https://pythonspeed.com/articles/verified-fakes/> * <https://martinfowler.com/articles/mocksArentStubs.html>

exarkun commented

2020-11-24 18:23:14 +00:00

Splitting the mock part of this off into https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3519

Splitting the mock part of this off into <https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3519>

exarkun commented

2022-07-01 14:52:48 +00:00

I updated the point about inlineCallbacks to instead talk about async def and await because the latter is increasingly widely used and so it makes the codebase more approachable for non-Twisted experts.

I updated the point about `inlineCallbacks` to instead talk about `async def` and `await` because the latter is increasingly widely used and so it makes the codebase more approachable for non-Twisted experts.