Skip to content

pangram: rework tests (discussion) #893

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Vankog wants to merge 3 commits into from
Closed

pangram: rework tests (discussion) #893

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5d97048
reworked pangram spec to account for a teachable BDD/TDD progression …
Vankog Sep 8, 2017
92180f1
refine pangram test after feedback
Vankog Sep 15, 2017
ed25b5c
new revision of pangram
Vankog Oct 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 38 additions & 19 deletions exercises/pangram/canonical-data.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,68 +1,87 @@
{
"exercise": "pangram",
"comments": [
"A pangram is a sentence using every letter of the alphabet at least once."
"A pangram is a sentence using every letter of the alphabet at least once.",
"",
"It is the track's responsibility to add further language specific tests, like:",
"input error handling (null/undefined parameters etc.)",
"referential transparency (i.e. evaluating, a function/method gives the same value for same arguments every time.)",
"etc.",
"",
"'error' and 'null' should be treated according to the languages' specifics and possibilities."
Copy link
Copy Markdown
Member

@petertseng petertseng Oct 13, 2017

Choose a reason for hiding this comment

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

this is not strictly necessary, since there are no longer any error or null used in this file (unless I missed one, sorry!)

],
"version": "1.1.0",
"version": "1.2.0",
"cases": [
{
"description": "Check if the given string is an pangram",
"description": "checks if the given string is a pangram.",
"comments": [
"Output should be a boolean denoting if the string is a pangram or not."
],
Copy link
Copy Markdown
Contributor

@ilya-khadykin ilya-khadykin Sep 8, 2017

Choose a reason for hiding this comment

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

Don't forget to change the version accordingly, the current one is (semantic versioning):

"version": "1.1.0",

Copy link
Copy Markdown
Contributor Author

@Vankog Vankog Sep 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Funny you brought this up right in the moment I amended this change ^^
I changed to 2.0.0, because the structure changed completely. Is this correct or should I do 1.2.0?

Copy link
Copy Markdown
Contributor

@stkent stkent Sep 8, 2017

Choose a reason for hiding this comment

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

Here are the guidelines governing test versioning: https://github.com/exercism/problem-specifications#test-data-versioning

Since it appears that (correct me if I'm wrong):

  • there's no new property
  • the existing property has not been renamed
  • there are no new keys
  • no key types have been changed

we don't need a major version bump here, and v1.2.0 should suffice (minor vs patch because inputs/outputs were changed).

Copy link
Copy Markdown
Contributor Author

@Vankog Vankog Sep 8, 2017

Choose a reason for hiding this comment

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

thanks for the guide. Changed it to 1.2

"cases": [
{
"description": "sentence empty",
"description": "returns false for an empty message.",
"property": "isPangram",
"input": "",
"expected": false
},
{
"description": "pangram with only lower case",
"description": "recognizes a perfect lower case pangram.",
"property": "isPangram",
"input": "the quick brown fox jumps over the lazy dog",
"input": "abcdefghijklmnopqrstuvwxyz",
"expected": true
},
{
"description": "missing character 'x'",
"description": "recognizes lower case pangram with spaces.",
"property": "isPangram",
"input": "few quips galvanized the mock jury box",
"expected": true
},
{
"description": "recognizes missing characters, e.g. 'x'.",
"property": "isPangram",
"input": "a quick movement of the enemy will jeopardize five gunboats",
"expected": false
},
{
"description": "another missing character 'x'",
"description": "recognizes missing characters, e.g. 'h'.",
"property": "isPangram",
"input": "the quick brown fish jumps over the lazy dog",
"input": "five boxing wizards jump quickly at it",
"expected": false
},
{
"description": "pangram with underscores",
"description": "recognizes an upper-case pangram.",
"property": "isPangram",
"input": "the_quick_brown_fox_jumps_over_the_lazy_dog",
"input": "THE QUICK BROWN FOX JUMPS OVER THE LAZY DOG",
"expected": true
},
{
"description": "pangram with numbers",
"description": "recognizes a missing character in upper-case, e.g. 'G'.",
"property": "isPangram",
"input": "THE QUICK BROWN FOX JUMPS OVER THE LAZY DOLL",
"expected": false
},
{
"description": "recognizes a mixed-case pangram.",
"property": "isPangram",
"input": "the 1 quick brown fox jumps over the 2 lazy dogs",
"input": "ThE FiVe bOxInG WiZaRdS JuMp qUiCkLy",
"expected": true
},
{
"description": "missing letters replaced by numbers",
"description": "recognizes a missing character in mixed-case, e.g. 'Y'.",
"property": "isPangram",
"input": "7h3 qu1ck brown fox jumps ov3r 7h3 lazy dog",
Copy link
Copy Markdown
Member

@petertseng petertseng Oct 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

one interesting thing about this input is that the number of unique non-space characters in it is exactly 26, which is the intent of #852 .

Now it looks like we are testing mixed-case before non-alphabetics, all right, so we wouldn't have the letters -> numbers case, that seems fine.

If you believe that is still a useful bug to catch, then it would be useful to have a case like "Aabcdefghijklmnopqrstuvwxy"

The criterion is not met by "ThE FiVe bOxInG WiZaRdS JuMp qUiCkL" because the number of unique non-space characters in it is 28.

Copy link
Copy Markdown
Contributor Author

@Vankog Vankog Oct 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@petertseng Thanks for this hint.
However, I am not sure I understand the intention of this bugfix or the changed testcase. Is it to test for mixed-case, but with 26 distinct chars?
If so, I think we should add a testcase with exactly that intent to prevent future misunderstandings/changes. I added this as a ToDo item below: #893 (comment)

Copy link
Copy Markdown
Member

@petertseng petertseng Oct 13, 2017

Choose a reason for hiding this comment

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

Is it to test for mixed-case, but with 26 distinct chars?

Yes. The test case that got replaced is: lowercase + numbers, with 26 distinct non-space chars.

I was thinking it would get replaced with: lowercase + uppercase, with 26 distinct non-space chars.

I think you understood correctly.

"input": "ThE FiVe bOxInG WiZaRdS JuMp qUiCkL",
"expected": false
},
{
"description": "pangram with mixed case and punctuation",
"description": "recognizes a pangram with special characters.",
"property": "isPangram",
"input": "\"Five quacking Zephyrs jolt my wax bed.\"",
"input": "Sphinx_of-'black\" .quartz, >judge< my; #v0w!",
"expected": true
},
{
"description": "upper and lower case versions of the same character should not be counted separately",
"description": "recognizes missing characters (e.g. 'o') with special characters present.",
"property": "isPangram",
"input": "the quick brown fox jumps over with lazy FX",
"input": "Sphinx_0f-'black\" .quartz, >judge< my; #v0w!",
"expected": false
}
]
Expand Down