Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

permutationOf is not reliable for arrays containing non-primitive values #38

Open
radekn opened this issue Jan 28, 2016 · 2 comments
Open

Comments

@radekn
Copy link

radekn commented Jan 28, 2016

Example case:

a = function () {}
b = function () {}

demand([a, b]).be.permutationOf([a, b]) // this passes
demand([b, a]).be.permutationOf([a, b]) // this fails

The second assertion fails with AssertionError: [null,null] must be a permutation of [null,null].
This problem is caused by array sort function converting values to strings by default and using them for deciding what goes first.

[rant]
Unfortunately, in JavaScript, for most types of values (like e.g. functions, objects, arrays, and recently symbols, maybe also regexps) there is no defined order. IMO, this is one of the reasons JS sucks, because it unnecessarily forces us into linear time complexity (or some dirty tricks), where otherwise logarithmic would be possible, or quadratic instead of linear. At least there is Map that helps alleviate the problem a bit.
[/rant]

@moll
Copy link
Owner

moll commented Jan 29, 2016

Hey again. You're quite right. Thanks for bringing this up.

I agree wrt to JavaScript, the language. So many abstractions lacking that have been around for decades in the programming language space.

Anywho, due to lack of stable sort order, I think the permutationOf matcher needs to be redesigned to use Map or any of its polyfills to handle the case you bring up. Do you know what some other assertion libs do to solve this? Do they even solve this?

@radekn
Copy link
Author

radekn commented Jan 29, 2016

It looks like none of the other popular libs has a permutationOf or equivalent matcher - I glanced over the API docs of Should.js, Expect and Chai. Even Lodash doesn't have a helper for this. Maybe this issue is part of the reason for that. Achieving the same effect with the other libs would probably require something like this:

expect(a.length).to.be(b.length)
b.forEach(v => expect(a).to.contain(v))

Its main drawback is quadratic time complexity, assuming that contain has linear. Using Map would allow to reduce it to O(n) or O(n log n), depending on its (Map's) implementation.

radekn added a commit to radekn/js-must that referenced this issue Jan 31, 2016
Fixes moll#38

This should make `permutationOf` work correctly for arrays containing values other than strings, numbers and booleans.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants