binary: split error case tests#305
Conversation
|
Thanks @tommyschaefer . In this case I believe -1 is inappropriate because it is a valid decimal number, and it is possible to represent negative numbers in binary. (Although this exercise is only concerned with positive binary numbers.) So I agree with what you've done to remove redundant test cases, nice work. |
| "description": "numbers other than one and zero raise an error", | ||
| "binary": ["012", "2"], | ||
| "expected": -1 | ||
| "description": "2 is not a valid binary digit and raises an error", |
There was a problem hiding this comment.
I'd remove 'and raises an error' from the descriptions.
The error raising is an extension added by the Ruby track.
The readme just says "The program should handle invalid inputs."
|
Looking good. 👍 |
|
@Insti Thank you! Would it be helpful if I squashed these commits? |
|
Yes please. |
* Update binary tests to make one assertion per test * Use null to indicate invalid input rather than -1
tommyschaefer
force-pushed
the
binary/split-error-test-cases
branch
from
01092b0 to
905d77a
Compare
|
Commits have been squashed and I updated the PR description so that if / when this gets merged, #303 should also be closed 😀 |
* Regenerate tests from latest exercism/problem-specifications#305 revision * Remove compound assertion logic from BinaryCases since all tests with multiple assertions have been split into separate tests. * Use JSON null to indicate invalid input
|
🤞 |
* Regenerate tests from latest exercism/problem-specifications#305 revision * Remove compound assertion logic from BinaryCases since all tests with multiple assertions have been split into separate tests. * Use JSON null to indicate invalid input
|
They didn't close automatically, but since you reminded me I remembered to do it manually. |
|
Hmm... I think this might have just gotten closed rather than merged. Is that intentional? And my pleasure, @Insti! 😄 |
|
Gak! Thanks for spotting that! |
|
Haha! No problem 😄 Thank you so much for all of your help and feedback 👍 |
2 participants
Resolves #303
Does this seem like it covers all of the cases in the original? It felt to me like a number of the grouped assertions were testing the same thing, so I didn't include them all. I'm totally open to adding them, I just couldn't think of distinct names for all of them 😄!
Also, there has been some discussion about what the
expectedvalue should be. Bothnulland-1seem appropriate to me. I only chose to go with-1because I saw it used in thehammingdata, so I assumed that was the standard. If this isn't the case, I'm happy to change it!Thank you so much for your time and feedback! 😄