mjackson / expect

Write better assertions
MIT License
2.29k stars 117 forks source link

Bug report: The assertions does not differentiate between instances and values for primitives #115

Closed FagnerMartinsBrack closed 8 years ago

FagnerMartinsBrack commented 8 years ago

There are cases where expectations on a spy to be called with the same argument of the enclosed function can lead to false positives.

See this link: https://medium.com/@fagnerbrack/mocking-can-lean-to-nondeterministic-tests-4ba8aef977a0

The solutions are:

  1. Use generative testing to leverage triangulation/statistics
  2. Not use mock at all
  3. Check for the reference instead of the values.

This proposal allows this package to enable the solution 3 by making sure that methods that check the arguments are always using strict equality instead of loose equality.

This way it is possible to differentiate objects from literals, otherwise, it is impossible to create an efficient and deterministic test assertion.

I guess this is a bug report because it represents a fundamental assumption of how comparison should be made in unit tests, which is to make sure that every type, comparison or bind is strict by default, just becoming more loose as deem necessary.

mjackson commented 8 years ago

I'd prefer not to do any equality comparisons here and just use is-equal for everything. We deliberately chose to use conceptual equality here instead of strict equality because most of the time I find that's what I want when I use toEqual. To test strict equality, use toBe.

FagnerMartinsBrack commented 8 years ago

@mjackson

How would a consumer of the project use strict equality or prevent a false positive with the example below, without creating additional tests?

var input = new String();
spiedFunction(input);
expect(spy).toNotHaveBeenCalledWith(input)

toNotHaveBeenCalledWith (and its potential equivalent toHaveBeenCalledWith) uses loose equality, which is a mistake. This kind of test should never use loose equality due to the problems of trying to test things like an equal instance, null and undefined, etc.

As pointed out in the article I linked, this code is causing unintended false positives.

FagnerMartinsBrack commented 8 years ago

I'd prefer not to do any equality comparisons here

I am aware of that. That's why I wrote above: This is just a naive implementation, and in the title reinforcing that this is a Bug report. It is just a piece of change that tries to explain the problem better.