Hi! You might have been linked this blog post in a PR that has missing code coverage. There might be a request for changes linking to this blog post, asking that you resolve the missing code coverage gaps.
For each gap in coverage, at least one of the following is probably true:
- The code is reachable, so a unit test should exercise it
- The code can be refactored to not include that case
- This is a difficult-to-represent edge case in types and a type assertion is warranted
The next three sections of this blog post explains those three classifications in more detail and with examples. Whichever strategy you pick, go ahead and re-request review once you’ve got no new gaps code coverage — or would like help getting there.
The code is reachable, so a unit test should exercise it
We maintain a very high test coverage level because our project is the type that can be tested in a straightforward manner. If a situation can legitimately be hit by users, we want to have a unit test for that situation.
Example: Handling Edge Cases
Consider this capitalizeBeforeExclamation function that processes a text: string.
If text includes a !, all characters before it are upper-cased:
function capitalizeBeforeExclamation(text: string) {
const index = text.indexOf("!");
if (index === -1) {
return text;
}
return text.substring(0, index).toUpperCase() + text.substring(index);
}
An incomplete test suite for capitalizeBeforeExclamation might only test the case of a ! being found:
describe("capitalizeBeforeExclamation", () => {
it("capitalizes only text before the !", () => {
expect(capitalizeBeforeExclamation("abc! def")).toBe("ABC! def");
});
});
But, if it’s legitimately possible for text to be passed in that doesn’t have a !, then we should test that!
A more comprehensive test suite might look like:
describe("capitalizeBeforeExclamation", () => {
it("does not change text when no ! is present", () => {
expect(capitalizeBeforeExclamation("abc def")).toBe("abc def");
});
it("capitalizes only text before the ! when text exists", () => {
expect(capitalizeBeforeExclamation("abc! def")).toBe("ABC! def");
});
});
💡 Notice how the test titles became more clear, too. I find that unit tests titles that indicate the expected behavior upon a particular input (“it X when Y”) make tests easier to understand.
The code can be refactored to not include that case
Another common strategy is to simplify the code to not include as many cases for testing.
Developers often accidentally duplicate logical checks across areas of code. A check inside one function might no longer be necessary once a calling function is changed to also include that check.
Example: Duplicate Work
Take the following contrived createLabelIfSuffixed function that, depending on whether a suffix exists for some name, returns either undefined or an object with a suffix and text properties.
It has its own suffixes.get(name) lookup — as does a getLabelText helper function:
export function createLabelIfSuffixed(
name: string,
suffixes: Map<string, string>,
) {
const suffix = suffixes.get(name);
if (!suffix) {
return undefined;
}
return {
suffix,
text: getLabelText(name, suffixes),
};
}
function getLabelText(name: string, suffixes: Map<string, string>) {
const suffix = suffixes.get(name);
if (!suffix) {
return name;
}
return `${name} (${suffix})`;
}
The if (!suffix) check inside getLabelText is never going to be hit.
getLabelText is only called if there is a suffix for the name.
Code coverage checkers would therefore complain that that if statement’s body is not hit.
You might notice that the suffix variable inside getLabelText is always truthy, so a type annotation could tell TypeScript you don’t need to check if it exists:
function getLabelText(name: string, suffixes: Map<string, string>) {
// 🛑 See later - there's a better way!
const suffix = suffixes.get(name)!;
return `${name} (${suffix})`;
}
Type annotations are not a good first strategy for working with code refactors. They’re really meant as a last ditch effort when other strategies aren’t workable.
The root issue here is that the work of suffixes.get(name) is being done twice: first in createLabelForSuffix and again in getLabelText.
That work causes a split in logic each time it’s done.
Code has to handle the truthy case (a suffix existing) and a falsy case (a suffix not existing).
Instead of handling the same logic split twice, a cleaner refactor would be to pass the result of the first suffixes.get(name) into getLabelText.
That way, the code doesn’t need to handle the logic split a second time:
export function createLabelForSuffix(
name: string,
suffixes: Map<string, string>,
) {
const suffix = suffixes.get(name);
if (!suffix) {
return undefined;
}
return {
suffix,
text: getLabelText(name, suffix),
};
}
function getLabelText(name: string, suffix: string) {
return `${name} (${suffix})`;
}
By deduplicating work, we’ve both simplified our code and closed the gap in code coverage. Hooray!
This is a difficult-to-represent edge case in types and a type assertion is warranted
The rarest of these three cases is type checking pressuring you to handle a case that you know with certainty will not happen. This really is rare: most of the time, either that edge case really should be handled or code can be refactored nicely.
However, you may occasionally find an edge case in types where refactoring wouldn’t make sense and it’s annoyingly difficult or even impossible to get TypeScript’s types to understand your code. It might be reasonable to use a type assertion to tell TypeScript something it can’t figure out for itself.
Example: Map Gets
The built-in Map class is one place where TypeScript can sometimes be overly restrictive about types.
Map<K, V>.get<K> is typed as returning V | undefined because there’s no guarantee any arbitrary k (K) exists on a Map.
But, if you are absolutely 100% sure your .get will always a retrieve a value, then that’s inconvenient.
As an example, the counts Map<T, number> in the following createCounters<T> is used to store a count for each value.
It’s initialized with 0 for each value and is never used other than to retrieve one of those values.
But TypeScript doesn’t know that, so it thinks the count.get(value) returns number | undefined, not number:
function createCounters<T>(values: T[]) {
const counts = new Map(values.map((value) => [value, 0]));
return values.map((value) => () => {
const count = counts.get(value) + 1;
// ~~~~~~~~~~~~~~~~~
// Object is possibly undefined.
counts.set(value, count);
return count;
});
}
// 🛑 See later - there's a better way!
const count = (counts.get(value) ?? 0) + 1;
const count = counts.get(value)! + 1;
🚫 Type assertions are almost always a last ditch resort. They’re still better than using
any, or even more unsafe, TypeScript comment directives.
Don’t be afraid to ask for help
It’s ok if you’re having a hard time figuring out the right way to test the code! I want open source software to be a warm, welcoming place. Nobody should feel intimidated asking me for help with code.
Go ahead and post a comment explaining what you’ve tried, what did or didn’t work, and any specific questions you have. I’ll be happy to give you pointers and work with you to get test coverage up.
Thank you for sending a pull request in, let alone working with me on getting it approved! 💙
Liked this post? Thanks! Let the world know:
