Please allow `undefined` for `actual` when using `assertionInstance.assert()`? #183

Closed
domenic opened this Issue Jun 29, 2013 · 7 comments

Projects

None yet

3 participants

@domenic
Contributor
domenic commented Jun 29, 2013

Because of the getActual utility, it's impossible to do something where the actual value is undefined. E.g. if writing a plugin that tests someSpecialProperty's value,

this.assert(
  this._obj.someSpecialProperty === "foo",
  "expected foo, got #{act}",
  "foo",
  this._obj.someSpecialProperty
);

will give a bad AssertionError when used on { the: "object" }, which has actual equal to the object and prints something like "expected foo, got { the: "object" }".

@logicalparadox
Member

Though I see your point, I'm not keen on reversing the usage of getActual and modifying it would require changes to a whole bunch of function argument signatures for the plugin API which I don't want to do until there is more to gain. Something for the long-term checklist.

Until then, I see two options:

A. If you are just testing property values you can easily use an embedded Assertion. Gives a nice error.

new Assertion(this._obj).to.have.property('someSpecialProperty');
this.assert( /* ... */ );

B. Trickery Hackery

var prop = this._obj.someSpecialProperty;
this.assert(
  prop === 'foo',
  'expected foo, got #{act}',
  'expected not to get foo',
  'foo',
  _.inspect(prop) // chai util's inspect always returns string
);
@domenic
Contributor
domenic commented Jul 2, 2013

A. If you are just testing property values you can easily use an embedded Assertion.

Unfortunately I am indeed testing some more complicated internal state (promise fulfillment values, which you access via async callbacks). So this won't do the trick. Although, it does seem likely we have other cases in our API where undefined would be a natural actual value.

I guess right now property doesn't distinguish between "property does not exist", i.e. !(property in obj), and "property is undefined", i.e. obj.property === undefined. This will become increasingly important in ES6 where those will trigger different proxy traps; it's already important in ES5 where the latter triggers any getter while the former does not.

B. Trickery Hackery

That was my first attempt, but unfortunately the error message then looks like

"expected 'foo', got 'undefined'"

as opposed to

"expected foo, got undefined"
@domenic
Contributor
domenic commented Jul 2, 2013

Another way of looking at the problem is that constructing AssertionError instances without assert is error prone and hard to customize. The way showDiff and includeStack are derived seem to be part of the problem, as I'd have to duplicate that logic in my own AssertionError construction to maintain consistency. There's also implicit logic I'd have to pull out of getMessage regarding the message flag.

@logicalparadox
Member

I agree, our current implementation isn't optimal for all use cases. Perhaps it is time to start thinking about what the "nextgen" this.assert looks like. A couple of things off the top of my head:

1. Move it off of the Assertion constructor an on to the utils. This could pave the way for rewriting the assert interface to be legacy capable. Also, we don't break backwards compatibility for plugins that don't make the conversion immediately and we can depreciate this.assert on a major release.

1a. We would also have to revisit how configurations are stored in order for them to be recognized outside of Assertion. For example: chai.Assertion.showStack would become chai.config.showStack and etc.

2. Change the argument signature to accept a single argument of an object. We can figure out what property names to use later, but this would provide finer grain control for plugin authors of the settings of getMessage and etc.

As both resident ES6 expert and proponent of legacy support, does this seem like a good way to approach this problem? Can we write it in such a way that it will work across the board?

@Bartvds
Contributor
Bartvds commented Jul 2, 2013

If I may pitch in on this last message, specifically the part about decorating the chai object:

A official way to access an exported value of each plugins from within the test suites, like chai.plugins.myPlugin.<..> would be nice for programmability.

Also an export of the various utils, something like chai.util.inspect(). This could also hold helpers like isArray, objectDiff etc so we can use them when debugging failing tests instead of carting them around ourselves (and without dual import issues when writing cross node/browser tests).

And maybe it's interesting to think about the ideal way to present a custom multi-line error message. Its easy to just pass it in as the regular message but this does clutter reporter 'Spec' or 'List': you'd want a single-line in the spec-tree-structure and a detailed report below (like the way the string-diffs and stack trace show up). I'm not sure if this is possible with the current mocha though: I tried mashing things in the actual / expected values and disabling showDiff but mocha will always diff strings.

@logicalparadox
Member

@Bartvds

  • Exposing the utils seems like a good idea but I think it would be better suited for discussion after we tackle util.assert. Some things are very specific to Chai internals and shouldn't be exposed.
  • How failures are displayed is not a concern of Chai's - we leave that up to the reporter. Since we are test runner agnostic you will have to take that up with mocha.
    • BUT --- I understand where you are coming from. As such, I have put together my thoughts on that over at chaijs/assertion-error#1. Once we have a more standard idea of what we want to report we can rally the test runners to display more information.
@domenic
Contributor
domenic commented Jul 4, 2013

Would you be willing to accept a band-aid fix for this in Chai 1.x? Maybe something as silly as a last flag to this.assert that is allowUndefinedActual and defaults to false? Or maybe if your actual object is in the form { ___chai_really_actual___: x } then Chai unconditionally unwraps it into x?

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