Skip to content

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

Merged
merged 1 commit into from
Oct 6, 2017
Merged

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

merged 1 commit into from
Oct 6, 2017

Conversation

nholden
Copy link

@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
Author

nholden commented Oct 4, 2017

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": [
Copy link
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
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
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
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
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