Hamming tests for empty input don't make sense

[wolf99]

In the canonical-data.json for the hamming exercise there are three tests that appear to be in conflict with each other (or testing one things when intending to test separate things).

This question has impeded our update alignment of the C track's implementation of this exercise to match the current version of this problem specification (exercism/c#556) .

//...
    {
      "uuid": "919f8ef0-b767-4d1b-8516-6379d07fcb28",
      "description": "disallow first strand longer",
      "property": "distance",
      "input": {
        "strand1": "AATG",
        "strand2": "AAA"
      },
      "expected": {"error": "left and right strands must be of equal length"}
    },
    {
      "uuid": "8a2d4ed0-ead5-4fdd-924d-27c4cf56e60e",
      "description": "disallow second strand longer",
      "property": "distance",
      "input": {
        "strand1": "ATA",
        "strand2": "AGTG"
      },
      "expected": {"error": "left and right strands must be of equal length"}
    },
    {
      "uuid": "5dce058b-28d4-4ca7-aa64-adfe4e17784c",
      "description": "disallow left empty strand",
      "property": "distance",
      "input": {
        "strand1": "",
        "strand2": "G"
      },
      "expected": {"error": "left strand must not be empty"}
    },
    {
      "uuid": "38826d4b-16fb-4639-ac3e-ba027dec8b5f",
      "description": "disallow right empty strand",
      "property": "distance",
      "input": {
        "strand1": "G",
        "strand2": ""
      },
      "expected": {"error": "right strand must not be empty"}
    }

I read "empty strand" to mean a string of zero length, rather than a NULL or equivalent absence of any string at all (see #905 ).
However, given that input of both strands empty are allowed, this means that the latter two tests will fail due to unmatched string length even if empty string checking is not implemented. Thus I think that the latter tests will not test what is intended for them to test.

Can someone confirm my logic here?

I will suggest simply removing the latter two tests as a resolution, would this be acceptable?

PS: the following seems also related #1666

[SleeplessByte]

It seems that they're actually all left and right strands must be of equal length and not left strand must not be empty, right strand must not be empty.

Rather than removing it, I'm in favour of reimplementing them with the sensible error message.

[petertseng]

If the test cases are removed, please add in their stead some test that handles the below incorrect code:

def hamming(s1, s2)
  # this is incorrect because it should first check whether they're unequal length before returning 0
  return 0 if s1.empty? || s2.empty?
  raise UnequalLength if s1.length != s2.length

  # additional code omitted
end

Since those tests are at present the only tests that check for such an implementation, the tests would be incomplete without an equivalent test. If the eventual decision is to replace them with test cases with same input but corrected error messages, that does fit the bill.

[wolf99]

I'm not sure I follow the thoughts you raise @SleeplessByte and @petertseng

The following tests are already specified

1. Success: both strings empty (and implicitly, of equal length)
2. Success: Both strings non-empty and of equal length
3. Error: Strings of different length
4. Error: One string empty and the other not.

The tested cases being:

A. In the case both of the strings is empty, this falls under 1.
B. In the case where no string is empty, this falls under 2.
C. In the case where no string is empty but not of equal length, this falls under 3.
D. In the case where one of the strings is empty, this falls under 3.

We can see that case D above shows that the tests to check specifically for a single empty string is a re-statement of the case already described by C; thus the tests for 4 are duplicate tests.

I.e. testing for strings of an unequal length and testing for one string of 0 length and the other of non-0 is the same test, yet here we specify them separately for some reason.

I miss what additional exercise implementation test 4 would require?

    # this returns 0 for both test 3 and 4
    return 0 if s1.length != s2.length?

    # this returns 0 for test 4
    return 0 if s1.empty? || s1.empty?

    # so two tests covering the same thing, 
    # meaning the second check never
    # needs to be implemented.
    

[petertseng]

return 0 if s1.length != s2.length

The above line of code is incorrect. s1.length != s2.length is an error, not 0 distance.

We are all in agreement here that empty strings need not be treated differently from nonempty strings. But there are students who do not know, and who do special-case empty strings. And they special-case them wrongly. So those tests are making sure the special-casing is not done incorrectly. The tests are worthless for students who do not special-case empty strings, we are in agreement there.

[SleeplessByte]

And I am proposing to unburden those students who don't do special cases by normalising all the error messages.

[petertseng]

EBWODP: The incorrect code has been clarified to this:

def hamming(s1, s2)
  # this is incorrect because it should first check whether they're unequal length before returning 0
  return 0 if s1.empty? || s2.empty?
  raise UnequalLength if s1.length != s2.length

  # additional code omitted
end

[wolf99]

Thanks for clarifying I understand better now ☺️.
Will adjust my PR to reflect this.