Arrays passed to `.keys()` assertions are sorted (unexpectedly) by side-effect #359

Merged
merged 1 commit into from Feb 6, 2015

Projects

None yet

2 participants

@gregglind
Contributor

Example

var chai = require("chai");

var orig = ["b", "a"];
var mykeys = ["b","a"];
var O = {
  "b": 1,
  "a": 2
};

chai.expect(mykeys).deep.equal(orig);
chai.expect(O).keys(mykeys);
chai.expect(mykeys).deep.equal(orig);
// AssertionError: expected [ 'a', 'b' ] to deeply equal [ 'b', 'a' ]

This is caused by the .sort() at https://github.com/chaijs/chai/blob/master/lib/chai/core/assertions.js#L1070

Possible Repairs

  1. remove the sort()
  2. clone / copy the expected list earlier in the function.
  3. Something else

Other implications

  • unknown, thus my ask :) I don't know Chai's code base and there may be other sort issues. I didn't see any.

Action Request

If this sounds good, I can write up a PR tomorrow with whichever solution makes the most sense.

@gregglind gregglind added a commit to gregglind/chai that referenced this pull request Feb 4, 2015
@gregglind gregglind Fix #359 keys() sorts input unexpectedly c0f7b76
@gregglind gregglind added a commit to gregglind/chai that referenced this pull request Feb 4, 2015
@gregglind gregglind Fix #359 keys() sorts input unexpectedly ed77323
@gregglind
Contributor

(updated PR for style. Hard to find prior art on anons/closures in the tests!)

@keithamus
Member

Fantastic issue @gregglind! I wish every issue was as detailed as this 😄

@keithamus keithamus and 1 other commented on an outdated diff Feb 5, 2015
test/expect.js
@@ -602,6 +602,16 @@ describe('expect', function () {
expect({ foo: 1, bar: 2 }).to.not.have.all.keys(['baz', 'foo']);
expect({ foo: 1, bar: 2 }).to.not.contain.all.keys(['baz', 'foo']);
+ // issues/359
+ (function fix359() {
+ var expected = [ 'b', 'a' ];
+ var backup = expected.slice(0);
+ var obj = { "b": 1, "a" :1 };
+ expect(expected).deep.equal(backup);
+ expect(obj).keys(expected);
+ expect(expected).deep.equal(backup);
+ })()
+
@keithamus
keithamus Feb 5, 2015 Member

Stylistically I think these can just be inlined - I dont think there is a need for a closure. If you want to you could create a new it() test instead.

@gregglind
gregglind Feb 5, 2015 Contributor
  1. Thanks for the compliments! Chai is super useful on my most recent project at Mozilla.
  2. Closure it because
  • it seemed a little gross no matter where I put it.
  • I don't want to leak any of the vars out, and want to
  • make it clear it's part of keys(array) testing.
  1. Nested 'it' isn't called, afaict (so sad!), so tried this:
  it.only("keys(array), array should keep original order (#359)", function () {
    var expected = [ 'b', "a" ];
    var backup = expected.slice(0);
    var obj = { "b": 1, "a" :1 };
    expect(expected).deep.equal(backup);
    expect(obj).keys(expected);
    expect(expected).deep.equal(backup);
  });

It's clearer, but less elegant.

As an inline alternative.

    var issue359 = {
      original:  ['b', 'a'],
      expected: [ 'b', "a" ],
      obj: { "b": 1, "a" : 1 };
    }

    expect(issue359.expected).deep.equal(issue359.original);
    expect(issue359.obj).keys(issue359.expected);
    expect(issue359.expected, "keys should maintain original order").deep.equal(issue359.backup);

Or

    var keys_maintain_original_order_issue359 = function () {
      var expected = [ 'b', 'a' ];
      var original_order = expected.slice(0);
      var obj = { "b": 1, "a" :1 };
      expect(expected).deep.equal(original_order);
      expect(obj).keys(original_order);
      expect(expected, "sorting happened unexpectedly in keys()")
        .deep.equal(original_order);
    }
    keys_maintain_original_order_issue359();

Anyway, this is a weird case. I think I lean toward the last style. And this is probably more time on this than is really needed :) (But fun to consider alternatives!)

(pull updated to reflect last)

@gregglind gregglind added a commit to gregglind/chai that referenced this pull request Feb 5, 2015
@gregglind gregglind Fix #359 keys() sorts input unexpectedly d26d39f
@gregglind
Contributor

(updated patch)

@keithamus keithamus commented on an outdated diff Feb 5, 2015
test/expect.js
@@ -602,6 +602,18 @@ describe('expect', function () {
expect({ foo: 1, bar: 2 }).to.not.have.all.keys(['baz', 'foo']);
expect({ foo: 1, bar: 2 }).to.not.contain.all.keys(['baz', 'foo']);
+
+ var keys_maintain_original_order_issue359 = function () {
+ var expected = [ 'b', 'a' ];
+ var original_order = [ 'b', 'a' ];
+ var obj = { "b": 1, "a": 1 };
+ expect(expected).deep.equal(original_order);
+ expect(obj).keys(original_order);
+ expect(expected, "sorting happened unexpectedly in keys()")
+ .deep.equal(original_order);
+ }
+ keys_maintain_original_order_issue359();
+
@keithamus
keithamus Feb 5, 2015 Member

Sorry @gregglind, I don't think I made myself clear in the last message. There is no need for these assertions to be made inside a new function scope at all. Line 606, 614 and 615 can be completely removed, so as to keep the code inline with the other assertions - at the same indentation level. Alternatively if you feel like separating the test, it should go in a new it() e.g.

it('keys(array) does not mutate array'. function () {
...
})

If you do a new it() test, it will need to be on the same indentation level as the other it()s.

@gregglind
Contributor

All right, went with new it test. Coverage is still 100%, all tests pass.

@keithamus
Member

Thanks so much @gregglind for sticking with it. Code is perfect now, fantastic job 😄

@keithamus keithamus merged commit bd72280 into chaijs:master Feb 6, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@gregglind gregglind added a commit to gregglind/chai that referenced this pull request Feb 7, 2015
@gregglind gregglind WIP #359. Questions inside
- *before* didn't throw on too many args for Array.  Claim in bug to do so for Object. So throw on both?  Neither?
d1d863f
@gregglind gregglind added a commit to gregglind/chai that referenced this pull request Feb 7, 2015
@gregglind gregglind WIP re #359. Questions inside
- *before* didn't throw on too many args for Array.  Claim in bug to do so for Object. So throw on both?  Neither?
4db3314
@gregglind gregglind added a commit to gregglind/chai that referenced this pull request Feb 8, 2015
@gregglind gregglind fix #359. Add `.keys(object)
- `.keys(object)n => .keys(Object.keys(Object)`
- added exceptions for 'if first arg is non-string, then it must be only
arg.  => `.keys(Array|Object, ...)`

Warning:  `Object.keys` must exist on systems to use this functionality.
0baa6f1
@gregglind gregglind added a commit to gregglind/chai that referenced this pull request Feb 8, 2015
@gregglind gregglind fix #359. Add `.keys(object)
- `.keys(object)n => .keys(Object.keys(Object)`
- added exceptions for 'if first arg is non-string, then it must be only
arg.  => `.keys(Array|Object, ...)`

Warning:  `Object.keys` must exist on systems to use this functionality.
2b46e95
@gregglind gregglind added a commit to gregglind/chai that referenced this pull request Feb 9, 2015
@gregglind gregglind fix #359. Add `.keys(object)
- `.keys(object)n => .keys(Object.keys(Object)`
- added exceptions for 'if first arg is non-string, then it must be only
arg.  => `.keys(Array|Object, ...)`

Warning:  `Object.keys` must exist on systems to use this functionality.
4f40b37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment