Tests failing due to #408

[keithamus]

Hey @ljharb, the Travis job just failed because the tests in #408 seem to fail within Firefox 22.

Here are the results from the build: https://travis-ci.org/chaijs/chai/builds/56420793

Firefox 22.0.0 (Windows 7) expect ownPropertyDescriptor(name) FAILED
    expected 'blah: expected the own property descriptor for \'test\' on { test: NaN } to match { configurable: false,\n  enumerable: true,\n  writable: false,\n  value: NaN }, got { configurable: false,\n  enumerable: true,\n  value: NaN,\n  writable: true }' to equal 'blah: expected the own property descriptor for \'test\' on { test: NaN } to match { configurable: false,\n  enumerable: true,\n  writable: false,\n  value: NaN }, got { value: NaN,\n  writable: true,\n  enumerable: true,\n  configurable: false }'

Firefox 22.0.0 (Windows 7) expect ownPropertyDescriptor(name) FAILED
    expected 'blah: expected the own property descriptor for \'test\' on { test: NaN } to match { configurable: false,\n  enumerable: true,\n  writable: false,\n  value: NaN }, got { configurable: false,\n  enumerable: true,\n  value: NaN,\n  writable: true }' to equal 'blah: expected the own property descriptor for \'test\' on { test: NaN } to match { configurable: false,\n  enumerable: true,\n  writable: false,\n  value: NaN }, got { value: NaN,\n  writable: true,\n  enumerable: true,\n  configurable: false }'

Firefox 22.0.0 (Windows 7) should ownPropertyDescriptor(name) FAILED
    expected 'blah: expected the own property descriptor for \'test\' on { test: NaN } to match { configurable: false,\n  enumerable: true,\n  writable: false,\n  value: NaN }, got { configurable: false,\n  enumerable: true,\n  value: NaN,\n  writable: true }' to equal 'blah: expected the own property descriptor for \'test\' on { test: NaN } to match { configurable: false,\n  enumerable: true,\n  writable: false,\n  value: NaN }, got { value: NaN,\n  writable: true,\n  enumerable: true,\n  configurable: false }'

Firefox 22.0.0 (Windows 7) should ownPropertyDescriptor(name) FAILED
    expected 'blah: expected the own property descriptor for \'test\' on { test: NaN } to match { configurable: false,\n  enumerable: true,\n  writable: false,\n  value: NaN }, got { configurable: false,\n  enumerable: true,\n  value: NaN,\n  writable: true }' to equal 'blah: expected the own property descriptor for \'test\' on { test: NaN } to match { configurable: false,\n  enumerable: true,\n  writable: false,\n  value: NaN }, got { value: NaN,\n  writable: true,\n  enumerable: true,\n  configurable: false }'

To summarise them - it looks as though Firefox 22 just re-orders configuration properties strangely, so the strings that try to catch the error are wrong (because the properties are in the wrong order. It looks like its expect.js#L550, and expect.js#L559.

If you could address these in a new PR that'd awesome. Thanks @ljharb 😄

[ljharb]

Absolutely, I'll get to that ASAP!

How do I run the tests in Firefox locally?

[ljharb]

Also, would an acceptable solution be to use _.inspect for the expected object in the test? Then engine-specific key orderings wouldn't be covered, nor would they break the test.

[keithamus]

Easiest way to get firefox tests running locally is to, well, install firefox and run them. We have a karma config which you could use to make things easier*:

• Temporarily edit and replace the 'PhantomJS' keyword with 'Firefox'
• Change singleRun to false
• npm install karma-firefox-launcher
• karama start
• Edit tests, watch them run in your terminal

As for your other comment, using _.inspect sounds perfectly acceptable.

* Instructions off the top of my head, may not actually work

[ljharb]

Thanks, I'll try that. it'd be wonderful to have a make or npm run-script task so common tasks like this could be discovered and available by default :-)

[keithamus]

We have a Makefile, and running make test will run node and phantom tests. Running make test-sauce will run the tests in all browsers, but the sauce tests don't run without a saucelabs account credentials available.

If you have a saucelabs account, then you can set SAUCELABS_USERNAME to your username SAUCELABS_ACCESSKEY to your access key, and the tests will run successfully.

I can see how its a bit of a pain. It'd be nice if Travis could securely run our saucelabs tests for PRs (see travis-ci/travis-ci#1301) but we are we we are. If you have any suggestions for improvements I'd love to hear them, feel free to raise as many issues as you see fit 😄

[ljharb]

An HTML file that I could run in various browsers, that sauce also ran, seems like it would cover all the use cases and allow for easy local dev, without sauce integration.

[ljharb]

I'm definitely still having trouble getting a require in the tests (util/inspect) to work in phantom. Any tips?

[keithamus]

Ahh, I think this might be my bad advice.

The test runner depends on build/build.js - which is a built version of chai. You need to run make build every time you update Chai's source. So make sure you run make build if you haven't.

[ljharb]

OK - even when doing that though, I'm still getting this error output:

INFO [PhantomJS 1.9.8 (Mac OS X)]: Connected on socket 4qlUPMSd4qDdpAmYRu_d with id 50537547
PhantomJS 1.9.8 (Mac OS X) ERROR
  Error: failed to require "../lib/chai/utils"
  at /Users/ljharb/Dropbox/git/ljharb-chai.git/build/build.js:11

In both test/expect.js and test/should.js, I have var _ = require('../lib/chai/utils'); near the top of the describe.

[keithamus]

Ahh. The tests aren't built, and so require isn't available in the browser. The reason you're seeing that failure, is I believe because require is available in PhantomJS, specifically. Other browsers will likely fail saying undefined (require) is not a function

If you're using it to test that the error message is using _.inspect values, then I guess the easiest thing to do is just manually enter the strings - in other words, don't require lib utils. Make sense?

[ljharb]

That's what I did in the first place, but then that causes this very issue because object property order differs from engine to engine :-)

So hardcoding it is out, using require is out (so sad, that should work everywhere via browserify), which means the only other solution I can think of is to parse the string, and test for the first part of the error message, and then test the second part (the object) one property at a time. Very gnarly. Thoughts?

[keithamus]

We're not using browserify - although perhaps we should, but that's a separate discussion I guess.

The way I see it, right now we have two options:

1. Change the error tests to use a RegExp, something like:

   err(function(){
     var wrongDescriptor = {
       configurable: false,
       enumerable: true,
       writable: false,
       value: NaN
     };
     expect(obj).to.have.ownPropertyDescriptor('test', wrongDescriptor, 'blah');
   }, /^blah: expected the own property descriptor for 'test' on \{ test: NaN \} to match \{[^\}]*\}, got \{[^\}]*\}/);

   Not the best solution but certainly the quickest.

2. Make a PR that changes _.inspect to ensure it orders properties consistently across all browsers. This should also be quite easy in principle - but may cause a lot of test failures. If you want to go down this route here's some handy info:

   _.inspect discovers properties of an object using util/getProperties or util/getEnumerableProperties. Both of these return an array of keys. Simply return a sorted array instead, i.e. instead of return result just do return result.sort()

[keithamus]

@ljharb any progress with this?

[ljharb]

Not quite yet, but will take another look tonight.

[ljharb]

k, PR posted :-) I opted for the regex approach, since changing the key ordering I don't think is always desired. I also added a make task for testing on firefox.