Colliding Objects

Coding, design, and broken feedback loops

In a recent spec I was writing, one of the tests was about sanity-checking the content of several files I am using in production. The jasmine-node test loads these files via a series of async fs.readFile calls. It then inspects the content of each file by iterating over the bodyById object, making sure a minimal-length criterion is met.

My first implementation for the last part looked as follows (see model.spec.js):

Object.keys(bodyById).forEach(function(k) {
  var body = bodyById[k];
  expect(body).toBeTruthy();
  expect(body.length).toBeGreaterThan(minLength);
});
done();

Unfortunately, this test did not perform as expected. In situations where body was falsey jasmine-node did not print the proverbial T tests, A assertions, F failure, S skipped line and its exit code was 0 which indicates that from jasmine-node's standpoint all tests have passed.

After several iterations of trial-and-error I arrived at this: I wrapped the Object.keys() call with a try block and used an empty catch block catch(e) {}:

try {
  Object.keys(bodyById).forEach(function(k) {
    var body = bodyById[k];
    expect(body).toBeTruthy();
    expect(body.length).toBeGreaterThan(minLength);
  });
} catch(e) {}
done();

To my surprise this version worked! this is weird as this code silently ignores thrown exceptions, which intuitively seems like a step in the wrong direction (it reduces the test's sensitivity even further, where I wanted to make an under-sensitive test more sensitive). Still, I could not argue with the facts: with this version, min-length violations were caught are properly reported. I committed the code.

A few days later I got back to it. I wasn't satisfied with this "solution". Sure, it worked, but I didn't understand why, and I was sure there is a better solution.

When we are not satisfied with production code we refactor it: first, we check our baseline by running the tests and seeing they ate Green, we then apply a transformation, run the tests (again) to verify the overall behavior was preserved, and repeat until the code reaches a desired state.

This procedure is not safe for refactoring of tests: when transforming tests one may unintentionally mute the assertions there. Once such an accident has occurred running the tests again is pointless. Even if you get Green you cannot trust it. You are essentially locked in a chamber which keeps echoing "Green" no matter what you do. Here is one quick example for such a Transformation of Terror: Imagine a test which verifies that a certain div is present, and has a non-empty content:

expect(browser.text('.main-area')).toBeTruthy();

If a refactoring haphazardly drops the .main-area class selector:

expect(browser.text('')).toBeTruthy();

we end up with an assertion that practically always succeeds (as long as the browser object has loaded a page with at least one character the assertion passes) and, thus, tests nothing.

The solution to this predicament is simple:

Refactor Tests Only On Red

Coming to think about, it makes perfect sense: a test's "money time" is that point where it produces Red on broken production code. The fact that it produces Green on correct production code does not imply the opposite. For one, every test with no assertions will produce Green on correct code but will not produce Red on broken code.

Thus, if we want to refactor a test we must first put it in that spot where it is Red, by introducing an intentional bug into our production code. The test retaining its Redness throughout the refactoring steps keeps us confident that these steps did not cripple the test. Only after refactoring is over can we un-introduce the bug and get back to Green. To make things concrete here is the series of steps that I used when I refactored my jasmine-node spec, the goal of which was to get rid of that irritating catch(e) {} block.

Introduce an intentional break

I stated by inducing a failure. In this case, this was achieved by changing model.js. I added the following entry to the posts list:

  {
    id: "1",
    title: 'some title',
    body: '',
    publishedAt: '2014-01-29T11:41:00+02:00',
  }

As this entry has an empty body field, it will break the expect(body.length).toBeGreaterThan(minLength); assertion in the defines a body for every post spec. Indeed, when we run the specs we get a failure, and the return value ($?) is non-zero:

$ npm test

> application-name@0.0.1 test ....
....

.Express server started at http://localhost:3001
127.0.0.1 - - [Wed, 29 Jan 2014 18:30:51 GMT] "GET /posts/4 HTTP/1.1" 200 1256 "-" "Mozilla/5.0 Chrome/10.0.613.0 Safari/534.15 Zombie.js/2.0.0-alpha24"

Finished in 2.384 seconds
13 tests, 24 assertions, 1 failure, 0 skipped


npm ERR! weird error 1
npm ERR! not ok code 0
$ echo $?
1

This is exactly how we expect the test to behave.

First refactoring step

I change the block to what I initially wanted to write:

Object.keys(bodyById).forEach(function(k) {
  var body = bodyById[k];
  expect(body).toBeTruthy();
  expect(body.length).toBeGreaterThan(minLength);
});
done();

As expected, going back to this code brought back the original problem: now the tests are not failing:

$ npm test

> application-name@0.0.1 test ....
....

.Express server started at http://localhost:3001
127.0.0.1 - - [Wed, 29 Jan 2014 18:48:43 GMT] "GET /posts/4 HTTP/1.1" 200 1256 "-" "Mozilla/5.0 Chrome/10.0.613.0 Safari/534.15 Zombie.js/2.0.0-alpha24"
$ echo $?
0

So, jasmine-node is saying "Green". but as we're refactoring tests and we're under an intentionally induced breakage, the Red/Green semantics is reversed: Red means 'Good'. Green means 'something is not working properly'. Bottom line: this first step alone is not behavior-preserving. A second step is needed.

Second refactoring step

So, my first naive refactoring step was not successful (as it led me to Green). At this point I consulted jasmine-node manual and found the --captureExceptions flag:

--captureExceptions: listen to global exceptions, report them and exit (interferes with Domains in NodeJs, so do not use if using Domains as well.

This sounded about right. I added this flag to my jasmine-node configuration by editing the scripts.test value in package.json:

{
  "name": "application-name",
  "version": "0.0.1",
  "private": true,
  "scripts": {
    "start": "node site.js",
    "test": "jasmine-node --captureExceptions spec"
  },
  "dependencies": {
    "escape-html": "1.0.1",
    ....
  }
}

and I ran the tests again:

$ npm test

> application-name@0.0.1 test ....
....
.Express server started at http://localhost:3001
127.0.0.1 - - [Wed, 29 Jan 2014 12:24:03 GMT] "GET /posts/4 HTTP/1.1" 200 1256 "-" "Mozilla/5.0 Chrome/10.0.613.0 Safari/534.15 Zombie.js/2.0.0-alpha24"
...TypeError: Cannot read property 'length' of undefined
    at /home/imaman/workspace/green-site/spec/model.spec.js:34:20
    at Array.forEach (native)
    at oneDown (/home/imaman/workspace/green-site/spec/model.spec.js:31:29)
    at /home/imaman/workspace/green-site/spec/model.spec.js:47:9
    at /home/imaman/workspace/green-site/model.js:22:9
    at fs.js:266:14
    at Object.oncomplete (fs.js:107:15)
npm ERR! weird error 1
npm ERR! not ok code 0
$ echo $?
1

Woohoo! This step got us back on Red. This is indication that the combined effect of both refactoring steps is behavior-preserving.

At this point, the piece of code that I did not like has been changed to my taste, and I had the "proof" that I did not cripple anything. Time to wrap up.

Undo the breakage

This stage is trivial but nonetheless important. As we just finished refactoring the tests, we need to get out of the Red zone by reverting the breakage.

Finally, I verified Greenness by running the tests one last time:

$ npm test
> application-name@0.0.1 test ....
....
127.0.0.1 - - [Fri, 31 Jan 2014 22:44:29 GMT] "GET /posts/4 HTTP/1.1" 200 1256 "-" "Mozilla/5.0 Chrome/10.0.613.0 Safari/534.15 Zombie.js/2.0.0-alpha24"

Finished in 2.475 seconds
13 tests, 27 assertions, 0 failures, 0 skipped

$ echo $?
0

That's it. Everything is back to normal.