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

Sum Of Multiples incorrect test case #256

Closed
jrossiter opened this issue Mar 12, 2016 · 6 comments · Fixed by #257
Closed

Sum Of Multiples incorrect test case #256

jrossiter opened this issue Mar 12, 2016 · 6 comments · Fixed by #257

Comments

@jrossiter
Copy link

In the varTests test cases for the Sum Of Multiples exercise it has:
{[]int{}, 10000, 0}

But the accompanying README.md file states:

If no set of numbers is given, default to 3 and 5.

Should this test case actually be the following or have I misread the description?
{[]int{}, 10000, 23331668}

@petertseng
Copy link
Member

You're right given the current state of the world.

It's worth noting for historical data why you're right and xgo is wrong. Go's tests were written in July 2014 before the default was added in December 2015 and not one but two people have had problems.

So we should change the test. It doesn't make much sense to make a Go-specific exception.

Did a quick survey of the languages. They somehow all use 3, 5 as well. Not going to try to trace back the lineage of that.

@kytrinyx
Copy link
Member

Having read through this and the older issue, I think that the right move is to improve the problem to remove the "defaults" requirement. I've opened an issue here: exercism/problem-specifications#198

In this case, I believe that the Go exercise is actually correct (though currently confusing because of the thing in the README). Should we just close this issue?

@petertseng
Copy link
Member

Well... then Go is correct in the sense of "this is where we would like to be eventually". Until all tracks with this exercise are consistent, one side or the other will be confusing (depending on which side the README supports).

So the answer might depend on: How fast will other tracks be able to remove their defaults, and thus when will we be able to remove the default from the README?

Basically it's weighing the costs of temporarily making Go conform to a state that is not the final state we desire versus the cost of confusing those who attempt this exercise in Go (I surveyed languages and Go's the only one that doesn't default).

I'm unsure of my answer to this question because I don't know when to expect exercism/problem-specifications#198 to be done. Stats say there's less than one submission a day.

Alternative could be to stick something in the go tests or something saying "ignore what the readme says about defaults. If no multiples are specified, there are none". I originally had this in my response, but I thought it was weird to have a Go-only exception. But now that I hear that we want this to be the actual behavior, then this could be a thing to do.

@kytrinyx
Copy link
Member

What if we remove the line in the README right now?

  • The Go exercise will not be contradicting the README.
  • The other tracks' exercises will not be directly contradicting to the README, but they will have a seemingly odd choice of extension, where the test guides the solution beyond what the README says.

Alternately, I think that your suggestion of sticking a comment in the Go tests is fine. We can even link to the issue as an explanation.

Having typed this out, I think I like your suggestion best.

@petertseng
Copy link
Member

The other tracks' exercises will not be directly contradicting to the README, but they will have a seemingly odd choice of extension, where the test guides the solution beyond what the README says.

Hmm, it seems that it can be quite difficult to understand from the tests what the desired defaults are. From looking at the Ruby tests for example, it might not be obvious without experimentation. Some may even be forced to read the example solution to figure that out.

I would have suggested immediately amending the README had it not been for that.

@kytrinyx
Copy link
Member

That's a good point. OK, let's go with a comment in the Go test suite. I'll do that now.

kytrinyx added a commit that referenced this issue Mar 13, 2016
Clarify the discrepancy between the Go implementation and the current README.
This fixes #256.
robphoenix pushed a commit to robphoenix/exercism-go that referenced this issue Dec 22, 2016
default values are no longer mentioned in the README, so this note is no
longer necessary.

exercism#256
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 a pull request may close this issue.

3 participants