Skip to content
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

Luhn: Update tests to 1.7.0 (1 changed tests, 2 new tests) #900

Merged
merged 1 commit into from
Mar 25, 2020

Conversation

tejasbubane
Copy link
Member

@tejasbubane tejasbubane commented Mar 14, 2020

Changes included:

  1. Luhn: Update testcase to avoid wrong solution  problem-specifications#1480: 1.4.0 to 1.5.0:
    Change "055a 444 285" test into "055b 444 285" for subtle reasons.
    New test name: using ascii value for doubled non-digit isn't allowed

  2. Luhn: Add a test with an odd number of spaces problem-specifications#1500: 1.5.0 to 1.6.0:
    Add test case: valid number with an odd number of spaces

  3. luhn: check a number with an even remainder problem-specifications#1635: 1.6.0 to 1.7.0:
    Add test case: invalid long number with an even remainder

(Edit by @sshine: Added short description of the addressed commits.)

Copy link
Contributor

@sshine sshine left a comment

Choose a reason for hiding this comment

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

Thanks for your changes!

I found it difficult to decipher what test change was associated with what version change, since they're all in one commit. I didn't want to ask you to split them up before I knew how difficult that might be. A finer granularity comes at the cost of time spent now, but makes both reading git blame later and reviewing now much easier.

exercism/problem-specifictions#1480 renames changes a test, "valid strings with a non-digit included become invalid" into "using ascii value for non-doubled non-digit isn't allowed", even though it might look like one test was removed and another one was added.

The other two are simple test additions.

I'm approving this and will squash merge a short description of each canonical version bump to each mention in the PR and commit message so that future inspection is easier. Separate commits per version bump would also have worked.

Comment on lines 63 to 64
-- This track is not testing these cases, since we would rather focus on the algorithm,
-- and because it seems strange to be unable to distinguish between well-formed invalid input and malformed input.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm remarking that the comment above was left by 610ba69.

@sshine sshine changed the title Update luhn exercise to 1.7.0 Luhn: Update tests to 1.7.0 (1 changed tests, 2 new tests) Mar 25, 2020
@sshine sshine merged commit a1bf7e3 into exercism:master Mar 25, 2020
@tejasbubane tejasbubane deleted the luhn-1-7-0 branch March 25, 2020 14:36
@tejasbubane
Copy link
Member Author

A finer granularity comes at the cost of time spent now, but makes both reading git blame later and reviewing now much easier.

I'll be mindful of this in future. Thanks! 👍

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