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

fix: accept empty keys when actual object is empty #1384

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Zirak
Copy link

@Zirak Zirak commented Apr 6, 2021

When comparing object keys, the user may happen upon comparing two empty
objects. Before this commit, this would be an error:

// before
expect({}).to.have.all.keys({});
AssertionError: keys required

This commit makes it legal for keys to accept no keys when the actual object
is itself empty.

I happened upon this when generating values in property-based testing. Some
tests had object comparisons as the above, but failed for the empty input,
forcing a dance like this:

if (Object.keys(right).length === 0) {
    expect(left).to.be.an('object').that.is.empty;
} else {
    expect(left).to.have.all.keys(right);
}

There is a negative side to this PR to consider: keys will change its
behaviour depending on the object being inspected. That may be fine or it may be
not, depending on the project's goals.

Another thing to consider is how now there are "two" ways to check if an object
is empty:

  1. expect({}).to.be.empty
  2. expect({}).to.have.all.keys({})

That, I feel, is less of a problem, as there seem to be multiple ways of
achieving the same result elsewhere in the library.

Thanks for your time.

When comparing object keys, the user may happen upon comparing two empty
objects. Before this commit, this would be an error:

    // before
    expect({}).to.have.all.keys({});
    AssertionError: keys required

This commit makes it legal for `keys` to accept no keys when the actual object
is itself empty.
@Zirak Zirak requested a review from a team as a code owner April 6, 2021 17:42
@Zirak
Copy link
Author

Zirak commented Apr 6, 2021

There's something here that I carelessly slipped by. In current chai, assert.hasAllKeys({}) currently throws:

> require('chai').assert.hasAllKeys({})
Uncaught [AssertionError: expected {} to have key 'undefined'] {
  showDiff: true,
  actual: [],
  expected: [ 'undefined' ]
}

In current chai, it could be expected to fail like its siblings, with a keys required message. This is something that can be handled in this commit, or silently accepted and fixed in another PR.

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

Successfully merging this pull request may close these issues.

None yet

1 participant