binary: Add test data for "binary" exercise #296
Conversation
| }, | ||
| { | ||
| "description": "invalid binary numbers raise an error", | ||
| "binary": ["012", "10nope", "nope10", "10nope10", "001 nope", "2"], |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This might be done by extracting to an "errors" array to match the "cases/decimal" one above.
There was a problem hiding this comment.
Yeah, I think it would probably be worth having separate tests for different types of error.
|
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? |
|
I think it's fine to add it, in case it takes time for tracks to deprecate it. |
|
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:
and
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! 😄 |
|
@tommyschaefer thanks for your continued progress on this. Each thing you're checking for should have it's own (nameable) test case. 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. |
| "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"], |
There was a problem hiding this comment.
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
changed the title
|
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. |
That's a good solution! Thank you. |
binary: Add invalid edge case tests. Closes #275
Adds test data for "binary" exercise for exercism/ruby#394.
Thank you so much for your time!