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

contain doesn't support non-enumerable properties defined with defineProperties #196

Closed
blacksun1 opened this issue Sep 11, 2016 · 3 comments · Fixed by #197
Closed

contain doesn't support non-enumerable properties defined with defineProperties #196

blacksun1 opened this issue Sep 11, 2016 · 3 comments · Fixed by #197
Assignees
Labels
feature New functionality or improvement

Comments

@blacksun1
Copy link
Contributor

Hi guys,

this is an issue I've found via the Code include assertion (which is basically just a wrapper around Hoek.contain. I've got an object with some properties defined with Object.define(). If the properties are marked as {enumerable: false} then contain fails to see them as defined. Here is a test which shows the issue.

import Code from "code";
import Lab from "lab";
import Hoek from "hoek";

const lab = exports.lab = Lab.script();

lab.it("Hoek.contain should not fail on definedProperties", (done) => {

  const test = {
    "muffin": "Chocolate"
  };
  Object.defineProperty(test, "milkshake", {
    "value": "Chocolate",
    "enumerable": true
  });
  Object.defineProperty(test, "cake", {
    "value": "Chocolate",
    "enumerable": false
  });

  // This passes
  expect(test.muffin, "test.muffin should equal").to.equal("Chocolate");
  expect(test.cake, "test.cake should equal").to.equal("Chocolate");
  expect(test.milkshake, "test.milkshake should equal").to.equal("Chocolate");
  expect(Hoek.contain(test, {"muffin": "Chocolate"}), "muffin should be a property").to.be.true();
  expect(Hoek.contain(test, {"milkshake": "Chocolate"}), "milkshake should be a property").to.be.true();
  // This does not - shouldn't it?
  expect(Hoek.contain(test, {"cake": "Chocolate"}), "cake should be a property").to.be.true();

  return done();
});

Is this by design or is it a bug?

Regards,

@DavidTPate
Copy link

It makes sense with the current implementation that it isn't able to match non-enumerable properties as it is implemented with Object.keys(...) which will only retrieve the enumerable properties.

Not sure if this is just a side effect or not, but wanted to see what was going on.

@blacksun1
Copy link
Contributor Author

FYI: @geek, @nlf, @DavidTPate.

#197

The pull request changes the implementation from Object.keys to Object.getOwnPropertyNames so that it supports non-enumerable members.

@nlf nlf closed this as completed in #197 Sep 19, 2016
@nlf nlf added the feature New functionality or improvement label Sep 19, 2016
@nlf nlf self-assigned this Sep 19, 2016
@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New functionality or improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants