Skip to content
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

simple-cipher: Add canonical data #966

Closed
wants to merge 4 commits into from
Closed
Changes from all commits
Commits
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
123 changes: 123 additions & 0 deletions exercises/simple-cipher/canonical-data.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
{
"exercise": "simple-cipher",
"version": "1.0.0",
"cases": [
{
"description": "Random key cipher",
"property": "random",
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure, but I don't think this line actually does anything. The property key is normally used for specifying which method/property to test. As this is just a container for individual cases, I think it can be removed.

"cases": [
{
"description": "It has a key made of letters",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe shorten to "Key has only lower-case letters"?

"property": "key",
"input": "any",
"matches_regex": "^[a-z]+$"
Copy link
Member

Choose a reason for hiding this comment

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

We are in the process of enforcing lowerCamelCase keys (see #987), so I would suggest to rename the keys (not only this one) to lowerCamelCase.

},
{
"description": "It can encode",
"property": "encode",
"input_plaintext": "aaaaaaaaaa",
"output_key": "cipher.key"
},
{
"description": "It can decode",
"property": "decode",
"input_key": "cipher.key",
"output_string": "aaaaaaaaaa"
Copy link
Member

Choose a reason for hiding this comment

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

This should be renamed to expected.

},
{
"description": "It is reversible",
"property": "encode -> decode",
"input_encode": "plaintext string",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe rename to plaintext or input?

"output_decode": "plaintext string"
Copy link
Member

Choose a reason for hiding this comment

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

This should be renamed to expected.

}
]
},
{
"description": "Incorrect key cipher",
"property": "incorrect",
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure, but I don't think this line actually does anything. The property key is normally used for specifying which method/property to test. As this is just a container for individual cases, I think it can be removed.

"cases": [
{
"description": "It throws an error with an all caps key",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this test should be about testing a string with a single uppercase character, as that is the minimal counter-example.

"property": "Cipher Key",
Copy link
Member

Choose a reason for hiding this comment

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

I think the property should be key

"input_all_caps_string": "ABCDEF",
"output_error": "Bad key"
Copy link
Member

Choose a reason for hiding this comment

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

This property should be renamed to expected and should be structured as an object with a single error key. See this example.

},
{
"description": "It throws an error with a numeric key",
"property": "Cipher Key",
Copy link
Member

Choose a reason for hiding this comment

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

I think the property should be key

"input_all_number_string": "'12345'",
Copy link
Member

Choose a reason for hiding this comment

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

Rename to input

"output_error": "Bad key"
Copy link
Member

Choose a reason for hiding this comment

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

This property should be renamed to expected and should be structured as an object with a single error key. See this example.

},
{
"description": "It throws an error with empty key",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe rephrase to something like: "An empty key is invalid"

"property": "Cipher Key",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe rename the property to key?

"input_empty_string": "''",
Copy link
Member

Choose a reason for hiding this comment

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

I would rename to input.

"output_error": "Bad key"
Copy link
Member

Choose a reason for hiding this comment

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

This property should be renamed to expected and should be structured as an object with a single error key. See this example.

}
]
},
{
"description": "Substitution cipher",
"property": "substitution",
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure, but I don't think this line actually does anything. The property key is normally used for specifying which method/property to test. As this is just a container for individual cases, I think it can be removed.

Copy link
Member

Choose a reason for hiding this comment

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

Should this be encode?

Copy link
Member

Choose a reason for hiding this comment

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

Possibly it should be substitution_encode or substitutionEncode, if there are other encoding methods with different types of inputs and outputs.

"cipher_key_string": "abcdefgihj",
Copy link
Member

Choose a reason for hiding this comment

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

Is this the key that is used in the nested cases? If so, I would suggest removing it here and copying it to each single test case. I would also rename it to key.

"cases": [
{
"description": "It keeps submitted key",
Copy link
Member

Choose a reason for hiding this comment

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

What does this test do?

"property": "key",
"input_key": "cipher.key",
"output_key": "input_key"
},
{
"description": "It can encode",
"property": "encode",
"input_string": "aaaaaaaaaa",
Copy link
Member

Choose a reason for hiding this comment

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

Rename to input

"output_key": "abcdefghij"
},
{
"description": "It can decode",
"property": "decode",
"input_string": "abcdefghij",
Copy link
Member

Choose a reason for hiding this comment

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

Rename to input

"output_string": "aaaaaaaaaa"
Copy link
Member

Choose a reason for hiding this comment

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

Rename to expected

},
{
"description": "It is reversible",
"property": "encode -> decode",
"input_encode": "abcdefghij",
Copy link
Member

Choose a reason for hiding this comment

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

Rename to input

"output_decode": "abcdefghij"
Copy link
Member

Choose a reason for hiding this comment

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

Rename to expected

},
{
"description": "It can double shift encode",
"property": "key",
"cipher_key": "iamapandabear",
"case": {
"property": "encode",
"input_string": "iamapandabear",
Copy link
Member

Choose a reason for hiding this comment

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

Rename to input

"output_string": "qayaeaagacia"
Copy link
Member

Choose a reason for hiding this comment

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

Rename to expected

}
},
{
"description": "It can wrap on encode",
"property": "encode",
"input_string": "zzzzzzzzzz",
Copy link
Member

Choose a reason for hiding this comment

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

Rename to input

"output_string": "zabcdefghi"
Copy link
Member

Choose a reason for hiding this comment

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

Rename to expected

},
{
"description": "It can wrap on decode",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe shorten to Decode wraps input or something like that.

"property": "decode",
"input_string": "zabcdefghi",
Copy link
Member

Choose a reason for hiding this comment

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

Renamed to input

"output_string": "zzzzzzzzzz"
Copy link
Member

Choose a reason for hiding this comment

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

Rename to expected

},
{
"description": "It can handle messages longer than the key",
"property": "cipher.key",
Copy link
Member

Choose a reason for hiding this comment

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

I would use the encode property here and flatten the nested case (with its property removed).

"cipher_key": "abc",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just use key as the property?

"case": {
"property": "encode",
"input_string": "iamapandabear",
Copy link
Member

Choose a reason for hiding this comment

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

Rename to input

"output_string": "iboaqcnecbfcr"
Copy link
Member

Choose a reason for hiding this comment

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

Rename to expected

}
}
]
}
]
}