debitoor / chai-subset

"containSubset" object properties matcher for Chai
http://chaijs.com/plugins/chai-subset/
MIT License
82 stars 20 forks source link

When errors are reported, they use the state of the expected object at the end instead of at the time they were used. #26

Open Ecafracs opened 8 years ago

Ecafracs commented 8 years ago

When writing a long series of tests with a complicated expectedObject, I'd like to be able to make changes to the expectedObject along the way, so that it's easier to rearrange or insert tests that modify a persistent object.

Unfortunately, when errors are reported at the end, the output is incorrect.

var expectedObject = {val: 1};

it('should fail', function() {
  expect({val: 2}).to.containSubset(expectedObject);
});

it('should pass', function() {
  expectedObject.val++;
  expect({val: 2}).to.containSubset(expectedObject);
});

In this example, the failed test will say something like:

expected: {val: 2}
actual: {val: 2}

This is because it's printing the value of expectedObject at the end of the test run, instead of the value of expectedObject at the time it was run.

I can work around this by cloning the expectedObject passed into containSubset, but it would be much nicer if the library did this for me, as it's only needed when there's an error.

MarkHerhold commented 8 years ago

This is interesting... there are serious performance considerations to cloning every object, so perhaps it could be an option? The other issue is determining how to clone the object.

Ecafracs commented 8 years ago

That's why I'm suggesting to clone it only when there's an error. Another possibility is to start building the final output at the time of the error, instead of waiting until the end when the object has already been changed.

ebdrup commented 8 years ago

I dont think there will be any performance problem if you just clone on error. And substacks deepclone module should do the trick

On 3. mar. 2016, at 02.17, Mark Herhold notifications@github.com wrote:

This is interesting... there are serious performance considerations to cloning every object, so perhaps it could be an option? The other issue is determining how to clone the object.

— Reply to this email directly or view it on GitHub.

eagleeye commented 8 years ago

@Ecafracs how about creating PR for this?)

onetom commented 6 years ago

-1 (probably)

Is there a legitimate use-case though for using such expected values which get mutated? I'm coming from Clojure world, so to me this looks like a rather scandalous practice.

Adding this feature would actually obscure the fact that mutating expected values has been used, hence indirectly encouraging a bad practice for structuring test cases.

On top of that it would complicate the code of this plugin, which translates to more potential bugs, extra dependencies (substack's deepclone) and add a bit more future maintenance burden.

Ecafracs commented 6 years ago

As my silence on this bug might indicate, I haven't really had a pressing need to fix this bug since I'd been working around it, and have also long since moved on from the project that was affected by this bug.

But just to respond to @onetom, I don't see how this could be considered a feature request. It's a bug that results in incorrect output. If I wanted to write more verbose test code to fully specify the expected object as I perform mutations on the test object, I probably wouldn't bother to use this plugin.