Opened at 2016-04-12T19:28:36Z
Last modified at 2022-07-01T14:52:48Z
#2777 new task
modernize tests
Reported by: | warner | Owned by: | |
---|---|---|---|
Priority: | minor | Milestone: | undecided |
Component: | code | Version: | 1.11.0 |
Keywords: | Cc: | ||
Launchpad Bug: |
Description (last modified by exarkun)
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
- 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
Change History (14)
comment:1 Changed at 2016-04-12T19:38:18Z by warner
comment:2 Changed at 2016-04-18T23:43:57Z by meejah
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 into class 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.*
comment:3 Changed at 2016-04-22T16:42:18Z by warner
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.
comment:4 Changed at 2016-04-22T16:44:59Z by warner
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.
comment:5 Changed at 2016-04-26T18:24:32Z by Brian Warner <warner@…>
In 84a1064/trunk:
comment:6 Changed at 2020-01-13T20:03:40Z by exarkun
- Description modified (diff)
- Summary changed from modernize tests, use "mock" to modernize tests
comment:7 Changed at 2020-01-13T20:04:24Z by exarkun
- Description modified (diff)
comment:8 Changed at 2020-01-13T20:05:00Z by exarkun
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.
comment:9 Changed at 2020-01-15T19:58:28Z by warner
I'm intrigued (and out-of-date, as usual).. do you have a pointer handy on how "testing doubles" work, vs Mock?
comment:10 Changed at 2020-01-18T12:13:54Z by exarkun
comment:11 Changed at 2020-01-20T13:47:37Z by exarkun
comment:12 Changed at 2020-11-24T18:23:14Z by exarkun
- Description modified (diff)
Splitting the mock part of this off into https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3519
comment:13 Changed at 2022-07-01T14:51:26Z by exarkun
- Description modified (diff)
comment:14 Changed at 2022-07-01T14:52:48Z by exarkun
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.
#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).