Errors, Assertions, and Test Coverage

I wrote about the Unexpected Lessons from 100% Test Coverage, inspired by people like Isaac Schlueter, who challenge the conventional wisdom that 100% coverage is bad. Part of this post involved reflections on thinking about delineating assertions and errors that I thought warranted being split up into its own post.

Lines that check for error and conditions and make assertions are notoriously some of the hardest to cover during testing. Attempting to test these can be very helpful in understanding our code.

schema-dts transforms RDF triples into an intermediate representation of "classes", and eventually TypeScript typings. The codebase for these transformations included checking for a number of boundary conditions, and a number of other assertions. When looking at my test coverage, I saw that most of those cases were not being tested.

In an effort to testing them, I looked at some use cases and saw some common themes.

A: Intra-function Impossibility

There was a line in my code that my tests never covered. It looked something like this:

// Top-level module:
const wellKnownTypes = [ ... ];  // Top-level var with more than one "Type".
const dataType = ...;

function ForwardDeclareClasses(topics: ReadonlyArray<TypedTopic>): ClassMap {
  const classes = new Map<string, Class>();
  for (const wk of wellKnownTypes) {
    classes.set(wk.subject.toString(), wk);
  }
  classes.set(dataType.subject.toString(), wk);
  for (const topic of topics) {
    // ...
  }

  if (classes.size === 0) {
    throw new Error('Expected Class topics to exist.');
  }

  return classes;
}

As you can see just reading this function and knowing: (classes.size === 0) will never happen. For one, there’s a classes.set(K, V) a few sections above. And we set a few other key-value pairs from this wellKnownTypes array that is hard-coded to always have a set number of elements.

One can try to understand the point of this error: It could be to show that none of the RDF triples we got are turned into classes (in which case we might want to compare classes.size with wellKnownTypes.length + 1 instead). Alternatively, it can be a poorly-placed verification for when we had less confidence that we were building classes properly, and had no clear concept of such “well known” types.

In my case, creating a map with just the well knows seemed fine. If the ontology is empty or missing data, we’ll likely find it at earlier steps or later ones. And the error gives no clear context as to what’s going wrong. So in my case, the answer was to kill it:

-  if (classes.size === 0) {
-    throw new Error('Expected Class topics to exist.');
-  }

B: Inter-Function Assertion

Another error I saw looked like this:

function ForwardDeclareClasses(topics: ReadonlyArray<TypedTopic>): ClassMap {
  // ...
  // ...
  for (const topic of topics) {
    if (!IsClass(topic)) continue;
    // ...
    classes.set(
        topic.Subject.toString(), new Class(topic.Subject, /* ... */));
  }
  // ...
  return classes;
}
function BuildClasses(topics: ReadonlyArray<TypedTopic>, classes: ClassMap) {
  for (const topic of topics) {
    if (!IsClass(topic)) continue;
    const cls = classes.get(topic.Subject.toString());
    if (!cls) {
      // Line (A) is next ---------------
      throw new Error(/**... class should have been forward declared */);
    }
    toClass(cls, topic, classes);
  }

In this case, line A never happened (and the (!cls) condition was always false). This should make sense: ForwardDeclareClasses literally checks if a TypedTopic satisfies IsClass(), and, if so, unconditionally adds it to the map. BuildClasses assert that a topic matching IsClass exists in the map.

One way to get test coverage for this line is to export BuildClasses and test it. But that seems like it goes against the spirit of making the codebase better. A better approach is ask what this line is trying to do:

Interlude: Expectations, Errors, and Assertions

Sometimes, we assert things because they either…

1. are error conditions that can happen in the wild due to poor data or input,
2. shouldn’t happen, and if they did it’s a sign of a bug in our code, or
3. shouldn’t happen, and if they did it’s a sign of cosmic radiation.

I decided to differentiate these. If my test coverage report complains about an uncovered...

1. error condition, I should test it. If I can’t, I should refactor my code to make it testable;
2. assertion that might indicate a bug, some code golf to make these always run might be in order (more on that in a bit);
3. assertion that is totally impossible, maybe I should delete it.

I refer to #1 as an error condition. Test these. For assertions, I often found that the line between #2 and #3 is often the function boundary (this isn’t always true). Cases of intra-function assertions (like case study A above) seem so useless that we’re better off removing them. Cases of inter-function assertions (like this case) seem useful enough to stay.

The Fix

I found that this distinction is not just helpful to split hairs: it’s also very helpful for someone reading the code: Is this error something that can happen in normal operation? or, is it a sign of a bug? I decided to make this clear:

1. Normal error conditions: if + throw, or similar.
2. Bug assertions: assert and assertXyz variants.

With that, I ended up with this change:

+import {ok} from 'assert';
+
 // ... more imports ...
+const assert: <T>(item: T|undefined) => asserts
+

 function BuildClasses(topics: ReadonlyArray<TypedTopic>, classes: ClassMap) {
   for (const topic of topics) {
     if (!IsClass(topic)) continue;

     const cls = classes.get(topic.Subject.toString());
+    assert(cls);
-    if (!cls) {
-      throw new Error(/**... class should have been forward declared */);
-    }
     toClass(cls, topic, classes);
   }
 }

Here, thinking about covering a line of code fundamentally helped me communicate what my code does more effectively. A lot of the “moving things around” that I had to do is very much semantic code golf (that happily happens to give a better test coverage score), but I’d like to think it’s net positive.

Read more about this experience here

Cover image is in in the public domain, licensed under CC0.