Skip to content

Conversation

cmccandless
Copy link
Contributor

Currently, there are no tests confirming that a game with bonus rolls for a spare or strike in the 10th frame is complete. This PR adds a case for both spare and strike, attempting to roll an additional ball after the bonus rolls have been completed, expecting an error to be raised upon the roll attempt.

For confirmation that these are not currently tested for, the current example solution in the Python track passes all existing tests, but fails these new tests.

Currently, there are no tests confirming that a game with bonus rolls for a spare or strike in the 10th frame is complete. This PR adds a case for both spare and strike, attempting to roll an additional ball after the bonus rolls have been completed, expecting an error to be raised upon the roll attempt.
Copy link
Member

@rpottsoh rpottsoh left a comment

Choose a reason for hiding this comment

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

I think "previousRolls" needs to be updated for these two proposed additional cases.

"description": "cannot roll after bonus roll for spare",
"property": "roll",
"input": {
"previousRolls": [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 7, 3, 2],
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I am wrong here. But the 2nd ball in the 10th is a spare, which rewards the roller with 1 bonus roll, 2, yielding a score of 12.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You read it correctly. However, perhaps you missed the roll input field? In case there is any confusion, try pairing the rolls into frames.

Frame No Ball 1 Ball 2
1 0 0
2 0 0
3 0 0
4 0 0
5 0 0
6 0 0
7 0 0
8 0 0
9 0 0
10 7 3
Bonus 2 (valid) 2 (from "input": { "roll": 2 })

"description": "cannot roll after bonus rolls for strike",
"property": "roll",
"input": {
"previousRolls": [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 10, 3, 2],
Copy link
Member

Choose a reason for hiding this comment

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

A strike in the 10th rewards the roller with two extra rolls. I count 15 in the 10th.

Copy link
Member

Choose a reason for hiding this comment

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

I have called these bonus rolls, the test description calls them fill balls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, you counted correctly but neglected the roll input field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have called these bonus rolls, the test description calls them fill balls.

Other tests already referred to them as bonus rolls, so I remained consistent the the other tests. Perhaps another PR would be in order to make the description and tests consistent, but that is another issue.

@rpottsoh
Copy link
Member

Test Description.

Copy link
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

I believe the cases are correct (if I am wrong, then two people made a same mistake and I made a mistake in https://github.com/petertseng/exercism-problem-specifications/blob/verify/exercises/bowling/verify.rb), the version is incremented appropriately, and this seems like an OK place for these tests.

Anyone observed a submitted implementation that behaves incorrectly under these new tests? So many tests that we had test for being able to score the game at the right time (when the game is complete) and possibly hopefully implementations reached the conclusion of not being able to roll exactly when the game is complete, but perhaps some implementation misses it.

@rpottsoh
Copy link
Member

I understand my err.

@rpottsoh rpottsoh merged commit 1806718 into master Feb 15, 2018
@rpottsoh rpottsoh deleted the cmccandless-patch-1 branch February 15, 2018 21:00
@rpottsoh
Copy link
Member

Thanks @cmccandless!

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.

4 participants