Add `propertyDescriptor` assertion #408

Merged
merged 1 commit into from Mar 30, 2015

Projects

None yet

2 participants

@ljharb
Contributor
ljharb commented Mar 25, 2015

No description provided.

@keithamus
Member

@ljharb thanks for the PR!

I'm not entirely sure if having enumerableProperty is the best method to have. Mainly because if we had the full descriptor set, we'd have to have writableProperty, configurableProperty, etc.

Maybe it'd be better to have a generic property to get access to the descriptor interface, something like:

expect(myObject).to.have.propertyDescriptor('length').and.have.property('enumerable', true);

Slightly longer but much more flexible. Thoughts?

@ljharb
Contributor
ljharb commented Mar 25, 2015

Interesting - I do see how that'd be more flexible, but it's much more verbose - I want to use this in the es6-shim where I have lots of tests that assert that a property exists and is (or is not) enumerable. I don't often check configurability or writability, nor the existence of getters and setters.

How complex would your suggestion be to add? I'm not sure I yet know enough about the internals of chai to be able to add it quickly.

@keithamus
Member

If we were to go with my suggestion (I'm not mandating it, just giving some food for thought) then you're about half way there... you'd need something like:

function propertyDescriptor (name, msg) {
  if (msg) flag(this, 'message', msg);
  var obj = flag(this, 'object');
  var descriptor = Object.getOwnPropertyDescriptor(Object(obj), name);
  flag(this, 'object', descriptor);
  this.assert(
      descriptor
    , 'expected #{this} to have descriptor ' + _.inspect(name)
    , 'expected #{this} not to have have descriptor ' + _.inspect(name)
  );
}

Take particular note of flag(this, 'object', descriptor) which sets the object to assert on for the rest of the chain. Meaning that the .and.have.property() part asserts on the descriptor, not the initial object.

@keithamus
Member

I'd also say, either way, we should probably also make the assertion deep aware, so that developers could do .to.have.deep.enumerableProperty (or .to.have.deep.propertyDescriptor if that's the path we go down).

@keithamus
Member

Just to play devils advocate some more, you could use existing assertions in a fairly non-verbose way (and a way closer to how the API would be consumed) to check non-enumerability:

it('should have non enumerable length', function () {
    expect(object).to.not.include.keys('length');
    expect(object).to.have.property('length');
});
@ljharb
Contributor
ljharb commented Mar 25, 2015

The failure message for "not to include keys" isn't very clean though, since it would list all the keys iirc.

I'll play with this and see what I come up with, thanks.

@ljharb
Contributor
ljharb commented Mar 26, 2015

@keithamus For the "deep" part - does that mean that toHavePropertyDescriptor would by default only check own properties? That's not how Object.getOwnPropertyDescriptor works, so that'd be a bit trickier.

@keithamus
Member

The deep flag should be used to access far down properties, for consistency with .property, e.g.

expect({ foo: bar: new Thing() }).to.have.deep.propertyDescriptor('foo.bar.length');

We could of course call it .ownPropertyDesciptor to make it even more obvious that it aligns with Object.getOwnPropertyDescriptor - especially as I think Object.getPropertyDescriptor is coming in ES6/7.

expect(new Thing()).to.have.ownPropertyDescriptor('length');
@ljharb
Contributor
ljharb commented Mar 26, 2015

@keithamus I've updated the PR to cover the generic propertyDescriptor assertion. I didn't add anything for "deep" because I don't know what that entails :-)

@ljharb ljharb changed the title from Add `enumerableProperty` assertion to Add `propertyDescriptor` assertion Mar 26, 2015
@keithamus
Member

Also, as an aside, I'm not sure we need havePropertyDescriptor - please do correct me if I'm wrong but I believe the rest of the codebase avoids combining verbs in methods. Just having propertyDescriptor is sufficient.

@ljharb
Contributor
ljharb commented Mar 26, 2015

I copied the style verbatim from ownProperty above it, which has haveOwnProperty. Would you prefer I remove it? (Personally I prefer the alias, as I have a strong dislike for chaining non-function getters, but that's pretty tough to avoid with mocha/chai anyways)

@keithamus
Member

Yup, I see. I'll eat my words then, havePropertyDescriptor can stay.

What are your thoughts about renaming them both to [have]OwnPropertyDescriptor?

P.S. the keywords in chai are entirely optional expect(foo).ownProperty('bar') works the same as expect(foo).to.have.an.ownProperty('bar')

@ljharb
Contributor
ljharb commented Mar 27, 2015

Sure, it's Object.getOwnPropertyDescriptor so that makes sense. I'll fix that.

@ljharb
Contributor
ljharb commented Mar 27, 2015

@keithamus hmm - if I were to add deep support, then it would read .to.have.deep.ownPropertyDescriptor which doesn't make sense, whereas .to.have.deep.propertyDescriptor does. Thoughts?

@keithamus
Member

Hmm, that is indeed a quandary. If es6/7 is to get Object.getPropertyDescriptor then we have a semantics problem if we use .propertyDescriptor.

My feelings are we should do the simplest thing first - so adding .ownPropertyDescriptor, without deep support. If deep support is requested later, we can add it. If it later transpires it can be shortened to .propertyDescriptor without conflicting with ES* semantics then we can shorten it later. How do you feel about that?

@ljharb
Contributor
ljharb commented Mar 28, 2015

ES6 absolutely doesn't have that, and as a member of TC39 I haven't seen any proposals for it for ES7.

I'm fine with calling it ownProperty and not adding deep support, for now.

@ljharb
Contributor
ljharb commented Mar 28, 2015

Just pushed an update with ownProperty and no deep support.

@keithamus keithamus commented on the diff Mar 29, 2015
lib/chai/core/assertions.js
@@ -889,6 +889,53 @@ module.exports = function (chai, _) {
Assertion.addMethod('haveOwnProperty', assertOwnProperty);
/**
+ * ### .ownPropertyDescriptor(name[, descriptor[, message]])
+ *
+ * Asserts that the target has an own property descriptor `name`, that optionally matches `descriptor`.
+ *
+ * expect('test').to.have.ownPropertyDescriptor('length');
@keithamus
keithamus Mar 29, 2015 Member

Could you please add a few more examples here? Specifically I'd like to see the following:

expect('test').to.have.ownPropertyDescriptor('length', { enumerable: false, configurable: false, writable: false, value: 4 });
expect('test').ownPropertyDescriptor('length').to.have.property('enumerable', false);
expect('test').ownPropertyDescriptor('length').to.have.keys('value');

Feel free to adjust those as you see fit - but it should show a couple of examples to demonstrate what's possible (especially for any extra arguments).

@ljharb
ljharb Mar 30, 2015 Contributor

Sure, those look great :-)

@keithamus keithamus and 1 other commented on an outdated diff Mar 29, 2015
lib/chai/core/assertions.js
+ actualDescriptor
+ , 'expected #{this} to have an own property descriptor for ' + _.inspect(name)
+ , 'expected #{this} to not have an own property descriptor for ' + _.inspect(name)
+ );
+ } else if (descriptor) {
+ this.assert(
+ _.eql(descriptor, actualDescriptor)
+ , 'expected the own property descriptor for ' + _.inspect(name) + ' on #{this} to match ' + _.inspect(descriptor) + ', got ' + _.inspect(actualDescriptor)
+ , 'expected the own property descriptor for ' + _.inspect(name) + ' on #{this} to not match ' + _.inspect(descriptor)
+ , descriptor
+ , actualDescriptor
+ , true
+ );
+ } else {
+ this.assert(true);
+ }
@keithamus
keithamus Mar 29, 2015 Member

Logic could be cleaned up here to avoid the this.assert(true):

    if (descriptor) {
      this.assert(
          _.eql(descriptor, actualDescriptor)
        , 'expected the own property descriptor for ' + _.inspect(name) + ' on #{this} to match ' + _.inspect(descriptor) + ', got ' + _.inspect(actualDescriptor)
        , 'expected the own property descriptor for ' + _.inspect(name) + ' on #{this} to not match ' + _.inspect(descriptor)
        , descriptor
        , actualDescriptor
        , true
      );
    } else {
      this.assert(
          actualDescriptor
        , 'expected #{this} to have an own property descriptor for ' + _.inspect(name)
        , 'expected #{this} to not have an own property descriptor for ' + _.inspect(name)
      );
    }
@ljharb
ljharb Mar 30, 2015 Contributor

Hmm - the way you've changed it, if descriptor is passed but actualDescriptor is not present, then the wording won't be as clear imo that the property descriptor wasn't there at all. In other words, if the actualDescriptor is not present, it doesn't really matter what you passed as descriptor the actionable test failure is "the property didn't exist", not "it didn't match".

However, I think I can tweak it to avoid the assert(true) which I think is the purpose of your comment :-)

Fix incoming.

@keithamus
Member

Sweet! Awesome work @ljharb. Consider this merged 😄

@keithamus keithamus merged commit 8a29595 into chaijs:master Mar 30, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
This was referenced Mar 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment