Bugs are all over the tinyest piece of code. There is a reason why we need to test our code: in most cases, our code does not behave as we expect it to do. Causes range from poor understanding of the language internals (integer overflow, floating point rounding error, implicit type conversion aka coercion...) to bad algorithm implementations (e.g. forgetting to initialize a list of distances or not considering corner cases). A good test base will help us find most of these errors, by telling the test runner what we expect will happen given certain conditions.

However, tests won't tell how to fix an error, but they provide useful feedback to help us find its source (and most importantly, they assert that indeed there is an error). That's the reason why many code repositories won't accept a bug fix unless a test is provided for the error that it addresses. That's the case of this story.

First step: noticing a bug

As I mentioned in my last post about testing with Mocha and Chai, I came across an error in the chai-as-promised plugin while using it along the chai-spies plugin. It all happened after I added the following line to one of my tests files:

chai.use(chaiAsPromised);

In the aforementioned post I explained that this is where we wire the plugin to the Chai assertions library.

I required the plugin because I was about to write a test involving promises (field in which Domenic Denicola, the author of the plugin, is an expert), but boy was I surprised when not only that new test failed, but eleven more did!

I should have realized immediately where the conflict was, but my lack of focus at the moment led to a lot of head-scratching and trial and error before I actually confirmed the issue. I did not know which of the two plugins I was using caused the failure, but since chai-spies had been working correctly so far and chai-as-promised worked both with and without the other plugin I took my chances with the latter: submitted an issue in its repository, and a few hours later I got an answer from Domenic. It linked to a pull request which had not been accepted because it did not provide tests.

After cloning the repository and checking out the PR, I tried once again and it was perfectly fixed. Eager to help and excited to contribute to a project by Domenic, I decided to try my hand at isolating the bug and providing a test for it.

Second step: locating the bug

Since I already had a patch that fixed the bug, my problem was not locating it, but reproducing it. This patch was based on the assumption that chai-as-promised did not use correctly the Chai plugin API, so I decided that it would not help me very much locating the bug, because its consequences would be in the Chai code, not in the plugin's.

However, the Chai plugin API is simple enough to understand it in a sitting and looking at the pull request (which was very brief) I interpreted that chai-as-promised was doing a custom overwriting of existing chainable methods, instead of using a method provided by the API for that purpose. This was the fix implemented in the patch.

Third step: reproducing a minimal version of the bug

Fortunately, the chai-as-promised code is not very long, is all in a single file (as most Chai plugins are, if not all), and is well structured (well indented, meaningful variable names, comments on the "why's" of the code...). However this was not a very easy bug to find, because it was related to JavaScript property accessors. In Chai, there are properties defined to be used both as normal properties and as methods, those are called chainable methods:

expect(myFunction).to.have.been.called();
expect(myFunction).to.have.been.called.exactly(5);

See how called is used as a method, but also we use it to access one of its own methods? That's the meaning of "chainable", and it's the basis of Chai wonderfully self-descriptive assertions.

called is a chainable method introduced by chai-spies. It is also the one that would not work in my code after wiring chai-as-promised. Naturally, the first thing I did to try to find how to fire this bug again was start removing and adding things like no tomorrow. I didn't get any useful conclusion, the error seemed related to things which made no sense at all for me. Also, it would not happen if I called chai.use(chai-spies) after chai.use(chaiAsPromised), which kind of confirmed the origin of the bug detected at step 2.

What was my next brilliant idea? Start removing code like crazy from the chai-spies code. I downloaded it and removed everything but the setup for the called chainable method. The error was still there, so I kept erasing things (chai-spies code is even simpler than the one in chai-as-promised) until the bug started to disappear sometimes (sometimes being replaced by other errors). Then, I finally found out the offending line, the one that could switch on and off the bug if removed.

Since I had removed a lot of things, my "bug finding test" didn't consist on checking if called() would still work. It checked that I could execute expect(fn).to.be.spy; without triggering an error (I had previously identified this error as the same I was looking for). This Chai property (spy) checks if fn had a JavaScript property set. I found out that if I removed this check (the property is __spy), the error would not trigger anymore. So that is.

The error was that chai-as-promised would corrupt the JavaScript properties of objects that were passed to chainable methods. With this knowledge, I came up with a test that failed for the current master branch of the repository, but that was fixed by the PR.

The only thing left to do was fork the repository, add the test, modify the test script (because in the original one the call to chai.use(chaiAsPromised) was done before the execution of the tests, logically, so I needed a new environment for this one, and I had Domenic's permission), include the PR (so the other guy would take credit for his awesome fix) and submit it as a new pull request to the repository.

Conclusion

I have a special skill to make posts long and boring, so I will not extend anymore on this, but to remark the importance of having a test for a bug, even one caused by an API misuse. If further changes take place in the repository, we will be certain that this exact bug will not be introduced again without us noticing (if the PR is accepted).