ℹ️ Summary: Test the destination, not the route.
We have a module that doubles numbers. It does this by bit-shifting the supplied number once left.
// double.js
// exported to allow testing...
export const shiftLeft = (num, places) => num << places;
export const double = x => shiftLeft(x, 1);We test it and it works!
import {double, shiftLeft} from 'double.js';
// test double
assert.equal(20, double(10));
assert.equal(30, double(15));
// test shiftLeft
assert.equal(2, shiftLeft(1,1));
assert.equal(4, shiftLeft(1,2));An vexatious colleage sees that our double function uses bit shifting and decides to have a bit of fun with us. She adds the following test:
assert.equal(18014398509481982, double(Number.MAX_SAFE_INTEGER)); // trollface.webp
// 1 TEST FAILED ========================
// AssertionError [ERR_ASSERTION]: 18014398509481982 == -2Huh?! -2? It looks like she's revealed an issue with our use of bit shifting. No matter! We can use a multiplying function instead:
// double.js
// exported to allow testing...
export const shiftLeft = (num, places) => num << places;
const mul = (multiplicand, multiplier) => multiplicand * multiplier;
export const double = x => mul(x, 2);Our tests now pass! We clean up our file by removing unused functions...
// double.js
-// exported to allow testing...
-export const shiftLeft = (num, places) => num << places;
const mul = (multiplicand, multiplier) => multiplicand * multiplier;
export const double = x => mul(x, 2);But now our tests fail! Removing shiftLeft causes the tests to fail, despite the fact that it's not relied upon anywhere.
Our tests are failing, so one of two things is true. Either:
- Our module really is broken (the tests work) or
- Our module is not broken, but the tests are still failing
Number 2 indicates that we are testing something unrelated to whether or not the module works.
So what do we do now? We update the tests, even though our code is working and nothing is broken. This is a waste of time, which is added to the time we wasted writing the test in the first place.
Test that when you call double(2) it returns 4, don't test what method it used to arrive at that solution, because for the purposes of testing the double function, it doesn't matter. If the module is supposed to get from point a to point b, and initially it does it by going "right, forward, left" then we change it to go "left, forward, right" but it still ends up at point b, the tests should not fail. We are concerned with the destination, not how we got there.
Counter argument, if you have unit tests for
shiftLeftanddoubleis doing nothing but callingshiftLeftwith a fixed parameter then I don't think there's a need to testdoubleat all as long as it's not a library with a sole purpose of providing double functionality, in that case I'd prefer tests for onlydoublei. e. the function I expose, but that's just my opinion for this very case. If it is a module that has 100s of private functions and only one function exposed, I would prefer to test each function individually so that it would be easy for me to write, maintain and pinpoint issues (if any). By your approach (correct me if I'm wrong), you'd only write tests for exposed function and leave rest without any tests? That doesn't seem like a viable approach to me.Secondly, if you have a function that you want to remove, it is not removed until you remove its unit tests as well. In your scenario, if you have tests for both functions and decide to change the internal implementation of
doublemakingshiftLeftuseless, then you remove it but leave its unit tests then I don't think it is removed completely (IMO, function and its test should be removed together in one commit).It is a relatively subjective topic to discuss and for a language like javascript (that doesn't come with any opinion of its own) there's lots of grey area and no definitive correct approach.