Hi,
During a review, the person ask me if his code check if the X character is only acceptable at the end of the ISBN. So I look at the test and I see this test:
I think this is the test that test it. But here is the problem I have.
The sum % 11 is not equal to 0.
Here is his solution:
def is_valid(isbn):
isbn = isbn.replace('-', '')
if len(isbn) != 10:
return False
sum = 0
for index, digit in enumerate(list(isbn)):
if digit.isdecimal():
sum += int(digit) * (10 - index)
elif digit == 'X':
sum += 10
else:
return False
return sum % 11 == 0
As you can see the 10 (replacing the X value) is not multiplied (this is how he write his solution). Because of this his test pass but not for the good reason. It pass because the sum % 11 != 0 and not because X is valid only as a check character.
I think we should add the test case where the ISBN is "3-598-2X507-5". This is a case where the sum is 264 (and 264 % 11 == 0). Doing so will handle that sum % 11 == 0 and the X character is not at the end.
Given that this was raised more than once, we have a reproducible wrong implementation that passes the tests and a test suite that indicates it should not pass (but accidentally does), I am in favour of adding a or multiple tests to fix this issue.
I commented this in the PR, but to get more eyeballs on it here, do we need to keep the current test if it doesnât catch all edge cases and its superseded by the OPâs suggestion?
The proposed test fails to check when X is substituted for 10 and multiplied (by 5, according to the position where it is located in the string). The sum would be:
Which would make the result false for the wrong reason, exactly as the current test does.
I think we should either have 2 tests, one that fails when X is substituted for 10 and not multiplied, and another that fails when it is substituted for 10 and multiplied; or we should find a convenient arrangement of digits that fails in both cases.
EDIT: This means Iâm in favor of adding the proposed test and also of keeping the current one (which asserts the case where X is substituted for 10 and multiplied). I donât think one deprecates the other, they are complementary.
The purpose of the tests isnât to be exhaustive. The purpose of the tests is to guide students towards a correct implementation. We add tests when itâs reasonable to assume that students may land on an incorrect implementation without a test to help highlight a specific bug.
Do people think this test case is needed? What implementation would lead to this? Is it likely that a student would unintentionally have a bug in their code that would lead to this result? Are we solving for a potential bug with this test or trying to write comprehensive tests?
If the student filters out X, the tests that depend on a valid X being substituted for 10 would fail.
And there are tests for the number of digits, like @glennj said above. So, if the student chooses to substitute only a X in the last position and ignore the others, those tests would fail.
So, I donât think this third test is needed after all, just the other two (the current one and the proposed one).
Students can write code which can have all sort of bugs which would require exhaustive testing to capture each and every case.
If we wanted tests which would catch every solution students could write, weâd be writing exhaustive tests. This is explicitly not the goal of the tests.
The question isnât if it is possible for a student to write buggy code that would be caught by the test but if it is probable. The question is about how likely that is to occur and how reasonable said code would look.
Yes, thatâs what I said. The third test isnât needed because the ânaturalâ incorrections it catches are mostly covered by other tests. Only the other two (one of them being the current test already present) add something substantial.
So, I think adding the proposed test without deprecating the current one is probably enough.