luhn: Add more tests by gibfahn · Pull Request #522 · exercism/problem-specifications

gibfahn · 2017-01-30T08:30:57Z

The luhn test suite didn't cover a lot of cases, including:

Strings of zeros
Non-alphabetic characters
Numbers which are only valid if you reverse the string

This updates the test suite to cover these.

Let me know if I'm doing anything wrong.

Insti · 2017-01-30T09:36:13Z

Your suggestions have caused me to identify something I think is an issue with this problem:#523
I don't think the string parsing tests belong in this problem.

(It's reasonable for you to have added more string parsing tests to given the current specification. ❤️ )

Insti · 2017-01-30T09:40:37Z

Strings of zeros

Why are strings of zeros invalid?

Numbers which are only valid if you reverse the string

What is this testing?

gibfahn · 2017-01-30T22:18:39Z

Why are strings of zeros invalid?

Looking at the README I somehow got the impression that they were invalid. I haven't found it clearly stated anywhere, but I'm willing to believe 0 is a valid answer.

Numbers which are only valid if you reverse the string

The formula works by reversing the sequence of numbers and going through them doubling every second digit. The current set of answers all pass even if you forget to reverse the sequence.

So I guess zero strings should pass. I'll update.

stevejb71 · 2017-01-31T01:13:49Z

@gibfahn "but I'm willing to believe 0 is a valid answer"

The readme states that strings of length 1 or less are invalid.

gibfahn · 2017-01-31T08:04:50Z

Why are strings of zeros invalid?
Looking at the README I somehow got the impression that they were invalid. I haven't found it clearly stated anywhere, but I'm willing to believe 0 is a valid answer.

The readme states that strings of length 1 or less are invalid.

0 as in the result of the formula is zero, talking about strings of zeros (i.e. 0000) where the calculation ends up being 0 mod 10 == 0.

Insti · 2017-01-31T09:18:08Z

Having tests for both valid and invalid zero strings would be useful.

000 valid
0000 invalid - missing check digit.

NobbZ · 2017-01-31T09:41:45Z

Why are 4 zeros missing the checkdigit? I take all but the last one, sum them up and get a sum of 0, now I substract 0 from 10 and get 10. 10 % 10 is 0, so my checkdigit is 0, which again is equal to what I took away before summing up. so 0000 is valid as well. Geoff Hubbard <notifications@github.com> schrieb am Di., 31. Jan. 2017 um 10:18 Uhr:

…

Having tests for both valid and invalid zero strings would be useful. 000 valid 0000 invalid - missing check digit. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#522 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADmR2yWKX5WTtgm_WXZnNGtE8ir7MT-ks5rXvxQgaJpZM4LxJNP> .

gibfahn · 2017-01-31T10:12:43Z

My (current) interpretation is that a single zero is invalid, but multiple zeros are always valid.

petertseng · 2017-02-04T10:41:01Z

+    {
+      "description": "lots of zeros are invalid",
+      "input": " 00000",
+      "expected": false


don't forget to change this one (I think we want this one to be valid, right?)

petertseng · 2017-02-04T10:52:10Z

+      "expected": true
+    },
+    {
+      "description": "simple valid sin",


probably worth moving this up such that it becomes the first true case - let the student solve the simplest case first.

also another note that this is another case that will reveal doubling from the wrong side - if doubling the wrong digits here we will get 14 instead of 10 and falsely think this is invalid.

This could possibly make the "must double from right" case redundant.

Makes sense, I've moved this up and removed the "must double from right" case

The luhn test suite didn't cover a lot of cases, including: - Strings of zeros - Non-alphabetic characters - Numbers which are only valid if you reverse the string

petertseng

These cases seem suitable for inclusion. I have one question about what the "another valid sin" is doing.

It's acknowledged by me that some of the added cases will be removed if #523 decides in favour of no validation, but that is an orthogonal concern to this PR. My approval on this PR is not meant to be construed as a vote in either direction on #523.

petertseng · 2017-02-04T19:20:54Z

+      "expected": true
+    },
+    {
+      "description": "nine doubled is nine",


in case anyone wonder, this caught a bug in mine (I was using mod 9 after doubling, which turns nine into zero instead of nine)

petertseng · 2017-02-04T19:21:44Z

+    },
+    {
+      "description": "another valid sin",
+      "input": "055 444 285",


can you confirm - what does this test that above cases do not? is it just here to contrast with 055-444-285? What is an example of an implementation that would pass the above cases but fail this one?

Part of it was the contrast with 0555-444-285, and part of it was the thought that having a couple of different longer valid numbers might help someone who was struggling to work out how the algorithm needed to be implemented.

I can remove it if necessary.

petertseng · 2017-09-17T00:59:37Z

+    },
+    {
+      "description": "symbols are not allowed",
+      "input": "055£ 444$ 285",


can I get a confirmation on whether the £ is in ASCII? I am seeing bytes 0xc2 0xa3 for it

* bin/generator: Big rewrite Can now generate tests without updating the test version number. New command line options: ``` Usage: bin/generate [options] exercise-generator -f, --freeze Don't update test version -h, --help Prints this help -v, --verbose Display progress messages ``` `sha1` in the "Common test data version: <%= sha1 %>" comment is now the commit ID of the relevant canonical-data.json file Split code up into classes. 100% test coverage. Added dependency: `require_all` gem Just requiring files in (potentially random) directory order was not sufficient to satisfy all class dependencies. The `require_all` gem ensures dependencies are met. * Rename 'new_content' to 'content' * Rename `isogram` to `beta` There's nothing special about isogram as a name, and `beta` continues the greek-letter fixture exercise naming pattern. * Rename `save_if_changed` to `save` * Use << rather than + for string concatenation * Remove redundant comment

behrtam mentioned this pull request Jan 30, 2017

luhn: Update tests and example exercism/python#409

Merged

petertseng mentioned this pull request Jan 31, 2017

Changes to Luhn tests exercism/haskell#469

Closed

robphoenix mentioned this pull request Feb 1, 2017

luhn: update test files exercism/go#479

Merged

petertseng reviewed Feb 4, 2017

View reviewed changes

luhn: Add more tests

0a4f44a

The luhn test suite didn't cover a lot of cases, including: - Strings of zeros - Non-alphabetic characters - Numbers which are only valid if you reverse the string

gibfahn force-pushed the update-luhn branch from b41b24b to 0a4f44a Compare February 4, 2017 18:45

petertseng approved these changes Feb 4, 2017

View reviewed changes

m-dango mentioned this pull request Feb 6, 2017

port exercise 'luhn' to Perl 6 exercism/raku#80

Merged

petertseng merged commit 11d18a1 into exercism:master Feb 8, 2017

petertseng mentioned this pull request Feb 8, 2017

Luhn tests missing coverage for common mistake (right vs left) exercism/go#500

Closed

gibfahn mentioned this pull request Feb 9, 2017

luhn: Update example and add more tests exercism/rust#253

Merged

petertseng mentioned this pull request May 9, 2017

luhn 1.0.0.2: only test isValid, use (most) x-common cases exercism/haskell#533

Merged

petertseng reviewed Sep 17, 2017

View reviewed changes

gibfahn deleted the update-luhn branch September 17, 2017 12:10

Uh oh!

Conversation

gibfahn commented Jan 30, 2017

Uh oh!

Insti commented Jan 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Insti commented Jan 30, 2017

Uh oh!

gibfahn commented Jan 30, 2017

Uh oh!

stevejb71 commented Jan 31, 2017

Uh oh!

gibfahn commented Jan 31, 2017

Uh oh!

Insti commented Jan 31, 2017

Uh oh!

NobbZ commented Jan 31, 2017 via email

Uh oh!

gibfahn commented Jan 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petertseng left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Insti commented Jan 30, 2017 •

edited

Loading

petertseng left a comment •

edited

Loading