isbn-verifier: missing test case for non-digit character in the middle

[kytrinyx]

@dnicolaev reported in https://github.com/exercism/exercism.io/issues/3747

---

Hello, I worked on ISBN verifier in Python - my solutions passed all the tests. However there is one case that is not covered - it is with a non digit character in the middle. And the solution I created - will pass this test case also. But it should not. 3598P1581X

[rpottsoh]

test data: https://github.com/exercism/problem-specifications/blob/master/exercises/isbn-verifier/canonical-data.json

description: https://github.com/exercism/problem-specifications/blob/master/exercises/isbn-verifier/description.md

@dnicolaev this is a pretty new exercise for Exercism. Thanks for bringing this potential edge case to light. I have included links to the test description and the test data to make it easier for others that may be interested in investigating this issue to find the documentation that defines this exercise.

[rpottsoh]

@dnicolaev I just want to confirm that your solution passes case "invalid character in isbn" if your input is 3-598-2K507-0 but fails the same case if you change the input to 35982K5070? Some may argue that another case is not necessary as a test for invalid characters in the middle of an ISBN code is already present in the test suite.

[dnicolaev]

My 1st solution worked next way (3598P1581X):

• separate last character X or digit - X in this case

• remained 9 characters - did calculations in case there are digits 3 10 + 5 9 + 9 8 + 8 7 + (P non digit - do nothing) + 1 5 + 5 4 + 8 3 + 1 2 + X * 1 = 264 264 % 11 = 0 So even the nondigit is in the middle - calculations on remained digits will give correct result.

  My 1st solution didn't have checks for invalid characters in the middle. Just did calculations on digit characters.

[rpottsoh]

I am not understanding how your solution caught the bad character when the input had hyphens but didn't catch it when the hyphens were absent. When this input is 3-598-2K507-0 your solution claims that that is a bad code (correct) but claims it is a good code when the hyphens are missing. I am curious to know why your solution doesn't think that 3-598-2K507-0 is not a good code too.

At this point I think I am more inclined to agree that adding another case to cover an input without hyphens with an illegal character in the middle is a good fit. Having the input equate as a good ISBN, if the illegal character is ignored, is a nice feature as well.

If you are willing, you may open a PR that contains an update to canonical-data.json that includes a new test case for this particular situation. I am sure over the coming days a few other people, that monitor this repo regularly, will include their thoughts on this issue.

[petertseng]

Having the input equate as a good ISBN, if the illegal character is ignored, is a nice feature as well

Let's have:

• One case that would be accepted by solutions that treat the letter as a 0 but do not increment the multiplier
• One case that would be accepted by solutions that treat the letter as a 0 and do increment the multiplier
• One case that would be accepted by solutions that treat the letter as a 10 already have this case, but check if this case would indeed be accepted.

[dnicolaev]

@rpottsoh sorry, but I don't understand what do you mean. there are 4 my iterations on exercism.io - right now I tested all them (using jupiter notebook) for 35982K5070 and 3-598-2K507-0 - all returns were False - so wrong ISBN.

[rpottsoh]

I don't understand what do you mean.

That's OK.

I was under the impression that your solution would pass case "invalid character in isbn" as it is written but would fail it if you omitted the hyphens from the input.

I was trying to establish that

 {
            "description": "invalid character in isbn",
            "property": "isValid",
            "input": {
              "isbn": "3-598-2K507-0"
            },
            "expected": false
        }

and

 {
            "description": "invalid character in isbn",
            "property": "isValid",
            "input": {
              "isbn": "35982K5070"
            },
            "expected": false
        }

are truly unique tests and that my second example above is effectively the same as

 {
            "description": "invalid character in isbn",
            "property": "isValid",
            "input": {
              "isbn": "3598P1581X"
            },
            "expected": false
        }

I think the point may be moot at this point. I think the idea of adding one (likely more) test cases is sounding like a good idea as @petertseng has indicated.

[petertseng]

I just want to confirm that your solution passes case "invalid character in isbn" if your input is 3-598-2K507-0 but fails the same case if you change the input to 35982K5070?

No, cannot confirm this. Regardless of whether the hyphens are present:

(K is treated as zero) 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, so the solution will reject both "3-598-2K507-0" and "35982K5070".

Compare to 3598P1581X. X is 10, P is 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, so the solution will accept both "3-598-P1581-X" and "3598P1581X".

[petertseng]

Every single incorrect implementation (and the one correct implementation) I entered into https://github.com/petertseng/exercism-problem-specifications/blob/verify/exercises/isbn-verifier/verify.rb#L27-L92 rejects "3-598-2K507-0", which means "3-598-2K507-0" is useless as a test case.

Unless someone can come up with a plausible incorrect implementation that accepts "3-598-2K507-0", I must recommend that "3-598-2K507-0" be removed. Replacing it with at least one of "3-598-P1581-X" or "3598P1581X" will do fine.

[rpottsoh]

I agree.
I think

        {
            "description": "invalid character in isbn",
            "property": "isValid",
            "input": {
              "isbn": "3-598-2K507-0"
            },
            "expected": false
        },

should be changed to

        {
            "description": "invalid character in isbn",
            "property": "isValid",
            "input": {
              "isbn": "3-598-P1581-X"
            },
            "expected": false
        },

I am willing to open a PR for this unless @dnicolaev would like the honors.

[dnicolaev]

@rpottsoh sorry for delay - I was away yesterday. For me experience is much important than honors ;-) I am new to git hub - so still learning how to contribute to other repositories.

I created PR for exercises/isbn-verifier/canonical-data.json - hopefully it is OK.

Should I do something else on other files ? For example isbn_verifier_test.py

Thank you for great tool and exercises !!!

[petertseng]

Should I do something else on other files ? For example isbn_verifier_test.py

That decision is left to the individual tracks, so officially we have no say in the matter, but since it was merged in here, it is highly likely track maintainers will accept PRs. There is no such thing as whether you should do it, it is only a question of what you are interested in doing and want to do.