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

Add assertions for ES6 Maps and Sets #632

Closed
teameh opened this issue Mar 4, 2016 · 6 comments
Closed

Add assertions for ES6 Maps and Sets #632

teameh opened this issue Mar 4, 2016 · 6 comments

Comments

@teameh
Copy link

teameh commented Mar 4, 2016

@keithamus:

@tiemevanveen you should raise a new issue - we can discuss the design of this and work towards adding it as a feature 😄


It would be cool if there would be assertions for ES6 Maps (and Sets).

For example something like:

expect(myMap).to.have.key('some_key');

Currently one could use:

expect(myMap.has('some_key'), 'map should have some_key').to.be.true;

But that doesn't work great..

@keithamus
Copy link
Member

We already have a .keys assertion so it makes sense to extend this to detect maps and sets. You would need to wrap the existing key extraction code into the else of an if that type detects maps and sets. So the result would look a little like:

var len = keys.length,
    any = flag(this, 'any'),
    all = flag(this, 'all'),
    actual,
    expected;
if (_.type(obj) === 'map') {
  actual = Array.from(obj.keys());
  expected = keys;
} else {
  actual = Object.keys(obj);
  expected = keys.map(String);
}

A good PR would also include some test coverage, and extend the existing documentation (found in the comments above the method).

@lucasfcosta
Copy link
Member

Hi everyone,
As I was taking a look at this I had a doubt about the type of keys: on Sets and Maps we can have virtually anything as a key, so are we going to accept all types to be passed as arguments?

TL;DR
Should this, for example, be allowed?

var myMap = new Map().set({}, 'value').set({lol: 'lel'}, 'laugh');
expect(myMap).to.have.all.keys({}, {lol: 'lel'});

Edit: Well, I've found a way to accept any object as a key in the case of Maps and Sets and still check for valid arguments when working with any other thing, I think if we're going to allow Maps and Sets to be use with this assertion we need to have full support for them.
The implementation is done and the tests we already have are okay in my fork.
I'll make sure to raise a PR as soon as I have enough tests to back it up. Should I raise it under master or 4.x.x?

@keithamus
Copy link
Member

@lucasfcosta awesome! Look forward to the PR 😄. If it is a breaking change, make it against 4.x.x - but it seems as though it'd be a feature without any breakages, as trying to assert if a map has a key will currently only cause an error; so PRing against master sounds like a good idea.

@bnord01
Copy link

bnord01 commented Mar 22, 2016

@lucasfcosta

Should this, for example, be allowed?

var myMap = new Map().set({}, 'value').set({lol: 'lel'}, 'laugh');
expect(myMap).to.have.all.keys({}, {lol: 'lel'});

I would expect this to fail. Keys in maps and values in sets are by reference and checked that way using has

var k1 = {}, k2={}, myMap = new Map().set(k1,1)
myMap.should.have.key(k1) // myMap.has(k1).should.be.true
myMap.should.not.have.key(k2) // myMap.has(k2).should.be.false

@lucasfcosta
Copy link
Member

@bnord01 thanks for the explanation, I didn't know that. Makes total sense to me. 😃

EDIT
Note: This is done that way on #633

@jdmarshall
Copy link

jdmarshall commented Nov 16, 2023

I'm not a fan of how keys() insist on an exact match instead of a subset match, but includes.keys() can match on subsets.

As implemented, .all. seems to be superfluous. It implies that some other check happens but unless I'm missing something to.have.all.keys() and to.have.keys() perform the exact same match operation, whereas I would expect the former to be more strict.

I'm commenting here only partially to register a complaint, and more so the next person doing a google search figures out why their tests are red.

This may be a documentation complaint though. All of the examples assume that an exact match is desired. The countercase is never explored.

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

No branches or pull requests

5 participants