add cloneArgs option to td.when/td.verify #417
Conversation
|
Hello, @searls! I know this is an old update, but we've just upgraded from 3.8.1 to the latest version and this is breaking for part of our testing suite. We're using testdouble in an Ember app, where sometimes the arguments to our stubs are Ember objects. These have a chain of references to the entire app registry and even the module registry, which cloneDeep then tries to clone and and some point encounters an undefined reference, throwing an error. Regardless of that specific error, cloning the entire context of the app is definitely bad. Is there any way to avoid calling We can definitely open a PR in this direction, we just need a bit of guidance on how would this be implemented. I'm guessing it's not trivial, considering that this concern is expressed in the PR description. |
|
Hey @monovertex sorry to hear that you're having this problem. It has literally been two years since I've even thought about this aspect of the library so I need some help understanding your need. Could you provide a few things?
I got confused because on its face: "Is there any way to avoid calling cloneDeep when the cloneArgs config option is not true" sure seems like that's how it should behave, so I'm afraid I don't understand |
| @@ -7,7 +7,7 @@ store.onReset(() => { callHistory = [] }) | |||
|
|
|||
| export default { | |||
| log (testDouble, args, context) { | |||
| store.for(testDouble).calls.push({ args, context }) | |||
| store.for(testDouble).calls.push({ args, context, cloneArgs: _.cloneDeep(args) }) | |||
There was a problem hiding this comment.
Thank you for your prompt reply @searls! The current implementation will always call cloneDeep, even when config.cloneArgs is false. True, it will only use the cloned args when that option is set, but the cloning of the arguments happens all the time. This is also highlighted by the original concern you expressed about this change having a performance impact for calling test doubles with large or hard to deep-clone arguments.
Unfortunately the current workaround was to downgrade back to 3.8.1, as there's no way to control the deep cloning.
I am not necessarily proposing an API change, but rather to see if the cloning could only be done when cloneArgs is true. From the code I referenced here, it seems that the config object is not available, so perhaps there's an architectural limitation?
Here is a very short reproduction that evidences how the cloning might impact a test. Obviously it's a ridiculous example, but in our particular case, there's a chain of references that leads to a similar prop definition, where cloneDeep then promptly throws an error.
Now, we would never be able to use the cloneArgs config option, that is true, but that is okay, we never needed it anyway, and we feel like the corect handling in those situation would be instead to use td.matchers.argThat and assert on particular properties we want to stay the same.
There was a problem hiding this comment.
Ahhhhhhhh, I get it.
Yeah this is an ancillary benefit of these changes that's not directly related to the functionality. The goal here that we were solving for was error messages and td.explain would lie to you if something passed to a test double was mutated during the test before the printout. Cloning the arguments is a way to ensure that the library's messages are accurate, which is indeed very important (as having test output that says stuff like "expected foo({age: 32}) but was actually foo({age: 32})" can do a lot of harm when debugging)
Yeah, I don't have a great solution here. If the issue is the error, then I think we can wrap cloneDeep in a try-catch and fall back on the original args if cloneArgs is some symbol that represents a failed processing.
There was a problem hiding this comment.
That doesn't sound like a bad idea actually. Although in our case we would probably prefer to maybe have a feature flag to turn off the deep cloning altogether, as it's something that has potential to slow down the app and maybe even cause huge memory leaks if a reference to the cloned object is retained somewhere, if the cloning is successful.
In our particular case, where we can't control the objects and their references (they're generated by Ember), we prefer the tradeoff of potentially inaccurate messages instead of the performance impact.
There was a problem hiding this comment.
Yeah, I understand the performance concern but in general this library really tries to reduce or eliminate global configuration settings wherever possible so I'm not comfortable adding a setting that could decrease the veracity of its messages
There was a problem hiding this comment.
If you're willing to wrap this in error handling and add a test I'd be happy to pick up a PR and take it from there, though
There was a problem hiding this comment.
Alright, I will add the try/catch, and we'll see how the test suite performs after that. If the performance hit is major, we'll most likely fork the library and add our own config guard.
Fixes #375
Fixes #416
Note that this will be a performance regression for anybody calling test doubles with large or hard to deep-clone arguments. Since that falls firmly in the category of "not how td.js is designed to be used", I'm going to put this out there and then see what hell it wreaks rather than aggressively benchmark now.