Tests, Traps and Smells
Three things reviewers skip: the tests themselves, the library defaults everyone assumes, and the code shapes where tomorrow's bug is growing.
The story
A payments team ships a change to refunds. Code review approved it. CI was green, including a test called rejects refunds larger than the payment.
A week later finance finds refunds larger than the original payments: customers were refunded ₹5,000 on ₹500 orders. The validation had been accidentally deleted in the change. So why did the test pass?
it('rejects refunds larger than the payment', () => {
expect(refunds.refund('pay_1', 99_999)).rejects.toThrow('too large')
})
There's no await. The test function returns before the assertion runs, so the test passes whatever refund does. It had passed, uselessly, since the day it was written. Nobody noticed, because a passing test is the one thing nobody reads.
Tests are code, and reviewers are expected to review them. A test that can't fail is worse than no test: it makes everyone confident in exactly the code that's least protected.
Part 1: Reading the tests
The test suite is also the fastest map of a codebase you don't know:
- What's tested shows what the author worried about. Many tests around retries? Retries have bitten them before.
- What's missing shows where to look. Money paths with only happy-path tests, no concurrent tests, no tests for the error branches: that's where the bugs are.
- How it's tested shows what's fake. If the database, the clock and the payment provider are all mocked, the tests can't catch anything that happens between them.
Then check each test can actually fail:
| Bug in the test | Signal |
|---|---|
| Un-awaited async assertion | expect(promise).rejects/resolves without await |
| No assertion | A test that only calls the code |
| Always-truthy assertion | expect(promiseOrObject).toBeTruthy() |
| Testing the mock | The function under test is itself mocked or spied with a return value |
| Weak call assertions | toHaveBeenCalled() without checking the arguments |
| Real time | Date.now(), new Date(), real setTimeout in tests: flaky or slow |
| Shared state | Module-level arrays, DB rows or singletons that outlive one test |
| Giant snapshots | Hundreds of lines nobody reads, approved on every change |
| Happy path only | No tests for failures, limits, duplicates or concurrency on risky code |
Drill 1: A test file
Drill 2: More tests
Part 2: Library and API traps
Some bugs come from a reasonable assumption about a library that happens to be false. They're worth memorising because they appear in almost every codebase, and because "I know that axios throws on 404" is a quick, credible finding in an interview.
| Trap | Reality |
|---|---|
fetch throws on HTTP errors | It doesn't. Check res.ok. |
| axios returns 404 responses | It throws on 4xx/5xx by default |
| HTTP clients have a timeout | axios's default is 0 (none). fetch has none. |
arr.map(parseInt) | parseInt receives the index as its radix: ['1','2','3'].map(parseInt) → [1, NaN, NaN] |
JSON.stringify handles everything | Map/Set become {}, undefined disappears, BigInt throws, Date becomes a string |
new Date('2024-02-30') fails | V8 silently rolls it over to March 1 |
| Moment dates are values | They're mutable: date.add() changes the original |
setTimeout takes any delay | Above about 24.8 days (2³¹ − 1 ms) it overflows and fires almost immediately |
encodeURI makes a safe query value | It leaves &, = and ? alone. Use encodeURIComponent or URLSearchParams. |
| bcrypt hashes the whole password | It uses only the first 72 bytes |
new URL('orders', base) appends | Without a trailing slash, the base's last segment is replaced |
| JSON numbers are exact | Integers above 2⁵³ lose precision |
Drill 3: Traps, part 1
Drill 4: Traps, part 2
Part 3: Smells that hide bugs
A smell isn't a bug. It's a shape of code that makes bugs likely and hard to see. In a review, report smells as risks, separately from bugs, and briefly: "not broken today, but this is where the next bug will come from".
| Smell | Why it breeds bugs |
|---|---|
| The same rule in two places | The copies drift, and the two answers disagree |
| Names that lie | Callers trust the name: a get that writes creates data by accident |
| Comments that disagree with code | One of them is wrong, and readers trust whichever they read |
| Boolean parameters | send(order, true, false) gets swapped and nobody notices |
| God functions | Every change risks every behaviour in a 300-line handler |
| Dead code kept "just in case" | Untested old rules that a refactor or a lookup table can quietly bring back |
Drill 5: Smells
Cheat sheet
| Category | First thing to check |
|---|---|
| Tests | Can each test fail? Look for missing await, missing assertions, mocks of the unit under test |
| Tests | What's not tested on money, auth and retry paths? |
| Tests | Real clocks, real timers, shared state, giant snapshots |
| Library traps | fetch vs axios errors, missing client timeouts |
| Library traps | map(parseInt), JSON.stringify of Map/Set/BigInt, mutable dates, new Date rollover |
| Library traps | setTimeout > 24.8 days, encodeURI, bcrypt's 72 bytes, URL base paths, IDs > 2⁵³ |
| Smells | Duplicated rules, lying names, stale comments, boolean params, dead code |
Say it in the interview
"The refund-limit test has no
awaiton itsrejectsassertion, so it finishes before the assertion runs: it passes whatever the code does. That matters because the limit check inrefundwas removed in this change, so the only test guarding it is a false negative. I'd fix the test first, watch it fail, then restore the check."
Go deeper
- JS · Reviewing a Codebase: testability and design critique.
- L1–L2 · Logic and TypeScript: the language-level traps.
- Next: Mock Reviews: put everything together.