Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add cloneArgs option to td.when/td.verify #417

Merged
merged 2 commits into from
Jun 8, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
39 changes: 39 additions & 0 deletions docs/5-stubbing-results.md
Original file line number Diff line number Diff line change
Expand Up @@ -615,6 +615,45 @@ fetch('/B', function (er, result) {}) // will be invoked 2nd
fetch('/C').then(function (result) {}) // will be invoked 1st
```

#### cloneArgs

Every now and then, the code under test will mutate the object you initially
pass into a stubbing configuration, and you want to be sure that when the test
double function is invoked by your code, the stubbing is determined to be
satisfied (or not) by comparing the actual call against the value at the time
you configured it.

Confused yet? Here's a quick example of how testdouble.js behaves by default:

```js
const func = td.func()
const person = { age: 17 }
td.when(func(person)).thenReturn('minor')

// Later, in your code
person.age = 30
func(person) // 'minor'
```

Maybe you don't want this! Maybe you want to be sure the stubbing is only
satisfied so long as the arguments are exactly as they were when you configured
the stubbing. You can do that! By setting `cloneArgs` to true, you can do the
following:

```js
const func = td.func()
const person = { age: 17 }
td.when(func(person), { cloneArgs: true }).thenReturn('minor')

// Later, in your code
person.age = 30
func(person) // undefined
```

While passing in data with an expectation that it be mutated by your subject is
generally Not Recommended™, this option should enable edge cases where mutation
is unavoidable or out of your control.

## Congratulations!

And that's about all there is to say about stubbing. Great news, because the
Expand Down
35 changes: 35 additions & 0 deletions docs/6-verifying-invocations.md
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,41 @@ var doNotCall = td.function()
td.verify(doNotCall(), {times: 0, ignoreExtraArgs: true}) // passes
```


#### cloneArgs

What if you want to verify a call took place and the subject (for better or
worse) mutated an argument after it was passed to the test double function?
Since testdouble.js saves arguments by reference by default, you won't get the
result you want:

```js
const func = td.func()
const person = { age: 17 }

// later, in your code
func(person)
person.age = 30

// back in your test
td.verify(func({ age: 17 })) // 💥 Test failure! td.js recorded age as 30!
```

For cases like these, you can work around the mutation by setting `cloneArgs` to
`true`:

```js
const func = td.func()
const person = { age: 17 }

// later, in your code
func(person)
person.age = 30

// back in your test
td.verify(func({ age: 17 }), { cloneArgs: true }) // 😌 all good
```

## Congratulations!

And that's everything there is to know about verifying behavior with
Expand Down
2 changes: 1 addition & 1 deletion src/explain.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ function argsFor (stub) {

function callDescription (calls) {
return calls.length > 0
? _.reduce(calls, (desc, call) => desc + `\n - called with \`(${stringifyArgs(call.args)})\`.`, '\n\nInvocations:')
? _.reduce(calls, (desc, call) => desc + `\n - called with \`(${stringifyArgs(call.cloneArgs)})\`.`, '\n\nInvocations:')
: ''
}

Expand Down
5 changes: 3 additions & 2 deletions src/store/calls.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@searls searls Aug 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@searls I opened #469, we can continue the discussion there. Thanks!

return callHistory.push({ testDouble, args, context })
},

Expand All @@ -30,7 +30,8 @@ export default {

where (testDouble, args, config) {
return _.filter(store.for(testDouble).calls, function (call) {
return argsMatch(args, call.args, config)
const pastArgs = config.cloneArgs ? call.cloneArgs : call.args
return argsMatch(args, pastArgs, config)
})
},

Expand Down
2 changes: 1 addition & 1 deletion src/store/stubbings.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export default {
return store.for(testDouble).stubbings.push({
callCount: 0,
stubbedValues,
args,
args: config.cloneArgs ? _.cloneDeep(args) : args,
config
})
},
Expand Down
2 changes: 2 additions & 0 deletions src/wrap/lodash.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as assign from 'lodash/assign'
import * as capitalize from 'lodash/capitalize'
import * as clone from 'lodash/clone'
import * as cloneDeep from 'lodash/cloneDeep'
import * as cloneDeepWith from 'lodash/cloneDeepWith'
import * as compact from 'lodash/compact'
import * as defer from 'lodash/defer'
Expand Down Expand Up @@ -41,6 +42,7 @@ export default {
assign,
capitalize,
clone,
cloneDeep,
cloneDeepWith,
compact,
defer,
Expand Down
36 changes: 34 additions & 2 deletions test/safe/explain.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ module.exports = {
assert._isEqual(result, {
name: undefined,
calls: [
{ context: 'lol', args: [88] },
{ context: 'woo', args: ['not 88', 44] }
{ context: 'lol', args: [88], cloneArgs: [88] },
{ context: 'woo', args: ['not 88', 44], cloneArgs: ['not 88', 44] }
],
callCount: 2,
description: theredoc`
Expand Down Expand Up @@ -205,6 +205,7 @@ module.exports = {
callCount: 1,
calls: [{
args: [],
cloneArgs: [],
context: baz
}],
description: 'This test double `foo` has 1 stubbings and 1 invocations.\n\nStubbings:\n - when called with `()`, then return `"biz"`.\n\nInvocations:\n - called with `()`.',
Expand Down Expand Up @@ -316,5 +317,36 @@ module.exports = {
},
isTestDouble: true
})
},
'a double with a mutated argument' () {
const person = { age: 17 }
testDouble.call('hi', person)
person.age = 30

result = td.explain(testDouble)

assert._isEqual(result, {
name: undefined,
calls: [
{ context: 'hi', args: [{ age: 30 }], cloneArgs: [{ age: 17 }] }
],
callCount: 1,
description: theredoc`
This test double has 0 stubbings and 1 invocations.

Invocations:
- called with \`({age: 17})\`.`,
children: {
toString: {
name: undefined,
callCount: 0,
calls: [],
description: 'This is not a test double function.',
isTestDouble: false
}
},
isTestDouble: true
})
}

}
28 changes: 26 additions & 2 deletions test/safe/verify.test.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
function shouldFail (fn, message) {
try {
fn()
assert.fail('should error')
assert.fail('expected to have failed, but did not')
} catch (e) {
assert._isEqual(e.message, message)
if (message) {
assert._isEqual(e.message, message)
}
}
}

Expand Down Expand Up @@ -282,6 +284,28 @@ module.exports = {
td.verify(testDouble())

assert._isEqual(warnings.length, 0)
},
'verification of a mutated value WITHOUT cloning should fail' () {
const person = { age: 20 }
testDouble(person)

person.age = 21

shouldFail(() => {
td.verify(testDouble({ age: 20 }))
})
},
'verification of a mutated value WITH clone: true should succeed' () {
const person = { age: 20 }
testDouble(person)

person.age = 21

td.verify(testDouble(person), { cloneArgs: false })
td.verify(testDouble({ age: 20 }), { cloneArgs: true })
shouldFail(() => {
td.verify(testDouble(person), { cloneArgs: true })
})
}
}
}
10 changes: 10 additions & 0 deletions test/safe/when.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -304,5 +304,15 @@ module.exports = {
result = knob('twist').door('push')

assert._isEqual(result, 'open')
},
'support cloneArgs option' () {
const person = { age: 19 }
td.when(testDouble(person), { cloneArgs: false }).thenReturn('no-clone')
td.when(testDouble(person), { cloneArgs: true }).thenReturn('clone')

person.age = 20

assert._isEqual(testDouble({ age: 19 }), 'clone')
assert._isEqual(testDouble({ age: 20 }), 'no-clone')
}
}