Replace hard-coded Bowling tests with test generator (Fixes #456) by gchan · Pull Request #465 · exercism/ruby

gchan · 2016-10-19T06:37:21Z

As per #456

Also updated the example solution to address new test case

Feedback is appreciated. I ran rubocop but I'm uncertain if it actually was able to detect offences (I deliberately indented incorrect to test rubocop).

Insti · 2016-10-19T06:51:21Z

Awesome, thanks.
I'll have a look through this shortly.

The current rubocop config is very permissive and doesn't detect much.

Insti · 2016-10-19T06:54:07Z

-    assert_respond_to @game, :roll
-    assert_equal 1, @game.method(:roll).arity
+  def roll(rolls)
+    rolls.each { |pins| @game.roll(pins) }


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

Insti · 2016-10-19T07:02:09Z

+    rolls.each { |pins| @game.roll(pins) }
+  end
+<% test_cases.each do |test_case| %>
+  def <%= test_case.name %><% if test_case.skipped? %>


Try to keep logic out of the template file.
Have a look at the PR for anagram generator: #464 for some examples of how to do this.

Basically the test template should look like:

def <%= test_case.test_name %> <%= test_case.skipped %> <%= test_case.work_load %> end

Also we're trying to standardise the method names in generators, so using test_name, skipped, work_load is preferred.

Insti

A good first pass, the generated file looks good. 👍

I just like to see the logic moved from the template to the cases file.

Nice work.

Edit: That big red circle looks scary, but it shouldn't 😃

gchan · 2016-10-19T07:31:40Z

Thanks, I have moved the logic into the cases file. Do you have any ideas how I could improve work_load?

Insti · 2016-10-19T08:07:34Z

+  end
+
+  def work_load
+    if assert_error?


You're checking assert_error? twice, (again on line 30)
What if you moved all this logic into the assert method?

Oops, the assert_error? conditional check isn't required in assert.

If we moved the logic to the assert method, we will still have one assert_error? check?

Insti · 2016-10-19T08:09:43Z

+  end
+
+  def assert
+    "assert_equal #{expected}, @game.score" unless assert_error?


hint: assert could return an array of lines that get joined later..

Something like this?

def assert if assert_error? [ "assert_raises StandardError do", " #{roll}", " @game.score", "end" ] else [roll, "assert_equal #{expected}, @game.score"] end end

Also updated the example solution to address new test case

Insti · 2016-10-19T20:20:08Z

Thanks @gchan, This looks much better now.

I'd personally refactor assert but it works fine as it is and I'm trying to cut down on micro-managing.

Nice work updating the example.rb to pass the tests. 👍

kotp · 2016-10-19T23:12:42Z

For Running rubocop with your own configuration (or a default) you can use this:

Style/Documentation:       
  Exclude:                 
    - './spec/**/*' 
    - './test/**/*'

Copy that and create a file named, default_rubocop.yml or whatever you want to name it, wherever you want to store it.

Then invoke rubocop -c where_I_stored_it/default_rubocop.yml and you will get the default cops to run...

Right now, the cops aren't really reporting anything (I would have to look to be absolutely sure, of course) so this gives you a way to use the tool in a manner that is more helpful than Bob (Jan says, 'Bob, tell me what you think of my code style.!', Bob says "Whatever.").

gchan · 2016-10-20T06:55:54Z

Thanks for the feedback @Insti . The change to move the logic out of the view was a good idea.

Also thanks @kotp for the tip for running rubocop with default or custom configuration.

kotp · 2016-10-20T11:57:58Z

Up to you @gchan if you do want to do the quick updates based on feedback from Rubocop. Or we can bring it in, according to me. @Insti has been looking at it closer than I have, I think. I will leave it up to his and your decision to bring it in, either as it stands or with those slight modifications.

gchan · 2016-10-20T23:26:18Z

There are some Rubocop violations that can be addressed in lib/bowling_cases.rb and exercises/bowling/example.rb. I can look at making those slight modifications tomorrow before we bring it in.

kotp · 2016-10-21T09:35:27Z

Thanks @gchan.

gchan · 2016-10-22T00:21:24Z

Corrected some Rubocop violations in a separate commit. Let me know if you need me to squash the commits.

kotp · 2016-10-22T04:50:06Z

The way the two commits are now, one is style related, the other is the actual "Cargo". So these two commits are good unsquashed, if you like. Together, you would not have to show the changes. It is up to you. But the style changes may be "noise" rather than "signal".

Insti · 2016-10-22T09:49:06Z


  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)


According to this 1.5 is a valid number of pins.

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

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.

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?

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.

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.

gchan · 2016-10-23T05:59:35Z

You're right about the style changes being 'noise' and is better off as a separate commit.

Is there anything else I need to do on this PR to bring it in?

Insti · 2016-10-23T15:55:05Z

-    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


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.

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.

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.

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.

Insti · 2016-10-24T17:49:20Z

Thanks for all your work on this @gchan ❤️

gchan · 2016-10-24T21:43:22Z

No problem. Thanks for the feedback. I learnt a few things along the way

Insti reviewed Oct 19, 2016

View reviewed changes

Insti suggested changes Oct 19, 2016

View reviewed changes

gchan force-pushed the generate-bowling-tests branch from c980e66 to 5742c74 Compare October 19, 2016 07:28

gchan force-pushed the generate-bowling-tests branch from 5742c74 to f1ef7ab Compare October 19, 2016 07:31

Insti reviewed Oct 19, 2016

View reviewed changes

Replace hard-coded Bowling tests with test generator (exercism#456)

1cd441f

Also updated the example solution to address new test case

gchan force-pushed the generate-bowling-tests branch from f1ef7ab to 1cd441f Compare October 19, 2016 08:16

Insti approved these changes Oct 19, 2016

View reviewed changes

Insti added the ready label Oct 19, 2016

Insti added in progress and removed ready labels Oct 21, 2016

Insti changed the title ~~Replace hard-coded Bowling tests with test generator~~ Replace hard-coded Bowling tests with test generator (Fixes #456) Oct 21, 2016

Correct some Rubocop violations for bowling exercise

6514708

Insti reviewed Oct 22, 2016

View reviewed changes

Insti reviewed Oct 23, 2016

View reviewed changes

Insti merged commit 142093e into exercism:master Oct 24, 2016

Insti removed the in progress label Oct 24, 2016

Uh oh!

Conversation

gchan commented Oct 19, 2016

Uh oh!

Insti commented Oct 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Insti left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gchan commented Oct 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gchan Oct 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Insti commented Oct 19, 2016

Uh oh!

kotp commented Oct 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gchan commented Oct 20, 2016

Uh oh!

kotp commented Oct 20, 2016

Uh oh!

gchan commented Oct 20, 2016

Uh oh!

kotp commented Oct 21, 2016

Uh oh!

gchan commented Oct 22, 2016

Uh oh!

kotp commented Oct 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gchan commented Oct 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gchan Oct 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Insti commented Oct 24, 2016

Uh oh!

gchan commented Oct 24, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

Insti left a comment •

edited

Loading

gchan Oct 19, 2016 •

edited

Loading

kotp commented Oct 19, 2016 •

edited

Loading

gchan Oct 24, 2016 •

edited

Loading