Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions exercises/bowling/.version
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
2
242 changes: 109 additions & 133 deletions exercises/bowling/bowling_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,224 +3,200 @@
require 'minitest/autorun'
require_relative 'bowling'

class GameTest < Minitest::Test
# Test data version:
# 0a51cfc
class BowlingTest < Minitest::Test
def setup
@game = Game.new
end

def test_must_be_able_to_roll_with_number_of_pins
assert_respond_to @game, :roll
assert_equal 1, @game.method(:roll).arity
def roll(rolls)
rolls.each { |pins| @game.roll(pins) }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that you've extracted this helper method. Good variable naming too 👍

end

def test_must_have_a_score
skip
assert_respond_to @game, :score
def test_should_be_able_to_score_a_game_with_all_zeros
# skip
roll([0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0])
assert_equal 0, @game.score
end

def test_should_be_able_to_score_open_frame
def test_should_be_able_to_score_a_game_with_no_strikes_or_spares
skip
@game.roll(3)
@game.roll(4)
roll_n_times(18, 0)
assert_equal 7, @game.score
roll([3, 6, 3, 6, 3, 6, 3, 6, 3, 6, 3, 6, 3, 6, 3, 6, 3, 6, 3, 6])
assert_equal 90, @game.score
end

def test_should_be_able_to_score_multiple_frames
def test_a_spare_followed_by_zeros_is_worth_ten_points
skip
[3, 4, 2, 3, 5, 2].each do |pins|
@game.roll pins
end
roll_n_times(14, 0)
assert_equal 19, @game.score
roll([6, 4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0])
assert_equal 10, @game.score
end

def test_should_score_a_game_with_all_gutterballs
def test_points_scored_in_the_roll_after_a_spare_are_counted_twice
skip
roll_n_times(20, 0)
assert_equal 0, @game.score
roll([6, 4, 3, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0])
assert_equal 16, @game.score
end

def test_should_score_a_game_with_all_single_pin_rolls
def test_consecutive_spares_each_get_a_one_roll_bonus
skip
roll_n_times(20, 1)
assert_equal 20, @game.score
roll([5, 5, 3, 7, 4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0])
assert_equal 31, @game.score
end

def test_should_allow_game_with_all_open_frames
def test_a_spare_in_the_last_frame_gets_a_one_roll_bonus_that_is_counted_once
skip
roll_n_times(10, [3, 6])
assert_equal 90, @game.score
roll([0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 7, 3, 7])
assert_equal 17, @game.score
end

def test_should_correctly_score_a_strike_that_is_not_on_the_last_frame
def test_a_strike_earns_ten_points_in_frame_with_a_single_roll
skip
@game.roll(10)
@game.roll(5)
@game.roll(3)
roll_n_times(16, 0)

assert_equal 26, @game.score
roll([10, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0])
assert_equal 10, @game.score
end

def test_should_score_a_spare_that_is_not_on_the_last_frame
def test_points_scored_in_the_two_rolls_after_a_strike_are_counted_twice_as_a_bonus
skip
@game.roll(5)
@game.roll(5)
@game.roll(3)
@game.roll(4)
roll_n_times(16, 0)

assert_equal 20, @game.score
roll([10, 5, 3, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0])
assert_equal 26, @game.score
end

def test_should_score_multiple_strikes_in_a_row
def test_consecutive_strikes_each_get_the_two_roll_bonus
skip
@game.roll(10)
@game.roll(10)
@game.roll(10)
@game.roll(5)
@game.roll(3)
roll_n_times(12, 0)

roll([10, 10, 10, 5, 3, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0])
assert_equal 81, @game.score
end

def test_should_score_multiple_spares_in_a_row
def test_a_strike_in_the_last_frame_gets_a_two_roll_bonus_that_is_counted_once
skip
@game.roll(5)
@game.roll(5)
@game.roll(3)
@game.roll(7)
@game.roll(4)
@game.roll(1)
roll_n_times(14, 0)

assert_equal 32, @game.score
roll([0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 10, 7, 1])
assert_equal 18, @game.score
end

def test_should_allow_fill_balls_when_the_final_frame_is_strike
def test_rolling_a_spare_with_the_two_roll_bonus_does_not_get_a_bonus_roll
skip
roll_n_times(18, 0)
@game.roll(10)
@game.roll(7)
@game.roll(1)

assert_equal 18, @game.score
roll([0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 10, 7, 3])
assert_equal 20, @game.score
end

def test_should_allow_fill_ball_in_last_frame_if_spare
def test_strikes_with_the_two_roll_bonus_do_not_get_bonus_rolls
skip
roll_n_times(18, 0)
@game.roll(9)
@game.roll(1)
@game.roll(7)

assert_equal 17, @game.score
roll([0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 10, 10, 10])
assert_equal 30, @game.score
end

def test_should_allow_fill_balls_to_be_strike
def test_a_strike_with_the_one_roll_bonus_after_a_spare_in_the_last_frame_does_not_get_a_bonus
skip
roll_n_times(18, 0)
@game.roll(10)
@game.roll(10)
@game.roll(10)

assert_equal 30, @game.score
roll([0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 7, 3, 10])
assert_equal 20, @game.score
end

def test_should_score_a_perfect_game
def test_all_strikes_is_a_perfect_game
skip
roll_n_times(12, 10)
roll([10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10])
assert_equal 300, @game.score
end

def test_should_not_allow_rolls_with_negative_pins
def test_rolls_can_not_score_negative_points
skip
assert_raises(
RuntimeError,
'Pins must have a value from 0 to 10') do
@game.roll(-1)
end
assert_raises StandardError do
roll([-1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0])
@game.score
end
end

def test_should_not_allow_rolls_better_than_strike
def test_a_roll_can_not_score_more_than_10_points
skip
assert_raises(
RuntimeError,
'Pins must have a value from 0 to 10') do
@game.roll(11)
end
assert_raises StandardError do
roll([11, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0])
@game.score
end
end

def test_should_not_allow_two_normal_rolls_better_than_strike
def test_two_rolls_in_a_frame_can_not_score_more_than_10_points
skip
assert_raises RuntimeError, 'Pin count exceeds pins on the lane' do
@game.roll(5)
@game.roll(6)
assert_raises StandardError do
roll([5, 6, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0])
@game.score
end
end

def test_should_not_allow_two_normal_rolls_better_than_strike_in_last_frame
def test_two_bonus_rolls_after_a_strike_in_the_last_frame_can_not_score_more_than_10_points
skip
roll_n_times(18, 0)
assert_raises RuntimeError, 'Pin count exceeds pins on the lane' do
@game.roll(10)
@game.roll(5)
@game.roll(6)
assert_raises StandardError do
roll([0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 10, 5, 6])
@game.score
end
end

def test_should_not_allow_to_take_score_at_the_beginning
def test_an_unstarted_game_can_not_be_scored
skip
assert_raises(
RuntimeError,
'Score cannot be taken until the end of the game',
) do
assert_raises StandardError do
roll([])
@game.score
end
end

def test_should_not_allow_to_take_score_before_game_has_ended
def test_an_incomplete_game_can_not_be_scored
skip
roll_n_times(19, 5)
assert_raises(
RuntimeError,
'Score cannot be taken until the end of the game') do
@game.score
end
assert_raises StandardError do
roll([0, 0])
@game.score
end
end

def test_should_not_allow_rolls_after_the_tenth_frame
def test_a_game_with_more_than_ten_frames_can_not_be_scored
skip
roll_n_times(20, 0)
assert_raises(
RuntimeError,
'Should not be able to roll after game is over',
) do
@game.roll(0)
assert_raises StandardError do
roll([0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0])
@game.score
end
end

def test_should_not_calculate_score_before_fill_balls_have_been_played
def test_bonus_rolls_for_a_strike_in_the_last_frame_must_be_rolled_before_score_can_be_calculated
skip
roll_n_times(10, 10)
assert_raises StandardError do
roll([0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 10])
@game.score
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error you are detecting is raised by the roll method, the way it is called hides this.
I'd like to see it made explicit that game.roll is what is raising the exception.

The way it is now @game.score is never executed, but it's still sitting there in the test looking like it is what will cause the problem. This is very misleading.

Copy link
Copy Markdown
Contributor Author

@gchan gchan Oct 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the test cases expect an exception to be raised by game.roll and an invalid roll while some are raised by game.score when the game is incomplete.

As there is no data in x-common to indicate whether the exception could be raised by score or by roll, it is difficult to procedurally generate test cases to include or exclude game.score from test cases. I'm open to any ideas.

The x-common data for exceptions sets the expected value as -1 and the decision of when to error is left up to the implementation (error when an invalid roll is made, error when scoring a game with an invalid roll, or error in both cases). With the current test cases, an implemented solution may decide to only error on game.score, game.roll, or both. This way a person could implement a solution that never errors on roll but only error on score.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for that good explanation.

I think you've made the right decision and we need to leave the tests the way they currently are.

We can revisit this if necessary if/when the canonical data is updated.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just hit the first review for bowling. The topic of discussion will be on "custom errors" as the solution is good... Thanks @gchan and @Insti for getting these changes in.

end
end

assert_raises RuntimeError, 'Game is not yet over, cannot score!' do
def test_both_bonus_rolls_for_a_strike_in_the_last_frame_must_be_rolled_before_score_can_be_calculated
skip
assert_raises StandardError do
roll([0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 10, 10])
@game.score
end
end

def roll_n_times(rolls, pins)
rolls.times do
Array(pins).each { |value| @game.roll(value) }
def test_bonus_roll_for_a_spare_in_the_last_frame_must_be_rolled_before_score_can_be_calculated
skip
assert_raises StandardError do
roll([0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 7, 3])
@game.score
end
end
private :roll_n_times

# Don't forget to define a constant VERSION inside of BookKeeping.
# Problems in exercism evolve over time, as we find better ways to ask
# questions.
# The version number refers to the version of the problem you solved,
# not your solution.
#
# Define a constant named VERSION inside of the top level BookKeeping
# module, which may be placed near the end of your file.
#
# In your file, it will look like this:
#
# module BookKeeping
# VERSION = 1 # Where the version number matches the one in the test.
# end
#
# If you are curious, read more about constants on RubyDoc:
# http://ruby-doc.org/docs/ruby-doc-bundle/UsersGuide/rg/constants.html

def test_bookkeeping
skip
assert_equal 1, BookKeeping::VERSION
assert_equal 2, BookKeeping::VERSION
end
end
14 changes: 7 additions & 7 deletions exercises/bowling/example.rb
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
module BookKeeping
VERSION = 1
VERSION = 2
end

class Game
RULES = { MIN: 0, MAX: 10 }.freeze
PINS = { MIN: 0, MAX: 10 }.freeze
at_exit { public :roll, :score }

private
Expand All @@ -20,16 +20,16 @@ def roll(pins)
end

def validate(pins)
raise 'Invalid number of pins' unless (RULES[:MIN]..RULES[:MAX]).cover?(pins)
raise 'Invalid number of pins' unless (PINS[:MIN]..PINS[:MAX]).cover?(pins)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to this 1.5 is a valid number of pins.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The correctness of example.rb is not super important, just something I noticed while I was looking through the code.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. Do we test for this? I don't think it is an important aspect in relation to what things are learned from the exercise. But definitely fodder for discussion in solutions!

I think when we were putting this together we didn't even consider it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. I guess the xcommon data doesn't test for non-integer numbers given that it needs to support languages that are typed?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think for this aspect, the discussion should go to x-common... find out if this is an aspect for others, and if we can change it in common.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The kata is about scoring bowling, not worrying if your test inputs are valid. So I would argue against adding test cases for it.
It is something that could be discussed during solution reviews.

raise 'Too many pins in frame' unless valid_frame?(pins)
raise 'Game is over, no rolls allowed' if game_complete?
end

def valid_frame?(pins)
last_roll_was_strike = @score_card[current_frame].last == 10

(last_frame? && last_roll_was_strike) ||
@score_card[current_frame].last.to_i + pins <= RULES[:MAX]
(last_frame? && last_roll_was_strike || spare?) ||
@score_card[current_frame].last.to_i + pins <= PINS[:MAX]
end

def score
Expand All @@ -40,10 +40,10 @@ def score
end

def score_frame(f, i)
strike_or_spare = [f.first, f.inject(:+)].any? {|e| e == RULES[:MAX]}
strike_or_spare = [f.first, f.inject(:+)].any? { |e| e == PINS[:MAX] }

if strike_or_spare
special(@score_card[i], @score_card[i+1], @score_card[i+2])
special(@score_card[i], @score_card[i + 1], @score_card[i + 2])
else
f.reduce(:+)
end
Expand Down
Loading