Skip to content

Replace test case - "invalid character in isbn". #1217

Merged
petertseng merged 2 commits intoexercism:masterfrom
dnicolaev:patch-1
Mar 30, 2018
Merged

Replace test case - "invalid character in isbn". #1217
petertseng merged 2 commits intoexercism:masterfrom
dnicolaev:patch-1

Conversation

@dnicolaev
Copy link
Copy Markdown
Contributor

@dnicolaev dnicolaev commented Mar 30, 2018

Set new test case to be 3-598-P1581-X.
It should avoid solutions to pass - when inside characters (P) is treat as 0.
3 * 10 + 5 * 9 + 9 * 8 + 8 * 7 + 0 * 6 + 1 * 5 + 5 * 4 + 8 * 3 + 1 * 2 + 10 * 1 = 264
264 is divisible by 11.

Closes #1212

Set new test case to be 3-598-P1581-X.
It should avoid solutions to pass - when inside characters (P) is treat as 0.
3 * 10 + 5 * 9 + 9 * 8 + 8 * 7 + 0 * 6 + 1 * 5 + 5 * 4 + 8 * 3 + 1 * 2 + 10 * 1 = 264
264 is divisible by 11.
Copy link
Copy Markdown
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

Thank you. Good change.

Before this can be Approved, please do two things:

@dnicolaev dnicolaev changed the title Replace test case - "invalid character in isbn" Replace test case - "invalid character in isbn". Closes #1212 Mar 30, 2018
@dnicolaev dnicolaev changed the title Replace test case - "invalid character in isbn". Closes #1212 Replace test case - "invalid character in isbn". Mar 30, 2018
@dnicolaev
Copy link
Copy Markdown
Contributor Author

@petertseng thanks for pointing me to what should be done.
Updated version number and added Closes keywords in PR description.

Copy link
Copy Markdown
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

I verified: the incorrect implementation https://github.com/petertseng/exercism-problem-specifications/blob/verify/exercises/isbn-verifier/verify.rb#L76-L82 now incorrectly accepts this case (a correct impl must reject), therefore this test case is now useful.

Given that:

  • there is automated verification
  • we had agreement in the linked issue that this is a useful case to add. The discussion has been open for 48 hours.
  • it's my good-faith belief that the replaced test case added no value; it has been present without additional comment in #924

My judgment is that there is no further need to wait; let's merge it now

@petertseng petertseng merged commit 0c67742 into exercism:master Mar 30, 2018
@petertseng
Copy link
Copy Markdown
Member

Great job, thank you.

petertseng added a commit to exercism/haskell that referenced this pull request Mar 31, 2018
…st (#670)

Set new test case to be 3-598-P1581-X.
This will actually catch incorrect solutions - when invalid characters (P) are treated as 0.
3 * 10 + 5 * 9 + 9 * 8 + 8 * 7 + 0 * 6 + 1 * 5 + 5 * 4 + 8 * 3 + 1 * 2 + 10 * 1 = 264
264 is divisible by 11.

The previous case did NOT have this property:
3 * 10 + 5 * 9 + 9 * 8 + 8 * 7 + 2 * 6 + 0 * 5 + 5 * 4 + 0 * 3 + 7 * 2 + 0 * 1 = 249
249 is NOT divisible by 11.

exercism/problem-specifications#1212
exercism/problem-specifications#1217
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