WIP: feat: add blog post about fixing code coverage gaps

cspell.json

@@ -106,6 +106,7 @@
 		"destructures",
 		"devreach",
 		"Dieiev",
+		"Dimitri",
 		"divs",
 		"dogfoding",
 		"dogfood",
@@ -119,6 +120,7 @@
 		"Durrant",
 		"dustinspecker",
 		"Dweck",
+		"Edvard",
 		"Eghbal",
 		"EightBittr",
 		"Elian",
@@ -250,6 +252,7 @@
 		"mindshare",
 		"minifiers",
 		"misbalanced",
+		"Mitropoulos",
 		"mochajs",
 		"Mugatu",
 		"Musk",
@@ -381,6 +384,7 @@
 		"tsconfigs",
 		"tsdevtest",
 		"tseslint",
+		"TSESTree",
 		"tslinter",
 		"Turbopack",
 		"twoslash",
@@ -392,6 +396,7 @@
 		"understocking",
 		"untriaged",
 		"unvetted",
+		"uppercases",
 		"upvote",
 		"upvoted",
 		"Vanderkam",

src/components/blog/mdx/LabeledMedia.astro

@@ -49,6 +49,7 @@ const descriptions =
 	}
 
 	.labeled-media-note {
+		display: block;
 		font-size: 0.9em;
 		padding-bottom: 1rem;
 		width: 100%;

src/content/blog/so-youve-got-a-gap-in-code-coverage/index.mdx

@@ -0,0 +1,260 @@
+---
+description: "This is my standard explainer for pull request reviews where changed lines aren't covered by unit tests."
+image:
+  alt: TODO
+  src: "~/assets/blog/the-scream.webp"
+pubDate: 2024-11-25
+tags: [
+	"code coverage",
+	"types",
+	"unit testing",
+]
+title: "So You've Got A Gap In Code Coverage"
+---
+
+import theScreamHappy from "~/assets/blog/the-scream-happy.webp";
+import theScreamWide from "~/assets/blog/the-scream-wide.webp";
+import LabeledImage from "~/components/blog/mdx/LabeledImage.astro";
+
+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-is-reachable-so-a-unit-test-should-exercise-it)
+- [The code can be refactored to not include that case](#the-code-can-be-refactored-to-not-include-that-case)
+- [This is a difficult-to-represent edge case in types](#this-is-a-difficult-to-represent-edge-case-in-types)
+
+The next three sections of this blog post explain 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.
+
+<LabeledImage
+	alt="Edvard Munch's The Scream, cropped to just show the screaming person"
+	description="Many developers' reaction to being asked to write more tests. It's not so bad, I promise!"
+	original="https://en.wikipedia.org/wiki/File:Edvard_Munch,_1893,_The_Scream,_oil,_tempera_and_pastel_on_cardboard,_91_x_73_cm,_National_Gallery_of_Norway.jpg"
+	src={theScreamWide}
+/>
+
+## 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.
+
+It looks like this area of code is something that can be hit by users, but doesn't have a unit test covering it.
+When a situation can legitimately be hit by users, we want to have a unit test for that situation.
+
+> 👉 See [Handling Edge Cases](#handling-edge-cases) below for an example.
+
+## 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.
+
+> 👉 See [Duplicate Work](#duplicate-work) below for an example.
+
+## This is a difficult-to-represent edge case in types
+
+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.
+
+> 👉 See [Map Gets](#map-gets) below for an example.
+
+## Examples
+
+These are some contrived examples showcasing the three strategies in order.
+I separated them out because I know a lot of people don't have the time to read them in detail.
+
+### Handling Edge Cases
+
+> 👉 Strategy: [The code is reachable, so a unit test should exercise it](#the-code-is-reachable-so-a-unit-test-should-exercise-it
+
+Consider this `capitalizeBeforeExclamation` function that processes a `text: string`.
+If `text` includes a `!`, all characters before it are upper-cased:
+
+```ts
+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:
+
+```ts
+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:
+
+```ts
+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.
+
+### Duplicate Work
+
+> 👉 Strategy: [The code can be refactored to not include that case](#the-code-can-be-refactored-to-not-include-that-case)
+
+Take the following `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:
+
+```ts
+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:
+
+```ts
+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:
+
+```ts
+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!
+
+### Map Gets
+
+> 👉 Strategy: [This is a difficult-to-represent edge case in types](#this-is-a-difficult-to-represent-edge-case-in-types)
+
+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`:
+
+```ts
+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;
+	});
+}
+```
+
+```ts
+// 🛑 See later - there's a better way!
+const count = (counts.get(value) ?? 0) + 1;
+```
+
+```ts
+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](https://learningtypescript.com/articles/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! 💙
+
+<LabeledImage
+	alt="Altered version of Edvard Munch's The Scream, cropped to just show the person, who is now smiling happily"
+	description="The feeling we all get from successfully writing comprehensive tests! Yippee!"
+	note="Altered version of Edvard Munch's The Scream courtesy of Michigan TypeScript's Dimitri Mitropoulos."
+	src={theScreamHappy}
+/>