mjackson / expect

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

toInclude doesn't work with Map object #176

Open bschlenk opened 7 years ago

bschlenk commented 7 years ago

Example:

it('toInclude should work for Maps', () => {
  const obj = { a: 'a', b: 'b', c: 'c' };
  const map = new Map();
  Object.keys(obj).forEach(k => {
    map.set(k, obj[k]);
  });
  expect(map).toInclude({ b: 'b' });
});

Error: Expected Map (3) {'a' => 'a', 'b' => 'b', 'c' => 'c'} to include { b: 'b' }

ljharb commented 7 years ago

It'd be new Map(Object.entries(obj)) ;-)

The trick here is that there's not a clean way to identify that something is a Map.

You can easily work around this by doing expect([...map]).toInclude(Object.entries({ b: 'b' })).

(official Object.entries polyfill: https://www.npmjs.com/package/object.entries)

bschlenk commented 7 years ago

Thanks for that nicer shorthand :)

Why wouldn't something like this work:

if (value instanceof Map) {
    // handle (expect(Map))
}

Is there some edge case with false positives/negatives that I'm not considering?

ljharb commented 7 years ago

instanceof is never reliable, and doesn't work cross-realm (like another iframe or web worker, or node's vm module).

bschlenk commented 7 years ago

How unreliable is instanceof in a nodejs environment? I'm sure I don't speak for everyone, but I don't ever see myself using expect within a browser. Would it be possible to detect these environments and warn about this being unreliable in those cases, but allow it when in an environment that supports it?

ljharb commented 7 years ago

@bschlenk you have no idea if any of your dependencies are giving you an object that came from another realm, so, just as unreliable as in a browser.

Separately, no, it would be a very bad idea for behavior to be environment-dependent (as opposed to capability-dependent) - that would be very confusing and error-prone to code.

ljharb commented 6 years ago

Detecting native or es6-shimmed Map/Set is doable via an abstraction; detecting core-js-shimmed Map/Set reliably is possible as of v2.5.1+ (iirc). This seems like a reasonable enhancement to consider.