Skip to content

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

Merged
merged 1 commit into from
Oct 24, 2017
Merged

Conversation

cruxicheiros
Copy link
Contributor

@cruxicheiros cruxicheiros commented Oct 24, 2017

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).

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.
Copy link
Contributor

@stkent stkent left a 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!

Copy link
Member

@rpottsoh rpottsoh left a comment

Choose a reason for hiding this comment

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

Nice job! Thanks.

@rpottsoh rpottsoh merged commit 11fbf7e into exercism:master Oct 24, 2017
@petertseng
Copy link
Member

petertseng commented Oct 25, 2017

it's possible to pass all the unit tests by checking if (year % 100) == (year % 400)

I was unable to verify that this statement is true.

The first case is 2015.

2015 % 100 == 2015 % 400 is 15 == 15 is true, whereas the expected answer is false. So a solution that checks whether (year % 100) == (year % 400) will fail, as it should.

Did you mean to cite a different improperly-passing solution?

@cruxicheiros
Copy link
Contributor Author

cruxicheiros commented Oct 25, 2017

@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 (year % 4 == 0) && (year % 100 == year % 400)

@petertseng
Copy link
Member

So, the solution in question also checks year % 4 == 0 so as to be able to correctly reject 2015, which was not stated in the PR.

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.

@cruxicheiros
Copy link
Contributor Author

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.

@Insti
Copy link
Contributor

Insti commented Oct 25, 2017

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.

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.

6 participants