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

Fix test name for binary search exercise #2883

Merged

Conversation

akmaekki
Copy link
Contributor

@akmaekki akmaekki commented Dec 28, 2024

2881

In JAVA track improve test names for binary search exercise.

Fixes #2881

Reviewer Resources:

Track Policies

Copy link
Contributor

This PR touches files which potentially affect the outcome of the tests of an exercise. This will cause all students' solutions to affected exercises to be re-tested.

If this PR does not affect the result of the test (or, for example, adds an edge case that is not worth rerunning all tests for), please add the following to the merge-commit message which will stops student's tests from re-running. Please copy-paste to avoid typos.

[no important files changed]

For more information, refer to the documentation. If you are unsure whether to add the message or not, please ping @exercism/maintainers-admin in a comment. Thank you!

@akmaekki akmaekki marked this pull request as ready for review December 28, 2024 16:03
@@ -119,7 +119,7 @@ public void nothingIsFoundInAnEmptyArray() throws ValueNotFoundException {
@Disabled("Remove to run test")
@Test
public void nothingIsFoundWhenTheLeftAndRightBoundCross() throws ValueNotFoundException {
List<Integer> sortedList = List.of(1, 2);
List<Integer> sortedList = List.of(2, 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can revert this again, but the test name suggests to me that right boundary (maximum) is less than the left boundary (minimum).

Copy link
Member

@kahgoh kahgoh Dec 28, 2024

Choose a reason for hiding this comment

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

Hey @akmaekki, the data for the tests comes from the problem specifications. As far as I can tell, the List.of(1, 2) matches the problem specification and so does the name. I think the name may be referring to way binary search works - left and right starts at the end and beginning of the list. At each step, compare the value you're looking for and update the "left" and "right" indexes depending on the comparison result. Repeat. If the value isn't in the list, the "left" and "right" will eventually cross over. Although, I'm little unsure why the value 0 is chosen as the value to find - it looks like the same case as aValueSmallerThanTheArraysSmallestValueIsNotFound.

If you feel there is still something wrong with the data for this test and would like to change, could you please open a discussion in the forums? Generally, we should fix the data in the problem specifications so that it can eventually be fixed in all tracks with this exercise, but we usually discuss changes to the problem specifications in the forums first.

The change to the other test method is fine because the change still matches the problem specifications. For this PR, I'd suggest backing this out for now to unblock it. If we agree to fix it in the forums, we can also do another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @kahgoh
Thanks for the elaborate discussion and tip with the forum. I am fine with backing this out. Will remove the changes done on nothingIsFoundWhenTheLeftAndRightBoundCross and refresh this PR.

@kahgoh kahgoh changed the title 2881 - improve test names for binary search exercise Fix test name for binary search exercise Dec 29, 2024
Copy link
Member

@kahgoh kahgoh left a comment

Choose a reason for hiding this comment

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

This looks good now! Thanks!

@kahgoh kahgoh merged commit a73fb98 into exercism:main Dec 29, 2024
4 checks passed
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.

Binary Search Exercise: Fix test name
2 participants