Skip to content

Swap '2016' for '1996' to stop faulty logic from passing unit test in Leap #979

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 2 commits into from
Oct 24, 2017

Conversation

cruxicheiros
Copy link
Contributor

@cruxicheiros cruxicheiros commented Oct 24, 2017

This Leap solution should not pass the unit test, since it checks whether year % 100 and year % 400 are equal in order to tell whether the year it has been passed is a leap year. This works for 2016 because 2016 % 100 and 2016 % 400 both evaluate to 16.

1996, another leap year that is divisible by 4 but not 100, does not have this property and, under that solution, would be falsely not classed as a leap year.

[This leap solution](http://exercism.io/submissions/7ef5b0ab93b540f79cdee9f153e0a21c) should not pass the unit test, since it checks whether `year % 100` and `year % 400` are equal in order to tell whether the year it has been passed is a leap year. This works for 2016 because `2016 % 100` and `2016 % 400` both evaluate to 16.

1996, another leap year that is divisible by 4 but not 100, does not have this property and, under that solution, would falsely not be classed as a leap year.
@cruxicheiros cruxicheiros changed the title Swap '2016' for '1996' to stop faulty logic from passing unit test Swap '2016' for '1996' to stop faulty logic from passing unit test in Leap Oct 24, 2017
@cmccandless
Copy link
Contributor

@cruxicheiros Thanks for the pull request! Good catch; turns out this issue was already addressed recently by updating the canonical data, but this track had not been updated yet. If you can modify your changes to conform to the new version of the canonical data, we should be good to go!

@cruxicheiros
Copy link
Contributor Author

cruxicheiros commented Oct 24, 2017

@cmccandless This is actually a different corner case. The one the canonical data has been updated to fix is one where it's possible to pass all the tests by checking if the year is evenly divisible by 16. With this one, it's possible to pass all the unit tests by checking if (year % 100) == (year % 400) - something which is still true with 2020.

Coincidentally, 16 is also not a factor of 1996, so changing the value in the canonical data from 2020 to 1996 would address both issues.

Edit: I have submitted a PR to the problem-specifications repo.

@cmccandless
Copy link
Contributor

@cruxicheiros Yes, submitting a PR to problem-specifications was the correct action. Thanks! 😄

Now that that PR has been merged, please update the rest of leap_test.py to conform to new canonical-data version, and we should be good to go!

@cruxicheiros
Copy link
Contributor Author

@cmccandless Updated the version number. I think that's it done.

@cmccandless cmccandless merged commit 9b1a59a into exercism:master Oct 24, 2017
@cmccandless
Copy link
Contributor

@cruxicheiros Merged! Congrats and thanks for working on this!

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.

2 participants