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

Pass comparison callback to sort radix-sort.test.js #29

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Mindgames55
Copy link

unit tests bug
unit tests rely on Array.sort . However, there is no comparison function which will sort the array as they were strings.

Not sure why tests passed in the case of the solution file. Sandbox is not very good for debugging but test should also fail in there bc of the bug in the test not the algo

**unit tests bug**
unit tests rely on `Array.sort` . However, there is no comparison function which will sort the array as they were strings. 

Not sure why tests passed in the case of the solution file. Sandbox is not very good for debugging but test should also fail in there bc of the bug in the test not the algo
@SeanDemps
Copy link

I noticed that the solution tests are actually skipped by default... however they do pass (incorrectly). The reason they pass is that the unit test is written in a way that the expectation and the result are the same array object and so you're doing a comparison on itself.

Copying the array and sorting it separately from the input passed to radix-sort demonstrates this if you modify the test to be:

it("should sort 99 random numbers correctly", () => { const fill = 99; const nums = new Array(fill) .fill() .map(() => Math.floor(Math.random() * 500000)); const res = [...nums]; const ans = radixSort(nums); expect(ans).toEqual(res.sort()); });

the test will not pass. (i.e your commit is necessary to fix the unit test, but also the test is written in a way that means nothing if you write your sorting function in a specific way)

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.

2 participants