isbn-verifier: Crafted input to catch more incorrect algorithms

[nathanielknight]

This commit adds an input that will be incorrectly validated by
algorithms that don't explicitly check the length of their inputs, as
discussed in #1199 and #993.

For example, if an algorithm only checks the first (or last) ten
digits of the input, ignoring leftovers, it will pass the current test
suite even though it doesn't correctly implement the spec.

closes #1199

[nathanielknight]

Big thanks to @petertseng for helping me with this submission. 👍

[petertseng]

Thank you, now the two implementations at https://github.com/petertseng/exercism-problem-specifications/blob/verify/exercises/isbn-verifier/verify.rb#L46-L52 and https://github.com/petertseng/exercism-problem-specifications/blob/verify/exercises/isbn-verifier/verify.rb#L62-L69 are covered by this test case, and these were the last two mistaken implementations being tracked!

Question: At this point, the input named "too long isbn" is superseded by the new case added in this PR. The mistaken implementation at https://github.com/petertseng/exercism-problem-specifications/blob/verify/exercises/isbn-verifier/verify.rb#L54-L61 also incorrectly accepts the new case, in addition to the "too long isbn" case.

I would suggest that this means the old "too long isbn" case should be removed, since it does not independently serve any other purpose. There are no other mistaken implementations that incorrectly accept it.

Any disagreements with this suggestion?

(Of course, I Approve this PR to be merged as-is if there are any disagreements, but I do think the now-obsolete case can be removed)

[petertseng]

(to avoid any mistakes, the old case I'm referring to that I suggest to remove is 3-598-21507-XX)

[nathanielknight]

I think it's worth keeping both cases. Having both "too long" and "too long with valid ISBN" cases highlights that a solution can have incorrect features even if it incidentally passes the cases designed to test those features.

[petertseng]

highlights that a solution can have incorrect features even if it incidentally passes the cases designed to test those features.

This is an interesting point! Let's talk about it (to make sure I understand the reasoning).

So I think that means that a student who initially writes a solution that checks all 11 digits (corresponding to https://github.com/petertseng/exercism-problem-specifications/blob/verify/exercises/isbn-verifier/verify.rb#L46-L52 ) would correctly reject the 3-598-21507-XX case, and think "good, I am indeed rejecting ISBNs that are too long". Then when they get to 98245726788 the student sees "ah, in fact I am not rejecting all ISBNs that are too long", and this is a good learning moment.

Okay, I can see this being a good thing. I wish that this could be a part of the experience for everyone as well. Currently a student who writes a solution that checks the left 10 digits (corresponding to https://github.com/petertseng/exercism-problem-specifications/blob/verify/exercises/isbn-verifier/verify.rb#L54-L61) would already incorrectly accept the 3-598-21507-XX case, and therefore they don't get this extra moment of learning. Is it OK that the experience is different for students who write such a solution?

(The answer might be yes, I haven't thought about it. I am just trying to figure out why and explain my approach to deciding whether to keep a case, and seeing if I should change my approach)

[nathanielknight]

So I think that means that a student who initially writes a solution that checks all 11 digits (corresponding to https://github.com/petertseng/exercism-problem-specifications/blob/verify/exercises/isbn-verifier/verify.rb#L46-L52 ) would correctly reject the 3-598-21507-XX case, and think "good, I am indeed rejecting ISBNs that are too long". Then when they get to 98245726788 the student sees "ah, in fact I am not rejecting all ISBNs that are too long", and this is a good learning moment.

Yeah, this is exactly what I meant! 👍

I think it's okay for learners to have different experiences as they move through the test suite (indeed, some will have learned this lesson before and be wise to this sort of ploy), but it's worth making sure we're getting the best possible outcome for as many folks as possible.

The cases we're considering are solutions that:

• validate length and check exactly 10 digits
• don't validate length and:
  • validate only the first 10 digits of input
  • validate only the last 10 digits of input
  • validate all digits of input with a generalized checksum

If the learner's solution validates length, I don't think we can catch them out with the ploy of a "treacherous test" that gives the right result for the wrong reason. If they're in the other category, we want a progression of tests where everybody gets a false positive that they'll catch eventually.

By keeping both exercises in, we get the "right outcome for the wrong reasons" in two out of three of the above cases (incorrect solutions that check 11 digits or the last 10 digits of input will reject 3-598-21507-XX for the wrong reasons). The goal would be to find an input that also gives the "right outcome for the wrong reason" for solutions that only check the first ten digits. I think that might just end up with a test case that's a random string of numbers that any solution would reject.

I have to sit down and game this out in more detail tonight... 😖

[nathanielknight]

Upon review, I think we can take out the "too long isbn" test(3-598-21507-XX). If all my checks are correct, the previous "too long isbn and no dashes" test (3598215078X) will spuriously pass for each of the three solutions types we discussed above. The new test case will catch the oversight later, and everybody gets the lesson.

I've updated the PR removing the old test case.

[ErikSchierboom]

I agree with removing the old test case. This seems like a natural progression to me.

[petertseng]

Hey there, just checking in to confirm (as should be known) that I do agree with things as they currently are (having removed 3-598-21507-XX).

It would have been interesting to get a progression that presents all of the three listed implementations (left 10, right 10, all 11) with first a passing test (as 3-598-21507-XX did for some), but I think logically one can see that is not possible unless all of those implementations would correctly reject the test case, and if we're doing that then I suppose we'd better think of a fourth type of implementation that would incorrectly accept the earlier case.

I believe enough time has passed since the last commit was submitted for review, so I think merging is appropriate, unless someone has any last words in the next 12 hours or so. Since you the submitter can merge, I will not be merging and instead leaving it to you to do so.

[rpottsoh]

I don't really have the time right now to do a thorough review. I trust @petertseng and @ErikSchierboom judgment here. Peter and I have worked together on issues pertaining to this particular exercise before.

Please squash when merging.