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

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 this issue Mar 3, 2016 · 6 comments

Comments

@Ecafracs
Copy link

Ecafracs commented Mar 3, 2016

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
Copy link
Contributor

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
Copy link
Author

Ecafracs commented Mar 3, 2016

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
Copy link

ebdrup commented Mar 3, 2016

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 [email protected] 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
Copy link
Contributor

@Ecafracs how about creating PR for this?)

@onetom
Copy link

onetom commented Nov 23, 2017

-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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants