Skip to content

fix pangram test for duplicate mixed-case chars#565

Merged
petertseng merged 2 commits intoexercism:masterfrom
rsslldnphy:pangram-test-fix
Jun 28, 2017
Merged

fix pangram test for duplicate mixed-case chars#565
petertseng merged 2 commits intoexercism:masterfrom
rsslldnphy:pangram-test-fix

Conversation

@rsslldnphy
Copy link
Copy Markdown
Contributor

From what I can see, the last test case is inteded to catch incorrect
programs such as this, which don't take into account different-cased
duplicates:

isPangram = (== 26) . length . nub . filter isAlpha

However the above program passed the final test as the count of distinct
upper and lower case characters is 27, rather than 26.

This commit changes the test text to a non-pangram that has exactly 26
distinct upper and lower case characters, which causes the above,
incorrect program to fail.

@rbasso
Copy link
Copy Markdown
Contributor

rbasso commented Jun 24, 2017

Hi!

Thanks for taking the time to open this PR.

From what I can see, the last test case is inteded to catch incorrect
programs such as this, which don't take into account different-cased
duplicates:

That test case was implemented by exercism/problem-specifications#439, as a solution to the issue exercism/problem-specifications#266:

Check out: exercism/kotlin#10

The implementation that passed all existing tests didn't need to lowercase the input and was able to double count the same upper-case and lower-case characters. A new test was implemented to swap out dog in the quick brown fox jumped over the lazy dog with FOX which catches the error after asserting it's not a pangram.

The implementation mentioned in exercism/kotlin#10 was the following:

fun isPangram(sentence: String): Boolean {
    return sentence
            .filter { it.isLetter() }
            .toList()
            .distinct()
            .count() >= 26
}

...which is very similar to your code:

isPangram = (== 26) . length . nub . filter isAlpha

However the above program passed the final test as the count of distinct
upper and lower case characters is 27, rather than 26.

I agree that it is undesirable that the solution passes the test, but the test cases that we used in this exercise came from this file in the exercism/problem-specifications repository, which contains a pool of test cases that all languages can share.

For that reason, it would be great if you could open an issue there, so that maintainers from other languages could discuss it, and the change would help more people.

After the change is discussed and merged there, then we'll also change it here. 👍

@petertseng
Copy link
Copy Markdown
Member

Ready to go here. Can we ask that https://github.com/exercism/haskell/blob/master/exercises/pangram/package.yaml#L2 change to 1.1.0.3?

@rsslldnphy
Copy link
Copy Markdown
Contributor Author

👍 have rebased and bumped version

From what I can see, the last test case is inteded to catch incorrect
programs such as this, which don't take into account different-cased
duplicates:

isPangram = (== 26) . length . nub . filter isAlpha

However the above program passed the final test as the count of distinct
upper and lower case characters is 27, rather than 26.

This commit changes the test text to a non-pangram that has exactly 26
distinct upper and lower case characters, which causes the above,
incorrect program to fail.
Comment thread exercises/pangram/package.yaml Outdated
@@ -1,5 +1,5 @@
name: pangram
version: 1.0.0.2
version: 1.0.0.3
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first three digits signify x-common version we are following. Since we are following 1.1.0, please 1.1.0.3 not 1.0.0.3

I am sorry that we did not document this (#538)

@petertseng
Copy link
Copy Markdown
Member

also, I considered asking for a https://github.com/exercism/haskell#example-solution fail example solution that would ensure that we have a test that catches this incorrect solution, but perhaps that problem is better solved on the problem-specifications side. The current fail solutions we have are checking for fails on Haskell-specific things (strictness)

@petertseng petertseng merged commit d69f5d4 into exercism:master Jun 28, 2017
@petertseng
Copy link
Copy Markdown
Member

Very good, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants