CodeSOD: Breaking Changes

We talk a lot about the sort of wheels one shouldn't reinvent. Loads of bad code stumbles down that path. Today, Mary sends us some code from their home-grown unit testing framework.

Mary doesn't have much to say about whatever case of Not Invented Here Syndrome brought things to this point. It's especially notable that this is Python, which comes, out of the box, with a perfectly serviceable unittest module built in. Apparently not serviceable enough for their team, however, as Burt, the Lead Developer, wrote his own.

His was Object Oriented. Each test case received a TestStepOutcome object as a parameter, and was expected to return that object. This meant you didn't have to use those pesky, readable, and easily comprehensible assert" methods. Instead, you just did your test steps and called something like:

 outcome.setPassed()

Or

 outcome.setPassed(False)

Now, no one particularly liked the calling convention of setPassed(False), so after some complaints, Burt added a setFailed method. Developers started using it, and everyone's tests passed. Everyone was happy.

At least, everyone was happy up until Mary wrote a test she expected to see fail. There was a production bug, and she could replicate it, step by step, at the Python REPL. So that she could "catch" the bug and "prove" it was dead, she wanted a unit test.

The unit test passed.

The bug was still there, and she continued to be able to replicate it manually.

She tried outcome.setFailed() and outcome.setFailed(True) and outcome.setFailed("OH FFS THIS SHOULD FAIL"), but the test passed. outcome.setPassed(False), however" worked just like it was supposed to.

Mary checked the implementation of the TestStepOutcome class, and found this:

class TestStepOutcome(object): def setPassed(self, flag=True): self.result = flag def setFailed(self, flag=True): self.result = flag

Yes, in Burt's reluctance to have a setFailed message, he just copy/pasted the setPassed, thinking, "They basically do the same thing." No one checked his work or reviewed the code. They all just started using setFailed, saw their tests pass, which is what they wanted to see, and moved on about their lives.

Fixing Burt's bug was no small task- changing the setFailed behavior broke a few hundred unit tests, proving that every change breaks someone's workflow.

[Advertisement] Forget logs. Next time you're struggling to replicate error, crash and performance issues in your apps - Think Raygun! Installs in minutes. Learn more.