-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
test: add mixed number types for LocalMaximomPoint tests #1668
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think these help coverage since the algorithm is independent of the specific numbers and just cares about the ordering.
If we're seeing this as contract-based testing, I also consider it a bit dubious that you expect a specific maximum when there are multiple valid return values (as is the case in your first test).
All in all, I'm afraid I wouldn't support this PR - I don't see the benefit it brings.
ps. you also seem to have unrelated changes in here, though that would be easy to fix.
@appgurueu Thank you for your feedback. Based on your insights, I have adjusted the test cases to align with the behavior of the existing algorithm.
I recognize the limitation of the original algorithm in not always returning the first local maximum. Your feedback on further improvements is welcomed.
Open in Gitpod know more
Describe your change:
Checklist:
Example:
UserProfile.jsis allowed butuserprofile.js,Userprofile.js,user-Profile.js,userProfile.jsare notFixes: #{$ISSUE_NO}.