Skip to content

binary: Add test data for "binary" exercise#296

Merged
Insti merged 3 commits intoexercism:masterfrom
tommyschaefer:master
Jul 25, 2016
Merged

binary: Add test data for "binary" exercise#296
Insti merged 3 commits intoexercism:masterfrom
tommyschaefer:master

Conversation

@tommyschaefer
Copy link
Copy Markdown
Contributor

Adds test data for "binary" exercise for exercism/ruby#394.

Thank you so much for your time!

Comment thread binary.json Outdated
},
{
"description": "invalid binary numbers raise an error",
"binary": ["012", "10nope", "nope10", "10nope10", "001 nope", "2"],
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 think that the error cases should be split into individual tests, each of them deserves a description of why it is an invalid input which will help when coding the solution.

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.

This might be done by extracting to an "errors" array to match the "cases/decimal" one above.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, I think it would probably be worth having separate tests for different types of error.

@NobbZ
Copy link
Copy Markdown
Member

NobbZ commented Jul 15, 2016

We are currently in the process of deprecating binary along some exercises that are very similar to it. Do we really need an update of the common tests here?

@kytrinyx
Copy link
Copy Markdown
Member

I think it's fine to add it, in case it takes time for tracks to deprecate it.

@tommyschaefer
Copy link
Copy Markdown
Contributor Author

tommyschaefer commented Jul 25, 2016

Hi Everyone! I've made a few adjustments to the test data based on the feedback above.

I'm not entirely sure that I correctly implemented the change @Insti and @kytrinyx recommended:

I think that the error cases should be split into individual tests, each of them deserves a description of why it is an invalid input which will help when coding the solution.

and

Yeah, I think it would probably be worth having separate tests for different types of error.

Did you mean to break each error case into it's own test case, or to just split the different "kinds" of error into different cases?

So sorry for the confusion and thank you so much for the feedback! 😄

@Insti
Copy link
Copy Markdown
Contributor

Insti commented Jul 25, 2016

@tommyschaefer thanks for your continued progress on this.

Each thing you're checking for should have it's own (nameable) test case.
The things being tested should NOT be in arrays.

If you cannot think of a distinct name, this is a sign that you might not be testing anything useful.

Edit: I've added some line comments to the commit to help clarify.

Comment thread binary.json
"description": "invalid binary numbers raise an error",
"binary": ["012", "10nope", "nope10", "10nope10", "001 nope", "2"],
"description": "numbers other than one and zero raise an error",
"binary": ["012", "2"],
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.

This should become 2 tests,

         "description": "2 is not a valid binary digit and so should raise an error",
         "binary":  "2",
         "expected" :  null,

and

         "description": "A number containing an invalid binary digit is invalid and should raise an error",
         "binary":  "01201",
         "expected":  null,

Discussion required: I'm not 100% sure null is the correct expected value here if we want to indicate in a general way that an error should be raised.

@Insti Insti changed the title Add test data for "binary" exercise binary: Add test data for "binary" exercise Jul 25, 2016
@Insti Insti merged commit 80c7f7b into exercism:master Jul 25, 2016
@Insti
Copy link
Copy Markdown
Contributor

Insti commented Jul 25, 2016

I got a bit trigger happy on merge for the x-ruby generator before this was finalised. In order to not have the x-ruby build break, I've merged this in it's current state and will create a new issue for the test case splitting.

@kytrinyx
Copy link
Copy Markdown
Member

I've merged this in it's current state and will create a new issue for the test case splitting.

That's a good solution! Thank you.

emcoding pushed a commit that referenced this pull request Nov 19, 2018
binary: Add invalid edge case tests. Closes #275
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.

4 participants