Skip to content

crypto-square: Don't test intermediate functions#922

Merged
ErikSchierboom merged 1 commit intoexercism:masterfrom
nholden:fix-crypto-canonical-data
Oct 6, 2017
Merged

crypto-square: Don't test intermediate functions#922
ErikSchierboom merged 1 commit intoexercism:masterfrom
nholden:fix-crypto-canonical-data

Conversation

@nholden
Copy link
Copy Markdown

@nholden nholden commented Oct 4, 2017

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.

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.
@nholden
Copy link
Copy Markdown
Author

nholden commented Oct 4, 2017

Here's one more relevant issue: #750

"cases": [
{
"description": "the spaces and punctuation are removed from the English text and the message is downcased",
"cases": [
Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Author

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.

Copy link
Copy Markdown
Contributor

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.

@Insti
Copy link
Copy Markdown
Contributor

Insti commented Oct 4, 2017

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.

@ErikSchierboom ErikSchierboom merged commit b66eccb into exercism:master Oct 6, 2017
@ErikSchierboom
Copy link
Copy Markdown
Member

Thanks @nholden !

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants