Skip to content

leap: add more test cases #622

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
Aug 28, 2018
Merged

leap: add more test cases #622

merged 1 commit into from
Aug 28, 2018

Conversation

Emerentius
Copy link
Contributor

@Emerentius Emerentius commented Aug 14, 2018

The exercise has just 5 test cases. This can easily let some bugs slip through.
This adds a check for a range of years (1600 to 1699) and a few additional centuries. The range is kept short so the user isn't spammed with a huge list of incorrect years with wrong code. It also avoids having the full solution in the tests where students could stumble upon it.

I got this faulty code from a student which passes all the old tests:

year % 4 == 0 && !year % 100 == 0 || year % 400 == 0

just 5 cases are likely to let some bugs slip through
added a check for a range of years (1600 to 1699) and
some additional centuries
Copy link
Member

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

Looks great! I like the way you implemented the procedural test; it's correct without giving away a complete implementation.

As with your other PRs, the soonest I can merge this is Monday the 27th, but I'm happy if another maintainer merges this before then.

@Emerentius
Copy link
Contributor Author

I thought on this a bit more. I found it puzzling that bitwise inversion would actually make it work.
-n = !n + 1 (two's complement) or put another way, !n = -n - 1. It just shifts the centuries by 1 and in that case the first modulo 4 check never passes.

So why did I think it passes all the tests? Because all the centuries tests check for false which this happens to return and cargo test -- --ignored doesn't run all tests, only the ignored ones. The very first test would have detected this.

We can ignore this PR but we should get a way to just run all tests in one command.

@petertseng
Copy link
Member

For this reason, I have always been forced to run cargo test && cargo test -- --ignored. I continue to look for any better solutions. Let me know of any.

@Emerentius
Copy link
Contributor Author

I found this relevant issue: rust-lang/rust#50363
A quick search through the source of libtest makes me believe that adding an --all flag shouldn't be difficult. Compiling rustc will be the biggest hurdle for me, but I'll try to tackle this.

@coriolinus coriolinus merged commit 839d936 into exercism:master Aug 28, 2018
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.

3 participants