Incomplete test coverage for scrabble-score

[dexterlemmer]

Sorry if I make any mistakes here. This is my first issue on exercism, and one of my first on GitHub. Furthermore, I'm actually rather new to TDD.

Proposals:

Proposal 1:

Add the following test to scrabble-score:

#[test]
fn german_letters_do_not_score() {
    assert_eq!(score("STRASSE"), 7, "\"SS\" should score 2.");
    assert_eq!(score("STRAßE"), 5, "'ß' should score 0");
}

Proposal 2:

Additionally, modify the non_english_scrabble_letters_do_not_score test to something like:

#[test]
fn non_english_scrabble_letters_do_not_score() {
    assert_eq!(score("pinata"), 8, "'n' should score 1");
    assert_eq!(score("piñata"), 7, "'ñ' should score 0");
}

Motivation:

Motivation for Proposal 1:

The exercise scrabble-score contains a test for ensuring only "English" letters is used, namely non_english_scrabble_letters_do_not_score. I assume that means all non-English letters shouldn't score. For example, the "word" "ß" should score 0!

Here's the problem: I originally thought the obvious solution is:

fn codepoint_score(cp: char) -> u32 {
  if !cp.is_uppercase() {
    panic!("This method assumes uppercase!");
  } else { match cp {
    'A'|'E'|'I'|'O'|'U'|'L'|'N'|'R'|'S'|'T' => 1,
    'D'|'G'                                 => 2,
    'B'|'C'|'M'|'P'                         => 3,
    'F'|'H'|'V'|'W'|'Y'                     => 4,
    'K'                                     => 5,
    'J'|'X'                                 => 8,
    'Q'|'Z'                                 => 10,
    _                                       => 0,
  }}
}

pub fn score(word: &str) -> u32 {
  word.chars().map(|c| c.to_uppercase()).map(codepoint_score).sum()
}

It doesn't compile due to to_uppercase() having an output-type of ToUppercase rather than char. So I resolved the compilation error by changing my code to:

fn codepoint_score(cp: char) -> u32 {
    ...
}

fn letter_score(letter: char) -> u32 {
    letter.to_uppercase().map(codepoint_score).sum()
}

pub fn score(word: &str) -> u32 {
    word.chars().map(letter_score).sum()
}

The above code correctly passes non_english_scrabble_letters_do_not_score, but also correctly fails the following test:

#[test]
fn german_letters_do_not_score() {
    assert_eq!(score("STRASSE"), 7, "\"SS\" should score 2.");
    assert_eq!(score("STRAßE"), 5, "'ß' should score 0");
}

I don't know if anybody else would go this route. However since in Rust's char.to_uppercase returns a ToUppercase, which can be obviously checked one codepoint at a time. And since the scrabble-score exercise, contains the test scoring_is_case_insensitive, I at least think that such an approach might occur more often. Even if it doesn't. Including a test that tests for something like this might encourage other students to think about some more "out of the box" problems during coding and/or testing.

My proposal is therefore to add a test that would fail if any character that char.to_uppercase converts to two or more English letters gets a score greater than 0.

Motivation for Proposal 2:

1. The test has two assertions, therefore if the test ever fails, the user needs to look up the line reference to determine why it failed. Simply adding the proposed information to the assertion should inform the user what went wrong without him needing to lookup the line reference.
2. The proposed version of the test is more self-documenting (i.e. it is easier for a reader to understand how each assertion in the test works).
3. This introduces a careful reader to the optional third parameter of assert!.

Example:

For some reason I can't seem to attach files. See iteration >=3 of http://exercism.io/submissions/b0f318070ab34df2aa3a03b0ecfcc0bb for a complete example.

Updates:

Several updates due to problems related to my being incapable attaching a file.

[petertseng]

Aside: I found it is quite amazing that assert_eq can take a message - it was added in 1.11 in rust-lang/rust@45a63d3 and never documented until rust-lang/rust@ff5ba73 (but did get a blog announcement in https://blog.rust-lang.org/2016/08/18/Rust-1.11.html which I apparently missed).

---

Thanks for the proposals and motivations for them! I agree with both of them, conditional on the Rust track's desire to continue using non-ASCII characters in our scrabble-score tests. Note that in using non-ASCII characters, our track is in contravention of the Exercism-wide exercism/problem-specifications#428.

Note that in #125 this track made an explicit decision to add the non-ASCII character because otherwise the implementation could in an extreme case be a one-liner. That decision was made before the Exercism-wide decision of exercism/problem-specifications#428.

So I would like to ask now, do we at this track stand by our earlier decision, or shall it be re-evaluted in light of exercism/problem-specifications#428? If we stand by our decision, I recommend applying both proposals in this issue.

If nobody comes forth to either support or oppose the decision to use non-ASCII characters, then in that case I also recommend to apply both proposals, since they definitely improve the state of the current tests.

[ijanos]

This is a well thought out proposal and definitely improves the current state of the exercise. I'm in favor of keeping non-ASCII characters in our tests.

[IanWhitney]

I support non-ASCII tests.

[dexterlemmer]

First. Sorry for my looong issue and now also comment.

@petertseng

So I would like to ask now, do we at this track stand by our earlier decision, or shall it be re-evaluted in light of exercism/problem-specifications#428? If we stand by our decision, I recommend applying both proposals in this issue.

I didn't know about exercism/problem-specifications#428.

My personal viewpoint: Yes, since:

1. AFAIK we don't currently have any Unicode-specific exercises (as proposed in Suggest specific exercises that deal with multi-language text handling. problem-specifications#455) in the Rust track as far as I'm aware. Until we do, we should probably keep this issue as well as Add test case to Hamming that shows the difference between len() and chars().count() #128.
2. In Rust safety is especially important. One of the few cases where the default approach isn't safe, is &str, being a slice of bytes rather than some container of characters. Yet many bugs in real-world software is due to improper handling of unicode. Therefore, it would be good to drill in "unicode safety" so to speak.
3. An important motivation for Suggest specific exercises that deal with multi-language text handling. problem-specifications#455 was that in many languages, it is a PITA to handle non-US-ASCII and you may well require additional dependencies. In Rust these are nearly non-issues. However, it may be a good idea to at least inform the learners about std::ascii::AsciiExt in the README.md of some exercise.

Some points against keeping the proposed tests (and indeed the original non_english_scrabble_letters_do_not_score test):

First problem:

Neither this nor #125 prevents one-line solutions. In my own solution the following works:

pub fn score(word: &str) -> u32 {
    word.chars().map(codepoint_score).sum()
}

This is because codepoint_score uses the rather obvious (if you know about to_ascii_uppercase)

match cp.to_ascii_uppercase() {
    ...
}

If I used #125's dictionary. I suspect the following would work (I'll test this later):

pub fn score(word: &str) -> u32 {
    let values: HashMap<char, u16> = dictionary();
    word.chars().map(|c| values.get(c.to_ascii_lowercase()).unwrap_or(0)).sum()
}

Second problem:

The fourth point made by @NobbZ as a comment on exercism/x-common/issues/428:

4. Make it easier to have uniform test-data across different tracks.

I don't know the importance of this.

Third problem:

exercism/go/issues/195 shows that dealing with unicode can get really nasty! If you handle some aspects of unicode (such as len() vs chars().count() and to_uppercase() vs to_ascii_uppercase(), you should probably also handle other aspects (such as RTL and U+202E and - if you're calculating the width of the string in the exercise - wide chars).

[hekrause]

I wanted to work on this issue, read all the links and came to this decision:

1.Proposal

According to Issue 428 it would be reasonable to only write non-Ascii tests when it makes sense in a real world example.
If you think what could be a valid input in a scrabble game it really depends on the language of the game. For example there exists a hebrew version of scrabble with awesome letters which are not in any ascii table.
So a good solution for this exercise would be to write test only for english language and make this clear in the description (Maybe with listing all valid letters).

2.Proposal

Adding a information in the console output what the purpose of the failing test is is a great thing which i think some should do in all future implementations.
Maybe somebody should open a policy issue to discuss this idea.

[petertseng]

it would be reasonable to only write non-Ascii tests when it makes sense in a real world example.
So a good solution for this exercise would be to write test only for english language and make this clear in the description (Maybe with listing all valid letters).

And there is precedent for this in exercism/problem-specifications#284 as well

information in the console output what the purpose of the failing test is

I have some hope that sometimes this can be expressed via the function name, but if that is not possible it is surely good to add it as the assert message.

[coriolinus]

I'd like to resolve this issue. I believe that both tests proposed could usefully be added to the exercise's suite, and discussion seems to have ceased with a general consensus in favor of both additions.

@dexterlemmer would you like to create a PR adding both test cases? If so, do you need any help getting started?