Allowing .keys() to accept Object #265

Closed
thejameskyle opened this Issue May 31, 2014 · 7 comments

Comments

Projects
None yet
4 participants
Contributor

thejameskyle commented May 31, 2014

describe('myFunction', function() {
  beforeEach(function() {
    this.bar = { baz: stubFn, tah: stubFn };
    this.foo = myFunction(this.bar);
    // >> { baz: [{ callback: stubFn, context: this.bar }], tah: [{ callback: stubFn, context: this.bar }] }
  });

  it('should work with an array', function() {
    expect(this.foo).to.have.keys(Object.keys(this.bar));
  });

  // vs.

  it('should work with an object', function() {
    expect(this.foo).to.have.keys(this.bar);
  });
});
Contributor

charlierudolph commented Oct 14, 2014

I don't think this reads very well or makes much sense.
Maybe some new syntax?

expect(foo).to.match.keys.of(bar)   // foo has the same keys as bar
expect(foo).to.contain.keys.of(bar) // foo's keys are a superset of bar's keys
Owner

keithamus commented Nov 26, 2014

I like @charlierudolph's suggestions. But maybe this needs discussing a bit further before anyone attempts a PR. @thejameskyle what are your thoughts on what @charlierudolph suggested?

Contributor

thejameskyle commented Nov 26, 2014

I can no longer remember the exact use case, but either syntax sounds good to me. At the time I figured keys would've been the easiest place to implement it.

Contributor

gregglind commented Feb 7, 2015

+1 on allowing/extending .keys (object), using Object.keys ().

Owner

keithamus commented Feb 7, 2015

Yeah, after taking the time to re-read this I am kind of in favour of .keys() being able to detect objects and extract the Object.keys() out from them; so the new syntax would just be .contain.keys(object);.

If anyone wants to take a stab at a PR; the keys assertion is L1004-L1074 of assertions.js, but the logic around extracting keys from the argument set is L1009-1011 - it should be a simple case of checking to make sure its only one argument, and that argument is an object - then calling Object.keys() on it. Probably the majority of the work is adding extra tests, (L571-669 of test/expect.js) and also extending the documentation appropriately (L970-1002 of assertions.js)

gregglind referenced this issue Feb 8, 2015

Merged

Fix #265 #361

Contributor

gregglind commented Feb 8, 2015

Notes:

  • testing was a lot of "cut and paste*. For the future with 'alias' like tests... maybe easier to test that the return / do the same assertion? (Unless that is too 'internal')
  • Thinking about this feature, I wonder if we are giving people a footgun here. "Normal" objects will work fine, in as much that on simple POJO's Object.keys(pojo) does the right thing. Are people going to try to pass in constructed objects, or one with non-enumerable properties?
  • "object, but not array" testing in JS continues to be stupid.
Owner

keithamus commented Feb 8, 2015

  • I think keeping the tests as simple as possible is a good idea. While they may be repetitive, they are easy to read and don't involve "dynamic logic" (for want of a better phrase)
  • I think this is as much of a footgun as passing objects to any assertion. We could add in detection to ensure a POJO is being passed in - but I think leaving it down to the dev is fine. Note that keys({}) would throw because it does a check for keys.length
  • See comments in PR about this.

keithamus closed this in 5753ecb Feb 9, 2015

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