Challenges testing mutable objects

[jbbeal]

Description

When testing functions that modify input objects, I don't see a way to validate my code without rewriting due to the fact that testdoubles store arguments by reference instead of creating a copy. Largely a duplicate of #375 , but since that focused on td.explain and I can't re-open, creating as a new issue. Basically, I just spent a couple of hours trying to figure out why I couldn't get a test to pass because of this and it made me salty.

Issue

Here's a simple test case that is similar, conceptually, to the function I was trying to test, and hopefully not a crazy enough use case that this is easy to dismiss. My imaginary use case is part of a system where we try to acquire a lock on an object before operating on it; we have many types of objects that can be locked just by setting "status = locked" on the object and saving to the database, so we want to create a single function to handle the locking behavior:

function lockAndDo ( dataObject, saveFunction, doFunction ) {
  dataObject.status = "LOCKED"
  saveFunction( dataObject )
  doFunction( dataObject )
  dataObject.status = "UNLOCKED"
  saveFunction( dataObject )
}

Obviously, in real life, this method would have a lot more error-checking and so forth. So, here's my failing test:

module.exports = {
  LockingWorkflow: () => {
    const data = { status: "UNLOCKED" }
    const saveFunc = td.func()
    const doFunc = td.func()

    lockAndDo( data, saveFunc, doFunc )

    td.verify( saveFunc( td.matchers.contains( { status: "LOCKED" } ) ) )
    td.verify( doFunc( data ) )
    td.verify( saveFunc( td.matchers.contains( { status: "UNLOCKED" } ) ) )
  }
}

Due to the behavior described in #375 , this test fails on the first td.verify call because the second invocation changes the status on dataObject back to UNLOCKED.

Unless I'm missing something, if I'm trying to write a function that has to work this way (maybe I'm using somebody else's data library), my production code has to make a copy of dataObject between locking and unlocking the object in order to have a test to validate this sequence. I'd much rather take the performance hit of having the test library clone objects!

[searls]

Great issue, well explained, and I understand what you're going for. Sorry for the trouble it's caused you.

Weirdly, I'm actually not able to replicate your example in a simple example script on runkit. It actually passes! Any idea on the difference here?

No doubt you actually are running into this issue, somehow, though. The funny thing to me is that I've been using this library for years, and I have never once hit this snag, and I think it comes down to a few different things I strive to practice:

• Don't write code that mutates objects
• Don't verify things when I could stub responses instead, which typically means that only one assertion of a whole module's worth of tests will typically verify (it's usually the final/terminal dependency that has a side effect for me). I mention this because stubbing doesn't require

All of this to say, that if you can provide a reproducible example of td.js clearly doing the wrong thing, I think this is something we should try to figure out. The real risk is that outright changing the current behavior will break anything that's not cloneable or can be incidentally different-upon-cloning, which is a huge risk IMO

[jbbeal]

Looks like the problem with the repro case is that "UNLOCKED" contains "LOCKED", so "td.matchers.contains('LOCKED')" succeeds on "UNLOCKED". (I thought I'd run it exactly before sending it in, but I must have changed something.) Here's a fixed version where I use 'AVAILABLE' instead of 'UNLOCKED': https://runkit.com/jbbeal/5cfbd6a9fa48c7001ad19085

Definitely understand the feedback, and those are also things that I strive to practice, but there are times where I break those rules. And I hear you on the 'different-upon-cloning' question.

[jbbeal]

That is to say, I understand why it is the way it is, and the challenges of changing this behavior right now. I won't stop using testdouble (I think it's great) if you close this as WONTFIX, but wanted to provide this example.

[jbbeal]

Here's another use case that I learned to work around in setting up my test: https://runkit.com/jbbeal/5cfbda1c858cdc001acad170

This is a case where a test case mistakenly passes. I'm trying to validate that a function loads a fresh copy of an order from our friendly neighborhood RESTful webservice, changes the status from 'OPEN' to 'FULFILLED', and then verifies that the function calls updateOrder to save that change back to the web service. I could change my code to return the object to inspect the value, but that wouldn't guarantee that the code set the status field before calling updateOrder.

I had a few tests that were falsely passing due to this pattern when I first started using testdouble.

[searls]

Ok, great, now we're cooking.

Feature cloneArgs landed in 3.12.0 via #417. See the added docs.

[jbbeal]

Perfect! Thanks for adding this.