Assertion in 1.9.2 throws JS error in 1.10.0

[aarontam]

We have several assertions in our core tests of the form

expect(foo).to.exist.and.to.have.length(bar)

After upgrading to 1.10.0 from 1.9.2, these assertions now throw an error

TypeError: '0' is not a function (evaluating 'expect(foo).to.exist.and.to.have.length(bar)')

I found that changing the assertion to

expect(foo).to.exist().and.to.have.length(bar)

seems to fix the problem, but I'm not entirely sure why. Any insight would be appreciated, thanks!

[lennym]

I have a similar issue in that assertions that were working in 1.9.2 now fail in 1.10.0 with the same TypeErrors for things not being functions.

In my case it is the calledWith and calledWithExactly methods in sinon-chai

Example:

callback.should.have.been.calledWith(elem);
// => TypeError: 'undefined' is not a function (evaluating 'callback.should.have.been.calledWith(elem)')

Edit: using sinon-chai@2.6.0

[lennym]

On further investigation, this seems to only be a problem for our in-browser tests run using karma, and not for unit tests run in a Node environment.

Karma is running in Phantomjs@1.9.8, Chrome@36.0.1985 and Firefox@33.0.0. The failures and errors are the same in each case.

[keithamus]

Seems like this is because the new end assertions return no-op functions, which have immutable properties (length, name).

When doing expect([1,2,3]).to.exist you're getting back the no-op function - rather than the Assertion instance, so by the time you get to expect([1,2,3]).to.exist.and.have.length you're actually getting back the Functions immutable length property, rather than Chai's length Assertion. In other words:

> expect([1,2,3]).to.exist()
{ __flags:
   { ssfi: [Function: assert],
     object: [ 1, 2, 3 ],
     message: undefined } }
> expect([1,2,3]).to.exist() instanceof chai.Assertion
true
> expect([1,2,3]).to.exist
[Function: assert]
> expect([1,2,3]).to.exist instanceof chai.Assertion
true
>

@joshperry got any ideas to get around this?

[keithamus]

Thinking out loud:

If we make every property/method check to see if it is getting the no-op function - and simply resolve it, then we can potentially fix this issue - having said that, while we might be able to get around expect([1,2,3]).to.exist.and.have.length(3);, there will be no way to fix expect([1,2,3]).exist.length(3);. The severity of this behaviour depends on how many people use the verbose syntax (former) vs just the assertions (latter).

The other way around would be to make chainableNoop return a function that does the assertion, rather than doing the assertion in the getter, but this will break code like expect([1,2,3]).to.exist; which is obviously a no-go.

Of course, another option would be to revert the originating PR and go back to having JSLint unfriendly assertions. I'd personally like to see them kept but not at the price of breaking other code or adding odd quirks.

[joshperry]

Like you said, this will be a problem with any of the properties on a function object that can not be overwritten: length, name, arguments, caller. I can definitely picture tests out there that look something like expect(args).to.be.arguments.and.have.length(3);

I don't think any breaking changes are going to be acceptable, and we aren't going to get this without breaking length and arguments assertions. As much as I don't want to, it looks like we should probably pull this PR and add some regression tests.

Another plan could be to create a new assertion called something like exec that is a function, which would work out-of-the-box with other plugins and would be easily implemented as a plugin itself. This way we can do normal chaining appended with this assert: expect([1,2,3]).to.exist.exec(); callback.should.have.been.calledOnce.exec(); sinon-chai

[logicalparadox]

As I don't want to prevent our users from upgrading in the future, I'm going to go with @joshperry on this and say revert the PR until an acceptable solution can be implemented.

As for .exec(), I recommend this as a plugin.

[aarontam]

Thanks for the quick investigation / explanation - will remove the version restriction after the reversion.

[keithamus]

PR was reverted. Although 1.10.0 is still the latest release. @logicalparadox we should probably release a new version which reverts the changes - I vote for v1.10.1 as this fixes a pretty big bug in v1.10.0 :wink:

[keithamus]

@logicalparadox if you're around - this could do with taking a look at. IMO commit b71b930d83213ece12829bfbd99bc26c5ca85fb8 should be released as Chai v1.10.1 (it reverts the functionality from #306 - which this issue talks about).

After #312 and #313 we could probably also cut a v1.11.0 release, as we've got lots of new stuff in :smile:

[scottohara]

On Nov 11 2014, @lennym wrote..

On further investigation, this seems to only be a problem for our in-browser tests run using karma, and not for unit tests run in a Node environment.

Karma is running in Phantomjs@1.9.8, Chrome@36.0.1985 and Firefox@33.0.0. The failures and errors are the same in each case.

@lennym - did you ever get this issue sorted?

I'm seeing a similar problem where foo.should.have.been.calledWith(bar) is throwing undefined is not a function.

My test suite used to work before upgrading to chai@1.10.0. Curiously, I tried reverting back to chai@1.9.2 but I still get the same error.

UPDATE: I did manage to successfully rollback to 1.9.2, by deleting the node_modules folder and reinstalling (ie. rm -rf node_modules && npm install). Rolling back by npm install chai@1.9.2 wasn't sufficient.

At first, I thought it was only calledWith() that had the problem, because changing the test to foo.should.have.been.called passed; but then I realised that foo.should.not.have.been.called also passed (which makes no sense...how can should.have.been and should.not.have.been both be passing?)

This occurs with both the should and expect APIs, but not the assert API. eg.

var foo = sinon.spy();
foo('bar');

// should
foo.should.have.been.called;  // PASS  (ok)
foo.should.not.have.been.called;  // PASS  (huh?)
foo.should.have.been.calledWith('bar');  // undefined is not a function

// expect
expect(foo).to.have.been.called; // PASS  (ok)
expect(foo).to.not.have.been.called; // PASS  (wat?)
expect(foo).to.have.been.calledWith('bar'); // undefined is not a function

// assert
assert.isTrue(foo.called);  // PASS  (ok)
assert.isFalse(foo.called);  // FAIL  (ok)
assert.isTrue(foo.calledWith('bar'));  // PASS (ok)

This behaviour was seen in karma in Chrome@41.0.2251.0, using the following npm modules:

$ npm ls --depth=0
├── chai@1.9.2
├── chai-as-promised@4.1.1
├── karma@0.12.30
├── karma-chai@0.1.0
├── karma-chrome-launcher@0.1.7
├── karma-mocha@0.1.10
├── karma-sinon@1.0.4
├── karma-sinon-chai@0.2.0
├── mocha@2.1.0
└── sinon@1.12.2

I checked if any of these modules bundled their own version of chai (perhaps one was still using 1.10.0 and ignoring my rolled-back 1.9.2 version?). Only karma-sinon-chai does, but it seems to be using 1.9.2 as well:

├─┬ karma-sinon-chai@0.2.0
│ ├─┬ chai@1.9.2
│ │ ├── assertion-error@1.0.0
│ │ └─┬ deep-eql@0.1.3
│ │   └── type-detect@0.1.1

[joshperry]

@scottohara It looks to me like the sinon-chai assertions were not being registered with chai for some reason. This is a great example of some small issue causing assertion properties to produce false-positives because javascript silently ignores non-existent property accesses.

[keithamus]

Closing this - as we have a 2.0.0 tracking PR. 2.0.0 will revert the behaviour that is causing this issue. If anyone disagrees, feel free to comment and I'll re-open it.

2.0.0 tracking issue is https://github.com/chaijs/chai/pull/364