ℹ️ 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 == -2
Huh?! -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.
sth 32 minutes ago
basically anything but edge layer tests are bad? i dunno about that
sequoia 31 minutes ago
change my mind ☕
sequoia 31 minutes ago
😛
alberto:octocat: 30 minutes ago
@Sequoia are you going to give a talk at assert.js?
alberto:octocat: 30 minutes ago
🙂
sth 30 minutes ago
so take this to it’s extreme, you’d never have a unit test again
sth 30 minutes ago
for a webapp you’d only ever tests controllers
Matt Mackay 29 minutes ago
despite the fact that it’s not relied upon anywhere.
It is relied upon, your build dependency graph would / could / should show that
sth 28 minutes ago
if you were publishing a module, you could export it from the file for testing, but not export it from the index of your module
👍
1
sequoia 28 minutes ago
@matt Mackay once I refactor double to no longer call shiftLeft, it is not relied upon anywhere as double was the only consumer.
sequoia 27 minutes ago
@sth that is a good point
sth 27 minutes ago
i like unit tests a lot, i can’t imagine only testing the outer layer of a module, in spite of that being the only thing you need to test, i’d rather split it up
sequoia 26 minutes ago
so take this to it’s extreme, you’d never have a unit test again
I’m not sure I follow you here; if you have a utility method that’s used throughout your application, it is being “exported” to those (internal) consumers.
sth 25 minutes ago
let’s say your large software project is a node_module
sth 25 minutes ago
would you have no unit tests?
sth 25 minutes ago
in spite of having hundreds of unexported functions?
sequoia 25 minutes ago
Not sure I follow
sequoia 25 minutes ago
what gets exported?
sth 24 minutes ago
let’s say it’s a cli that exports one function, and does some complex calculations on the data
sth 24 minutes ago
doesn’t really matter
sequoia 24 minutes ago
so this is good feedback that’s making clear that I attempted to make my point far too sloppily (thank’s for the feedback and good points!!). I should clarify that I am referring primarily to functions that have exactly one consumer, or are consumed exclusively within a single module/
sth 23 minutes ago
i think i would still disagree
sequoia 23 minutes ago
If these complex calculations naturally lived in one file of a reasonable length (<100 SLOC), I think it would be reasonable to only test the top level export, yes
sth 23 minutes ago
so it has nothing to do with whether the function is exported
sth 22 minutes ago
but the complexity of the whole module
sequoia 22 minutes ago
if it’s naturally broken into modules, each of which exposes its own API (even for internal consumption), those exposed APIs should be tested
sth 22 minutes ago
that i can agree with more
sth 22 minutes ago
ok but in js evertthing is amodule
sth 22 minutes ago
every file is a module, every folder is a module
sth 22 minutes ago
so where you draw the line is interesting to me
sequoia 21 minutes ago
yes, anywhere where you export. But if you wrote a function inside a single file and it’s only consumed within that file to support exported functions, testing it would be unnecessary.
sequoia 21 minutes ago
I’ll go further and say testing it would be bad 😄
sth 20 minutes ago
ok so it’s files then?
sth 20 minutes ago
what about a node_module with 100 files but only one exported function
sequoia 20 minutes ago
well each of those files must export something, no?
sth 20 minutes ago
just like the functions are internal to the file
the files are internal to the module
sth 19 minutes ago
why is internal to a file different than internal to a module?
sth 19 minutes ago
(imo there is no difference)
sth 19 minutes ago
the use of the word export doesn’t really mean anything, imo (edited)
sequoia 19 minutes ago
(I’m enjoying this discussion because these questions are challenging & I’m having to think very on the fly 😄)
sequoia 18 minutes ago
the use of the word export doesn’t really mean anything, imo
Strongly disagree here
sequoia 18 minutes ago
export definitely means something.
sth 18 minutes ago
do you generally support people importing from
/lib/internal_nondocumented_file.js (edited)
sth 17 minutes ago
if it’s not documented it’s internal no? not meant for consumption outside of the module
sequoia 17 minutes ago
directly, from outside of the module? No.
sth 17 minutes ago
so no tests required
sth 17 minutes ago
by your logic
sth 16 minutes ago
otuside the file/outside the module to me are equivalent, even tho the mechanics are slightly different (you technically cannot import from the file)
sequoia 16 minutes ago
I don’t 100% agree but I see your point and it is a good one. It sounds like you’re saying my criteria are vague to the point of being meaningless.
sth 16 minutes ago
essentially
sth 16 minutes ago
kinda the nature of node that everything can be considered a module
sequoia 15 minutes ago
Let me turn this around on you for a second:
Are you saying that if I write a reverse function that is called exclusively from inside my one file to reverse strings, I should expose that function and test it? (and presumably mock the call on each of the consumer’s tests?)
sth 14 minutes ago
in your logic a mere splitting of a file into multiple files for organization purposes will cause you to change your testing strategy
sequoia 14 minutes ago
and then if later you see my code and say “this is stupid, just inline this as myString.split('')reverse().join()“, then you should have to go back & remove the reverse test and update all the other tests to remove the mock?
sequoia 13 minutes ago
what is gained by all this rigamarole?
sth 13 minutes ago
it’s consistent, and scales (edited)
sth 13 minutes ago
easier to understand the code
sequoia 13 minutes ago
pffftt 😂 idk about all that
sequoia 13 minutes ago
How does this “scale” better?
sth 12 minutes ago
because you may compose your application with more and more complex functions
sth 12 minutes ago
splitting into smaller functions (imo) is generally a good idea, less cogniitive overhead, the same applies to tests imo
sth 11 minutes ago
if you can rely on the fact that all your subfunctions are tested, you simplify your test to only be concerned withwhat you’re actually doing in that function
sth 11 minutes ago
(integrating smaller functions)
sth 10 minutes ago
scales to modules, i don’t need to worry that arrayify works in my module, since that module is already tested externally
sequoia 10 minutes ago
but what happened to simplicity? Your approach produces probably 2x the LOC as mine (2x the test maintenance) and it’s not clear what’s gained by it. If the functions that call reverse all produce the same output before an after, what did you gain by writing and rewriting all these tests? none of this comes for free.
sth 10 minutes ago
let’s say your app uses every node_module in existence
sequoia 10 minutes ago
this is already true
sth 10 minutes ago
and by you logic you must test the behaviour of all those modules along with your code
sth 9 minutes ago
which of course would take you years (edited)
sequoia 9 minutes ago
:thinking_face: Let me see if I understand your point
sequoia 9 minutes ago
You’re saying that I’m saying “never mock at all”?
sequoia 9 minutes ago
I can see how that would be bad.
sth 9 minutes ago
this is what your argument suggests
sth 8 minutes ago
if you never test your internal functions
sth 8 minutes ago
how can you mock them
sequoia 8 minutes ago
hm… I don’t fully agree that it leads there (perhaps ad absurdum) but I see your point.
sth 8 minutes ago
i think my actual point of view would be dev discretion
sequoia 8 minutes ago
you’re not answering my question tho: what was gained by testing reverse, mocking, and rewriting all the tests after?
sth 7 minutes ago
when i work on the test for the top level i don’t have to understand reversal
sth 7 minutes ago
the example is too simplified to illustrate the benefits tho
sth 7 minutes ago
i think what you test really needs to be at your discretion
sth 6 minutes ago
along with whether or not you need to split code into smaller pieces at all
sth 6 minutes ago
but i wouldn’t use any blanket statements like, functions always used in one file shouldn’t be tested
👍
1
sth 6 minutes ago
i think it’s based on complexity not location and organization of code (edited)
👍
1
sequoia 5 minutes ago
I can agree with both of those points fully!