Skip to content

Fix sort testcase input sorted unintentionally#256

Closed
vzvu3k6k wants to merge 5 commits into
TheAlgorithms:masterfrom
vzvu3k6k:fix-sort-tests
Closed

Fix sort testcase input sorted unintentionally#256
vzvu3k6k wants to merge 5 commits into
TheAlgorithms:masterfrom
vzvu3k6k:fix-sort-tests

Conversation

@vzvu3k6k

Copy link
Copy Markdown

As getSortedVersion and other sorting functions modify a given slice, testcase inputs are unintentionally sorted beforehand.

To avoid that, passes a cloned slices to these functions.

A slice is a references to its underlying array. If a slice is
passed as an argument and modified in a function, the original
slice is also modified.

As getSortedVersion and other sorting functions modify a given slice,
testcase inputs are unintentionally sorted beforehand.

To avoid that, passes a cloned slices to these functions.

@tjgurwara99 tjgurwara99 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All seems good to me. Maybe add a comment on top of getSortedVersion and cloneIntSlice to make it easier to understand what is happening, even though it's mostly self explanatory.

@vzvu3k6k vzvu3k6k marked this pull request as draft February 27, 2021 18:56
This timeout is caused by fixing an issue that slices of testcases were
unintentionally being sorted.

If array is sorted, the complexities of bubble sort and insertion sort
are O(n), but in general they are O(n^2) and 500k is large enough to
cause timeout.
@vzvu3k6k

Copy link
Copy Markdown
Author

@tjgurwara99
Thanks for review. I've added a comment for cloneIntSlice at 651bbbc, but can't think of a good comment for getSortedVersion. If you have any good ideas, please let me know.

Now that go test runs on CI thanks to #266, I've reduced the slice length of testcases (at 0bbc1db) to avoid timeout on GitHub Actions (happened on https://github.com/TheAlgorithms/Go/runs/1995000745). It would be great if you could review this change as well.

@vzvu3k6k vzvu3k6k marked this pull request as ready for review February 27, 2021 19:17

@tjgurwara99 tjgurwara99 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work catching that unintentional sorting!

Comment thread sorts/sorts_testcases.go
Co-authored-by: Taj <tjgurwara99@users.noreply.github.com>
@tjgurwara99

tjgurwara99 commented Sep 10, 2021

Copy link
Copy Markdown
Member

This issue has been resolved recently when I was restructuring the repo - I completely forgot that there was a PR open on that exact same issue of test cases being sorted resulting in false passes. If I remembered I would have merged your PR but since this has been resolved, I am closing this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants