isbn-verifier: Test Suite Omits Important Edge Case

[kytrinyx]

Reported by @p0st0culus in exercism/exercism#3702

Confirmed missing from canonical data.

---

The test suite for the ISBN Verifier problem in the Swift track should test for an empty string ISBN. This is a an important edge case that @damlar noted was missing from my solutions which is also omitted from the test suite.

It's something that, in hindsight, I should have known to test for, but I didn't. That's the whole point of a well thought out test suite though: Code against assertions of what a valid solution should produce. I tend to implicitly put trust in the test suites, so that's my lesson learned. I'll take an active role in the test suites going forward.

I recommend modifying IsbnVerifierTests.swift as follows. Other content from the test suite omitted with ellipses for clarity:

class IsbnVerifierTests: XCTestCase {

    //...

    func testEmptyIsbn() {
        XCTAssertFalse(IsbnVerifier.isValid(""))
    }

    //...

    static var allTests: [(String, (IsbnVerifierTests) -> () throws -> Void)] {
        return [
            //...
            ("testEmptyIsbn", testEmptyIsbn),
            //...
        ]
    }
}

Thanks.

[rpottsoh]

This seems related to #902, which is dealing with including or excluding cases that test for invalid input. The recent trend has been to steer clear of test cases that check for invalid input unless the test has relevance to the problem. The assumption being that, in real life, some algorithm would likely pre-screen the input before passing it on to the algorithm, ISBN Verifier in this instance.

I personally have no issue with adding another test case that looks for an empty ISBN code to be verified.

[TwilightCitizen]

It’s somewhat related, and somewhat different. In the ISBN Verifier, we’re actually doing validation. There are already a few invalid ISBNs in the test suite because of this. And, “” is not a valid ISBN, but it isn’t tested for. Pertaining to #902, I’ll just say I think reasonable edge cases should always be tested for unless the problem statement explicitly says something like “Assume all inputs have been validated by another process.”

[coriolinus]

I have a somewhat different understanding from #902: as I see it, all exercises should be able to implicitly assume that inputs have been pre-screened for validity.

However, I'd agree that in this case we're actually performing validation, and a zero-length string is a reasonable case to test here.

[rpottsoh]

Appears to me that there are two changes that need to be made.

1. Add a case that checks for an empty string input.
2. All cases updated to include an input class per JSON Schema proposal: All inputs should appear under the input key #996.

[coriolinus]

Yes, but those two should be in separate PRs; they're distinct.

[rpottsoh]

I agree

[rpottsoh]

@p0st0culus where should this new case appear in the test data? I am working on a PR to get #996 out of the way.