Skip to content

Fix test name for binary search exercise #2883

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Dec 29, 2024
Merged

Fix test name for binary search exercise #2883

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1da90cd
improve test names for binary search exercise
akmaekki Dec 28, 2024
d482df5
revert one unjustified change
akmaekki Dec 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions exercises/practice/binary-search/src/test/java/BinarySearchTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ public void aValueSmallerThanTheArraysSmallestValueIsNotFound() {

@Disabled("Remove to run test")
@Test
public void aValueLargerThanTheArraysSmallestValueIsNotFound() throws ValueNotFoundException {
public void aValueLargerThanTheArraysLargestValueIsNotFound() throws ValueNotFoundException {
List<Integer> sortedList = List.of(1, 3, 4, 6, 8, 9, 11);

BinarySearch search = new BinarySearch(sortedList);
Expand All @@ -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 •
edited
Loading

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.


BinarySearch search = new BinarySearch(sortedList);

Expand Down