assert_called_once: Threat or Menace

I remember the first time I laid eyes on the beast: Summer, 2012. The air conditioning at Yelp HQ hummed imperceptibly as I reviewed code for a colleague. This wasn’t my first rodeo, but I was new to Yelp and to working in a large Python codebase. I painstakingly scanned the code for lurking bugs, but couldn’t find any. “Ship it!” I declared electronically, freeing my colleague to deploy his changes.

It is chilling to think that on that day, I looked the beast squarely in the eyes and never realized it. Cunning camouflage allowed it to slip past me and into production.

Hunt the Wumpus

So what is this horrific beast? I will show you, and you still might not spot it! Imagine you are that starry-eyed engineer from the Summer of ‘12 and you’re reviewing production code that looks something like this:

def restart_server(server): """Safely restart a server by removing it from the load balancer, temporarily silencing its monitoring, telling its mom it will be home before dark, etc. (The implementation doesn't matter so we'll leave it out, but assume it makes remote API calls and invokes system commands and generally performs expensive operations.) """ pass def restart_servers_in_datacenter(servers, datacenter): for server in servers: if server.endswith("-%s" % datacenter): restart_server(server)

This feature even comes with a unit test:

import mock module_under_test = __name__ def test_restart_servers_in_datacenter_with_match(): servers = set(["web1-sfo", "web2-phx", "web3-jfk"]) with mock.patch("%s.restart_server" % module_under_test) as mock_restart: restart_servers_in_datacenter(servers, "sfo") mock_restart.assert_called_once()

The main idea is that restarting actual servers whenever someone runs the test suite is not a great strategy. So instead of calling the real restart_server(), we use mock.patch() from the Python Mock library to swap in a test double. The fake restart_server(), which we call mock_restart, is a MagicMock which remembers how it has been called and provides convenience methods like assert_called_once_with to validate its experiences.

The code review includes output showing that this test passes. You can paste the code above into a file and run it with py.test if you don’t trust me:

Did you spot the beast?

It is Pitch Black

Here’s a clue: the test will happily continue to pass if you change the assertion to:

mock_restart.assert_two_wrongs_dont_make_a_right()

Or:

mock_restart.assert_two_plus_two_equals(5)

Or even:

mock_restart.ASDFASDFASDFASDFASDFASDFASDF()

What’s going on here?

Remember that a mock’s job is to say, “You got it, boss” whenever anyone calls it. It will do real work, like raising an exception, when one of its convenience methods is called, like assert_called_once_with. But it won’t do real work when you call a method that only resembles a convenience method, such as assert_called_once (no _with!).

You Are Likely to be Eaten by a Grue

More troubling, the test passes even if you remove the call to restart_server() from the code-under-test entirely!

This should make your blood run cold. Our test looks reasonable, emits a green “1 test passed,” but doesn’t test anything!

Production code with no test coverage is pretty bad, but this test actually has negative value. Someone spent energy writing it. More engineers will spend energy reading it. Future maintainers, not realizing that they are bedding down with the beast, will add features based on this test and these new features will also be untested. Unwary newcomers will see the useful-looking but ultimately poisonous assert_called_once() and spread it to other modules.

Kill The Beast

Now that we know the beast and how it ticks, how can we defeat it?

Fear of Commitment

Fellow Yelpers Cheng C. and Nicholas N. added a check to our pre-commit hooks that uses the compiler module to walk the AST looking for assert_called_once and a few other disguises commonly used by the beast. If the beast’s signature is detected, the commit is rejected.[1]

This band-aid was cheap to implement and provides a final, automated chance to slay the beast before it can sneak into the master branch. Blacklists are a crude instrument, however, so this hook is not a perfect defense.

You Can Go Your Own Way

After my colleague Keith M.’s first encounter with the beast (pointed out to him by battle-hardened Autumn 2014 me in a code review), he vowed to avoid the Mock convenience methods entirely:

I’ve switched to an idiom of checking mock objects that should be a lot more resilient to the fact that they are pernicious liars.

So instead of:

mock_restart.assert_called_once_with("web1-sfo")

Keith writes:

assert mock_restart.call_count == 1

assert mock_restart.call_args == mock.call(“web1-sfo”)

His rationale:

The helper method saves a line, but calling things on mocks and expecting them to have side effects (like raising an error) is just asking to get bit by typos and misspellings. Meanwhile, it’s much harder for a mock to accidentally end up with very specific values occupying a random attribute.

Glasses for Your Subaru?

The authors of Mock are aware of the beast and have built a defense into the library – autospec:

If you use a class or instance as the spec for a mock then you can only access attributes on the mock that exist on the real class.

If we change our test to use the autospec defense:

Then the beast is exposed!

$ py.test beast.py ============================================================================ test session starts ============================================================================ platform linux2 -- Python 2.6.7 -- py-1.4.20 -- pytest-2.5.2 collected 1 items beast.py F ============================================================================ FAILURES ============================================================================ _________________________________________________________________ test_restart_servers_in_datacenter_with_match _________________________________________________________________ E AttributeError: 'function' object has no attribute 'assert_called_once' beast.py:22: AttributeError ============================================================================ 1 failed in 0.01 seconds ============================================================================

autospec is very effective… if you remember to use it. It’s not on by default due to “ caveats and limitations”, so the beast relies on the uninitiated and forgetful to avoid being trapped by autospec.

Test-Driven Defense

The best defense against the beast is not a blacklist or a workaround or a clever library feature. It is a change in how you write software.

When I see a test containing a bogus method like assert_called_once, it tells me that the author never verified that the test could fail. A test that cannot fail is no test at all.

The easiest way to insure that your test can fail is to write the test first, watch it fail, then write production code until the test passes. This is Test-Driven Development (TDD) and in addition to being a relatively foolproof means of defeating the beast, it is the best way I know to write well-tested, well-factored, maintainable software.

TDD is what allowed me to outsmart the beast the first time. While adding a feature, I had copied from an existing, beast-infiltrated test. But because I had written the test first, I knew something was amiss when the test suite cheerfully passed. Suspicion and ipdb soon laid bare the beast’s tricks, and now I pass my experiences on to you.

So go, my friends, into the great wilderness of software. May your test suite remain faithful, honest, and beast-free.

[1] You can see this hook at https://gist.github.com/mrtyler/995fcb4282a9d15de625.

#!/usr/bin/python import compiler import compiler.ast import optparse import sys class MockChecker(object): def __init__(self): self.errors = 0 self.current_filename = "" self.non_existent_methods = [ 'assert_calls', 'assert_not_called', 'assert_called', 'assert_called_once', 'not_called', 'called_once', 'called_once_with', ] def check_files(self, files): for file in files: self.check_file(file) def check_file(self, filename): self.current_filename = filename try: ast = compiler.parseFile(filename) except SyntaxError, error: print >>sys.stderr, "SyntaxError on file %s:%d" % (filename, error.lineno) return compiler.walk(ast, self) def visitGetattr(self, node): if node.attrname in self.non_existent_methods: print >>sys.stderr, "%s:%d: you may have called a nonexistent method on mock" % (self.current_filename, node.lineno) self.errors += 1 def main(): parser = optparse.OptionParser(usage="%prog [options] file [files]", description="Checks that the test file does not contain non-existent mock methods") (opts, files) = parser.parse_args() if len(files) == 0: parser.error("No filenames provided") checker = MockChecker() checker.check_files(files) return 1 if checker.errors else 0 if __name__ == '__main__': sys.exit(main())

Back to blog

Yelp

Engineering