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

deepEqual may ignore object's null/undefined properties, by configuration #332

Closed
jpnovais opened this issue Dec 27, 2014 · 14 comments
Closed

Comments

@jpnovais
Copy link

I think it would be nice to have a chai configuration property to set chai to ignore null or undefined properties on object deepEqual assertion (e.g. chai.config.ignoreNullOrUndefinedProperties = true).

    var objA = {
        foo: 'bar',
        not_relevant: undefined,
        not_serializable: null
    };

    expect(objA).to.eql({foo: 'bar'});
    // false

    expect(JSON.pase(JSON.stringify(objA)).to.eql({foo: 'bar'}));
    // true

This is useful for instance when we want to filter objects's properties before serialization (e.g REST APIs). These objects may have optional properties set to undefined which won't be serialized. Example:

  var pick = require('mout/object/pick');
  var USER_FILTER = ['login','company'];

  function filterUser(user) {
    if(!user) return;

    return {
      login: user.login,
      company: user.company,
    };
    // same as: return pick(user, USER_FILTER);
  };

  var usersBD = {
    bob: {
      login: 'bob',
      lastLoggin: '2014-12-27T00:00:00+00:00',
      password: 'some_hash',
    },
    alice: {
      login: 'alice',
      lastLoggin: '2014-12-27T00:00:00+00:00',
      password: 'some_hash',
      company: 'Wonderland'
    }
  };

  //
  //
  expect(filterUser(usersBD['bob'])).to.eql({login: 'bob'});
  // will fail because filterUser(usersBD['bob']) returns
  // {login: 'bob', company: 'udefined'}
  // but since the object will be  serialized to JSON
  // does not matter

  expect(filterUser(usersBD['alice'])).to.eql({
    login: 'alice',
    company: 'Wonderland'
  }); // success

For this simple case scenario, I cloud do something like:

function filterUser(user) {
  if (!user) return;

  var filteredUser = {
    login: user.login
  };

  if (user.company) {
    filteredUser.company = user.company;
  }

  return filteredUser;
};

But sometimes we have several optional properties which will require more verbose code polluted with if statements.

Currently I overcome this with JSON.stringify.

Is this a good feature, does it meet chai purpose?

@keithamus
Copy link
Member

Sorry for late reply @jpnovais. Thanks for the issue! I'd like to learn a little bit more about the use case before we go forward with implementing code.

I'd argue that the test should be as strict as possible, and in fact filterUser is always returning company, whether or not it is defined. That is the contract set by your filterUser method - seemingly intentionally (because of the explicit return). Perhaps, if I may suggest, you need to rethink your testing strategy. You mention that "since the object will be serialized to JSON does not matter" - I would suggest that and say that your tests need to be higher level then.

I don't speak for the community, so if there is a strong support for this then I believe we should implement it, but I'd personally like to see a stronger use case because I think the current behaviour is correct.

Now, if we were to go down the road of implementing some kind of flag to disregard undefined values, I'd recommend it become a property - perhaps something like expect(foo).to.loose.eql()? chai.config is ideally meant for global config while this (to me) sits firmly in the camp of changing assertion behaviours.

@dclucas
Copy link

dclucas commented Jan 15, 2015

Found this issue when I was looking for a way to easily implement a test for a REST API. We are writing a JSONAPI compliant API. For POST actions, the specs say that the server should generate the IDs in case none are provided.

So, a request such as this

POST /photos
Content-Type: application/vnd.api+json
Accept: application/vnd.api+json

{
  "photos": {
    "title": "Ember Hamster",
    "src": "http://example.com/images/productivity.png"
  }
}

Should yield a response like this:

HTTP/1.1 201 Created
Location: http://example.com/photos/550e8400-e29b-41d4-a716-446655440000
Content-Type: application/vnd.api+json

{
  "photos": {
    "id": "550e8400-e29b-41d4-a716-446655440000",
    "title": "Ember Hamster",
    "src": "http://example.com/images/productivity.png"
  }
}

The ideal solution for us (of course, unless there's a better idea around) would be to perform a comparison that ignores the id of the request JSON object when comparing to the response.

And I agree that modifying global behavior would be less the ideal. For example, the same specs say that when posting with a client-generated ID, it should be stored as-is, so we would not be able to globally ignore an attribute.

@ahamid
Copy link

ahamid commented Nov 18, 2015

if this were available it may be possible to work around #304

@keithamus
Copy link
Member

Having a re-read of this, it seems like this is more like #72 right? It seems like what @jpnovais and @dclucas would get most benefit from is a .properties like assertion, or similar, where you only want to assert on a subset of an object, e.g.

expect({ a: 1, b: 2, c: 3 }).to.have.subset({ a: 1 });

Thoughts?

@jpnovais
Copy link
Author

expect({ a: 1, b: 2, c: 3 }).to.have.subset({ a: 1 }); seems to work for @dclucas. In my case what I need is something like:

expect({ a: 1, b: 2, c: 3 }).to.loose.eql({ a: 1, b: 2 }); 
// FAIL - property c is not expected

expect({ a: 1, b: 2, c: undefined }).to.loose.eql({ a: 1, b: 2 });
// PASS - property c is undefined so it's ignored

expect({ a: 1, b: 2, c: null }).to.loose.eql({ a: 1, b: 2 });
// FAIL/PASS? - property c is null. should null properties be ignored as well?

Is it useful (makes sense) to have a property like this?

@evtaranov
Copy link

+1
I have a similar case to test, where i need to ignore undefined values, because after JSON.parse(JSON.toString(...)) they are lost and deepEqual() return false.

@lucasfcosta
Copy link
Member

lucasfcosta commented Apr 21, 2016

Hello guys, thanks for your feedback! 😄

We currently have this, I think it could help you solve a big part of this problem. I think it would be easy to design a workaround using it. Maybe you could write something like:

var obj = { a: 1, b: 2, c: undefined, d: null };
var subset = {};
for (let key of Object.keys(obj)) {
  if (obj[key] !== null && obj[key] !== undefined) {
    subset[key] = obj[key];
  }
}

expect(obj).to.include.subset(subset);

I'm not sure this use case is common enough for us to have a specific assertion for this, maybe a flag would be enough, but even then it would be easy to design a workaround and maybe some people would like to ignore just undefined or just null so we would have to adapt to these cases. Just wrapping the code I wrote above into a function would be enough if you don't want to apply this recursively (on nested properties). Anyway, I'd like to hear further opinions so everyone gets satisfied. What do you guys think about this?

@gregglind
Copy link
Contributor

Once nesting and recursion become involved, the semantics for 'ignore undefined' becomes more complicated. For top-level key stuff, include.subset seems to be sufficient here.

As an alt. proposal... maybe having a utility function that does the 'subsetting' could be useful? Turning @lucasfcosta 's code above into a (js5!) function, for example. chai.utils.stripObject?

@lucasfcosta
Copy link
Member

Hi @gregglind, thanks for sharing your opinion!
Lodash already has this, it even allows the user to specify a criteria to define which things should be excluded.

If you are interested in it you can take a look at _.pickBy().

@keithamus keithamus mentioned this issue Sep 30, 2016
10 tasks
@keithamus
Copy link
Member

Thanks everyone for your feedback on this issue.

We've got a plan for this in our Roadmap https://github.com/chaijs/chai/projects/2! It'll likely be different to what @jpnovais proposed, but rest assured this is something we'll have a good way of handling come chai 5. We'll be releasing chai 5 soon, but for now I'll close this issue because it is tracked on our roadmap.

@ronjouch
Copy link

ronjouch commented Oct 31, 2018

"We've got a plan for this in our Roadmap https://github.com/chaijs/chai/projects/2 ! It'll likely be different to what jpnovais proposed, but rest assured this is something we'll have a good way of handling come chai 5. We'll be releasing chai 5 soon, but for now I'll close this issue because it is tracked on our roadmap."

Hi @keithamus . The roadmap is a moving artifact, and I think it no longer references what it did at the time you wrote that. Can you name / precise / link to the precise feature of chai 5 you were talking about? Thanks 🙂.

@keithamus
Copy link
Member

Sure @ronjouch! Specifically https://github.com/chaijs/chai/projects/2#card-10444381 and https://github.com/chaijs/chai/projects/2#card-10444260 capture the desired functionality. This is a matcher API which would allow you to pass expectations as properties in a deep-eql assertion, e.g.

expect({ a: 1, b: 2, c: 3 }).to.deep.eql({ a: 1, b: 2, c: expect.to.be.a('number')  }); 

or in the case of the OPs example for a users table you could do something like:

  expect(usersBD['alice']).to.eql({
    login: 'alice',
    password: expect.to.be.a('string').with.lengthOf(40),
    lastLogin: expect.to.be.a('string'),
    company: 'Wonderland'
  }); // success

If you're looking for an assertion which only tests the existance of some keys, we already have this available. But we think having a matcher API to make more broad assertions on object properties is the right direction here.

@rdx-rockstar
Copy link

Hi, @keithamus I am trying to implement it:
expect({ a: 1, b: 2, c: 3 }).to.deep.eql({ a: 1, b: 2, c: expect.to.be.a('number') });
but it gives error: "be not defined"
I can't find it in the documentation so please help me out!

@keithamus
Copy link
Member

This is not a feature yet, but it is a sketch for a future API

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

9 participants