testdouble.js
testdouble.js copied to clipboard
testdouble
Enforce calling with new for `td.constructor`, disallow for `td.function`
In my opinion, the following should be an error:
const Constructor = td.constructor(function () {});
Constructor(); // should throw, complaining it is not called with new
This would be a breaking change, necessitating changes to how people verify:
// need to use new in verify methods as well
td.verify(new Constructor(...matchers));
Conversely, the following should also be an error:
const notConstructor = td.function();
new notConstructor(); // should throw, complaining it MUST be called with new
I agree with this on an ES class / non-class divide, but so does the spec, which will raise a runtime exception when not called with new. Therefore, in the first case I can get behind this if the thing td.constructor is imitating was a class, but so often we can't tell because the vast majority of people are transpiling. If someone is using a native ES class and passing it to td.replace is it possible to check if it's a class?
Aside from class's, I don't think this should be an error because it's not in production code. People may write a constructor function that can be invoked either way (which is a common pattern that many people did and presumably still do), just as they might call a function they don't intend to serve as a constructor with new.
In short, I think the library's errors should map to the flexibility of the language. I wouldn't be opposed to printing a warning if we can detect someone's doing something fishy, but it seems somewhat low priority.
Closing, but may reopen with a really compelling example case or a more narrow proposal regarding the ES class specifically
But presumably you should know how your code is calling the mock (with new, or without), and if we are mocking a class, then we aren't testing that class, but the code that calls it, in which case we should know, and verify, if new was used.
At a minimum there should be an easier way to verify it was called with new, perhaps just:
td.verify(new foo(arg 1, arg 2));
But in that's the API you adopt, I would argue the following should fail:
new foo('bar');
td.verify(foo('bar'));
But that's going to be a breaking change as well.
I still think enforcing new is a good idea when the user explicitly declares it a constructor. Overloading is an anti-pattern, and I'd argue that the best answer from the article you linked is the "kill it with fire" option. You could certainly include an option to disable the new check, but it should be on by default. This is what I expect of "opinionated" libraries - deliver maximum value as tersely as possible, nudging me towards the right way of doing things.
It's hard to imagine a scenario where, even if the double was replacing a function outside my control, and that API had overloaded new, that I would be calling it both with new and without in the same test.
Like I said, I'm open to it, but I'm struggling to get excited about it, because I've literally not seen this happen in years, and almost anyone excitedly using new going forward is overwhelmingly likely to be using the class keyword, which enforces constructors be called with new, so wouldn't lead to a bug.
If there's a way to implement this that's minimally invasive and doesn't increase maintenance cost by introducing additional branching logic, I'm open to seeing it
almost anyone excitedly using new going forward is overwhelmingly likely to be using the class keyword, which enforces constructors be called with new, so wouldn't lead to a bug.
My point is that you are likely to end up in a situation where your unit tests are not failing, because testdouble does not complain about new, but it will complain in production or in integration tests because it receives an actual class. testdouble currently doesn't provide an easy way for me to mimic the "fail without new" behavior of a class.
Separating the relative prioritization of this versus other work, how would you suggest we detect use of new versus not in each of these cases?
var createTestDoubleFunction = () =>
function testDouble (...args) {
const calledWithNew = this instanceof testDouble
calls.log(testDouble, args, this, calledWithNew)
return stubbings.invoke(testDouble, args, this, calledWithNew)
}
