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

Make Array shuffle test pass in Node >= 11.0.0 #740

Merged
merged 4 commits into from
Jun 8, 2020

Conversation

Nevon
Copy link
Collaborator

@Nevon Nevon commented Jun 3, 2020

Due to a change in how array sorting works, the shuffle test would fail in Node >= 11.0.0.

 FAIL  src/utils/shuffle.spec.js
  ● Utils > shuffle › shuffles

    expect(received).toEqual(expected) // deep equality

    - Expected  - 2
    + Received  + 2

      Array [
    -   1,
    -   3,
        2,
    +   3,
    +   1,
      ]

      13 |       .mockImplementationOnce(() => 0.6)
      14 |
    > 15 |     expect(shuffle(array)).toEqual([1, 3, 2])
         |                            ^
      16 |     expect(shuffle(array)).toEqual([3, 2, 1])
      17 |     expect(Math.random).toHaveBeenCalledTimes(6)
      18 |   })

      at Object.<anonymous> (src/utils/shuffle.spec.js:15:28)

Instead of relying on implementation details of how the engine implements sorting, we'll add a requirement on the shuffle implementation to always return a different order than the input array. That way we can simply assert that the returned array is different from the input, rather than asserting on the exact order, which may differ between engine versions since it's explicitly undefined in the spec.

The updated test will pass in both Node 10 and 11+.

Due to a change in how array sorting works, the shuffle test would fail in Node >= 11.0.0.
Instead of relying on implementation details of how the engine implements sorting, we'll
add a requirement on the shuffle implementation to always return a different order than
the input array. That way we can simply assert that the returned array is different
from the input, rather than asserting on the exact order, which may differ between engine versions.
@Nevon Nevon requested review from tulios and JaapRood June 3, 2020 13:38
src/utils/shuffle.js Outdated Show resolved Hide resolved
@ankon
Copy link
Contributor

ankon commented Jun 3, 2020

add a requirement on the shuffle implementation to always return a different order than the input array.

This requirement can't be upheld with a single element array, and I'm also wondering: Is it needed? The famous enigma-encryption-bug comes to mind -- just because the elements are in the end in the same order doesn't mean that that order wasn't randomly chosen.

@Nevon
Copy link
Collaborator Author

Nevon commented Jun 3, 2020

Yes, that's why I have a special case for single element lists (or empty lists).

Originally I had the same thought as you did, regarding why not to return the same order for the output as input, which is why the original implementation worked the way it did (completely randomly). But then I thought about it for a bit and realized that as a user, that's unintuitive behavior (think about how no music player's shuffle plays the same song twice in a row).

Now, however, I'm coming back around to your way of thinking about it, since our use case is mostly to shuffle the order of the seed brokers, in which case we actually do want the original order to appear sometimes. So I think I'm gonna go back to the old way, and instead write the test to run the shuffle up to N times and make sure that we get at least one result where the order is different from the input. Granted, this will cause the test to be flaky, but I can give it a long list and have a relatively high max iterations so that in practice it will always pass.

Replace slow naive implementation with Durstenfeld Shuffle, to allow us to
run the test against a larger array.
@Nevon
Copy link
Collaborator Author

Nevon commented Jun 3, 2020

Bumping up the number of elements in the array revealed how slow my original, naive implementation of the shuffle was, so I replaced it with the Durstenfeld implementation of the Fisher-Yates Shuffle, which is a lot faster (and has a less biased distribution, as a side bonus).

@Nevon Nevon merged commit bd5cf4c into master Jun 8, 2020
@Nevon Nevon deleted the fix-array-shuffle-in-node-11 branch June 8, 2020 10:54
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

Successfully merging this pull request may close these issues.

None yet

3 participants