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

Add sum-of-multiples.json #206

Merged
merged 1 commit into from
Mar 22, 2016
Merged

Add sum-of-multiples.json #206

merged 1 commit into from
Mar 22, 2016

Conversation

petertseng
Copy link
Member

This is a synthesis of the existing tests in current tracks.

A short attempt to discern the history of the various tests:

Almost all tracks have the [3, 5] cases in common, as well as the
[7, 13, 17] and [43, 47] case. Indeed, these cases were present since
the first known sighting of sum-of-multiples at
exercism/exercism@607be68.

The [3, 5] to 100 case was added after discussion in
https://github.com/exercism/todo/issues/20 and
exercism/exercism#2486. Most tracks had
adopted them at the time of this PR.

The [4, 6] and [5, 6, 8] cases arose from discussion in
exercism/exercism#2341 - some tracks, but
not all, had adopted these tests at the time of this PR.

The [5, 25] case was a Haskell-specific case added in
exercism/haskell#50.

The [] and [1] cases were Go-specific cases that have been present ever
since the exercise was added to the Go track in
exercism/go#53.

Cases found in various tracks that were not included:

The [1] case of Go was originally a [1, 1] case but this is a conscious
decision not to test duplicate factors.

There were [0] and [0, 1] cases added to the Python track in
exercism/python#149 but this is a conscious
decision not to test 0 as a factor.

This is the second subtask of #198.

Closes https://github.com/exercism/todo/issues/154

@petertseng
Copy link
Member Author

Discussion is welcomed on whether to include tests for:

  • Having duplicates in the list of factors
  • Having 0 in the list of factors

@petertseng
Copy link
Member Author

Verified no mistakes in this file:

require 'json'

cases = JSON.parse(IO.read('sum-of-multiples.json'))

cases['cases'].each { |c|
  expected = c['expected']
  factors = c['factors']
  limit = c['limit']
  observed = (1...limit).select { |n|
    factors.any? { |f| n % f == 0 }
  }.reduce(0, :+)

  raise "Nope on #{c}, got #{observed}" if observed != expected
}

puts 'pass'

@kytrinyx
Copy link
Member

I think that it's the right move to not test duplicate factors. Also, 0 is a really weird case, with the identify for multiplication being 1 and I'd rather not complicate this exercise with it.

I think that this PR is good as is, and would love a second (and third) opinion.

This is a synthesis of the existing tests in current tracks.

A short attempt to discern the history of the various tests:

Almost all tracks have the [3, 5] cases in common, as well as the
[7, 13, 17] and [43, 47] case. Indeed, these cases were present since
the first known sighting of sum-of-multiples at
exercism/exercism@607be68.

The [3, 5] to 100 case was added after discussion in
https://github.com/exercism/todo/issues/20 and
exercism/exercism#2486. Most tracks had
adopted them at the time of this PR.

The [4, 6] and [5, 6, 8] cases arose from discussion in
exercism/exercism#2341 - some tracks, but
not all, had adopted these tests at the time of this PR.

The [5, 25] case was a Haskell-specific case added in
exercism/haskell#50.

The [] and [1] cases were Go-specific cases that have been present ever
since the exercise was added to the Go track in
exercism/go#53.

Cases found in various tracks that were not included:

The [1] case of Go was originally a [1, 1] case but this is a conscious
decision not to test duplicate factors.

There were [0] and [0, 1] cases added to the Python track in
exercism/python#149 but this is a conscious
decision not to test 0 as a factor.

This is the second subtask of #198.

Closes https://github.com/exercism/todo/issues/154
@petertseng
Copy link
Member Author

Thanks for review. Just FYI I'm changing the commit message (the json file contents haven't changed) so as to reference https://github.com/exercism/todo/issues/20, as I missed it in my history exploration.

We'll want to change the README (first subtask of #198) when tracks start adopting these, but that can wait a bit (and is an easy change to make in any case). Will probably emblazon the tracks; about time I gave that a try.

kytrinyx added a commit that referenced this pull request Mar 22, 2016
@kytrinyx kytrinyx merged commit 411222f into exercism:master Mar 22, 2016
@kytrinyx
Copy link
Member

Excellent! Let me know if you run into any weirdness with blazon.

@petertseng petertseng deleted the sum-of-multiples branch March 22, 2016 19:51
petertseng added a commit that referenced this pull request Oct 16, 2018
sum-of-multiples 1.4.1

As stated in our guidelines, we want descriptions that tell us why each
case is being tested:
https://github.com/exercism/problem-specifications#test-data-format-canonical-datajson

The canonical data was added in
#206 during a
time when descriptions were not required.

Then, placeholder descriptions were added in
aaa47ee and it was acknowledged at the
time that these descriptions were purely for *schema* compliance, and
not compliance with the guidelines nor a motivation for each case.

By examining the issues that led to each case being added, it is
possible to understand the descriptions.

With more cases being added to sum-of-multiples with clear descriptions,
it's time to bring the other descriptions up to speed as well.

Original exercise:
exercism/exercism@607be68

Additional cases:
exercism/exercism#2486
exercism/exercism#2341
exercism/haskell#50
exercism/go#53
#896
emcoding pushed a commit that referenced this pull request Nov 19, 2018
…nerators

Included test messages in hamming template
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.

2 participants