-
-
Notifications
You must be signed in to change notification settings - Fork 554
crypto-square: Don't test intermediate functions #922
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.
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
crypto-square: Don't test intermediate functions #922
Conversation
We want to remove tests of intermediate functions in crypto-square that test implementation details per these issues: - exercism/discussions#41 - exercism/ruby#422 This rewrites the canonical data for crypto-square so that we're only testing the expected value of the final "ciphertext" property and none of the intermediate properties.
Here's one more relevant issue: #750 |
@@ -1,82 +1,27 @@ | |||
{ | |||
"exercise": "crypto-square", | |||
"version": "2.0.0", | |||
"version": "3.0.0", | |||
"cases": [ | |||
{ | |||
"description": "the spaces and punctuation are removed from the English text and the message is downcased", | |||
"cases": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we still be using sub cases
keys if we are only testing one property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, good question! I see in the README:
Test cases can be arbitrarily grouped with a description to make organization easier.
These cases
all have to do with "normalizing" text, while the next group of cases
deal with "chunking" and ordering letters. I'm not sure whether or not that "makes organization easier." I'm happy to dump the sub cases
or save that re-organization for when we add new tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what the answer is either, I'm OK with leaving it as it is for now as the logical separation is still there even if the property name has changed.
If it ends up breaking someone's test generator I'm sure they'll submit a PR to fix it.
Removing the intermediate functions is good 👍 There are probably more test cases that need to be added, but they can be included by a separate PR once we work out what they are. |
Thanks @nholden ! |
We want to remove tests of intermediate functions in crypto-square that pry into implementation details per these issues:
This rewrites the canonical data for crypto-square so that we're only testing the expected value of the final "ciphertext" property and none of the intermediate properties.