-
-
Notifications
You must be signed in to change notification settings - Fork 554
leap: Fix loophole in unit tests #971
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
Currently, it's possible to pass all the unit tests by checking if `(year % 100) == (year % 400)`. This commit changes one of the test cases so that this is no longer possible. Note that this is a separate issue to exercism#955 -- however, 1996, like 2020, is not divisible by 16, so that loophole will remain closed.
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.
Nice catch, thanks for the PR!
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.
Nice job! Thanks.
I was unable to verify that this statement is true. The first case is 2015.
Did you mean to cite a different improperly-passing solution? |
@petertseng PR #979 in exercism/python. Sorry for any confusion. What I meant to say was that it was possible to pass the unit test by checking |
So, the solution in question also checks This means that the commit message was incorrect. We all need to be more careful that commit messages are correct. I considered rewriting master to amend the commit message, but I suppose it's too discourteous to ask everyone who already acquired that commit to reset it away. I considered recommending that the commit be reverted, then reapplied with a correct message, but perhaps as long as GitHub continues to exist, this written record is sufficient, but now needs an internet connection to be able to see it. |
I've edited the body of the pull request so that it contains the correct information for anyone who checks on GitHub. In future PRs I will triple-check that what I am writing is correct. Again, I apologise. |
❤️ We all miss things sometimes. @stkent, @cmccandless and @rpottsoh also missed this. It's the responsibility of the reviewers to help catch these issues so they can be fixed before the pull request is merged. That's why we have reviews. |
1.1.0: exercism/problem-specifications#955 1.2.0: `y % 4 == 0 && y % 100 == y % 400` exercism/problem-specifications#971
Currently, it's possible to pass all the unit tests by checking if
(year % 4 == 0) && (year % 100 == year % 400)
.This commit changes one of the test cases so that this is no longer possible. Note that this is a separate issue to #955 -- however, 1996, like 2020, is not divisible by 16, so that loophole will remain closed.
EDIT
This post formerly stated, incorrectly, that it was possible to pass the unit tests by checking
(year % 100) == (year % 400)
.