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

Allergies "invalid" input test is confusing #275

Closed
samjonester opened this issue Aug 22, 2016 · 3 comments
Closed

Allergies "invalid" input test is confusing #275

samjonester opened this issue Aug 22, 2016 · 3 comments

Comments

@samjonester
Copy link
Contributor

I'm opening this issue for discussion, as I'm not sure what the best solution is, and would like some feedback before opening a PR.

The test case below is confusing. It doesn't make sense why the expected response for an "invalid" input is all by one of the allowed data types. If we're going to expect error handling of this manner, then at a minimum, it should be documented better in the Readme. I'd prefer to just drop this test case, as I don't think it adds value to this exercise, though.

testCase "ignore non allergen score parts" $
    [ Eggs, Shellfish, Strawberries, Tomatoes, Chocolate
    , Pollen, Cats ] @=? allergies 509
@petertseng
Copy link
Member

It doesn't make sense why the expected response for an "invalid" input is all by one of the allowed data types.

It is something that I can explain, though the explanation won't necessarily make it make sense:

Prelude Data.Bits> 509 .&. 255
253

So the answer for 509 is the same as what it would be for 253.

If we're going to expect error handling of this manner, then at a minimum, it should be documented better in the Readme.

Agreed. The place to discuss is at exercism/problem-specifications#224

I'd prefer to just drop this test case, as I don't think it adds value to this exercise, though.

In this case, the place to make the PR would be https://github.com/exercism/x-common/blob/master/allergies.json (in addition to this track)

@samjonester
Copy link
Contributor Author

Thanks for the explanation, @petertseng! That makes sense. It does leads me to believe that the test was written for the specific implementation, which I think is something that is anti-TDD. From that explanation, I'm leaning towards your second suggestion, submitting a PR to allergies.json. Let's see where this conversation leads :)

@rbasso
Copy link
Contributor

rbasso commented Oct 3, 2016

It is not closed yet, but exercism/problem-specifications#224 was solved by merging exercism/problem-specifications#389, so I think it is safe to close this issue now that description.md explains the behavior of testing for 509.

Of course, I'll reopen it immediately if there is anything else to be discussed here.

@rbasso rbasso closed this as completed Oct 3, 2016
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

No branches or pull requests

3 participants