Add triangle test generator by favrik · Pull Request #474 · exercism/ruby

favrik · 2016-10-26T20:13:46Z

Hello!,

this is for issue #472, thoughts? :)

Thanks!

Insti · 2016-10-26T20:41:27Z

Awesome, thanks, ❤️ I'll review this a bit later.

bmulvihill · 2016-10-27T01:00:34Z

@favrik Thanks for the PR! We have recently standardized our generator interface so please ensure the methods test_name, skipped and workload are defined, and use them within your example.tt

This has only been recently decided and here is the currently open PR for reference

Excerpt from that PR's README update:

All generators currently adhere to a common public interface, and must define the following three methods:

test_name - Output the name of the test
workload - Output the body of the test
skipped - Output skip syntax

bmulvihill · 2016-10-27T01:07:48Z

  end

-  def test_isosceles_triangles_have_first_and_last_sides_equal
+  def test_all_zero_sides_are_illegal__so_the_triangle_is_not_equilateral


I see a couple test names with two underscores

Oh yeah, decided to replace commas with underscores too, but I see is not that really necessary. xD Ty!

bmulvihill · 2016-10-27T01:11:08Z

+class TriangleTest < Minitest::Test<% test_cases.each do |test_case| %>
+  def <%= test_case.name %><% if test_case.skipped? %>
+    skip<% end %>
+    triangle = Triangle.new(<%= test_case.sides %>)<% if test_case.expected%>


I would consider moving some of this logic into the workload method once it is defined

Check out Hamming as an example as well. It will help to remove the logic in the view, at least for skip.

favrik · 2016-10-27T03:53:01Z

@bmulvihill totally missed that PR!

Thanks for the feedback!, I'll apply the updates to conform to the interface in a few hours. :)

* Conform to the test generator common public interface * Fix double underscores in method names * Remove unused TriangleError class

favrik · 2016-10-27T08:08:41Z

Updated, thanks!

Insti · 2016-10-27T08:22:32Z

-    "triangle.#{triangle}?"
+  def workload
+    "triangle = Triangle.new(#{sides})
+    #{assert_or_refute} triangle.#{triangle}?, #{failure_message}"


This relies on the indentation of the test case generator being the same as the indentation of the template file, Which is not a problem now, but is rather fragile.
Have a look at how Transpose handles indenting: #466

Ok, it took me a while to understand this, but I think I get it now. Basically, if the *_cases.rb file changes (due to some refactoring for example), the indentation can be affected, and now you have 1 more thing that needs to be adjusted. Thanks!

Insti · 2016-10-27T08:33:51Z

      replaced = triangle + ' triangle ' + initial
    end
-    'test_%s' % replaced.gsub(/[ ,-]/, '_')
+    'test_%s' % replaced.gsub(/[, -]/, '_').squeeze('_')


gsub(/[, -]/, '_').squeeze('_')
Ruby has a method that does this.
tr_s(', -','_')

Insti · 2016-10-27T08:40:34Z

  def failure_message
-    "Expected '#{expected}', triangle is #{expected ? '' : 'not '}#{triangle}."
+    "\"Expected '#{expected}', triangle is #{expected ? '' : 'not '}" +
+    "#{triangle}.\""


Rather than splitting this line, how about refactoring it to extract some named methods.
You can also use %Q to avoid having to escape your doublequotes.
%Q("Expected '#{expected}', triangle is #{expected_type}")

Insti

Looking good.
Edit: Looks like you replaced the commit I'd commented on :( The comments are still relevant I think.

favrik · 2016-10-27T14:39:54Z

@Insti I only added a new commit, all your comments are relevant, thanks very much for the review! :)

I'll add another commit with the suggested changes.

* Use indentation for workload method * Use tr_s instead of gsub * Use %Q() to simplify string

favrik · 2016-10-27T19:46:49Z

I've added @Insti suggestions in a new commit, and also extracted some duplicate strings to a method. Thanks for the reviews!! :)

kotp

The prior version seems to be "cleaner", perhaps this is a little over engineered?

favrik · 2016-10-31T16:31:24Z

Hi @kotp, I guess you are referring to the expected_type method and related logic in lib/triangle_cases.rb, if that's the case, I agree. On one hand I think it's okay-ish. On the other hand, I think it's a bit convoluted due to how I'm handling the canonical data. I can revisit in a couple of days, suggestions are definitely welcome. Thanks!

Insti · 2016-11-05T09:55:44Z

@favrik have you had a chance to revisit this?

favrik · 2016-11-05T23:21:28Z

@Insti I haven't 😞, but I just looked at it again and realized I had logic that is not needed. xD Added a new commit. Could use a pair of fresh eyes too! 😄

kotp · 2016-11-06T01:51:02Z

+  attr_reader :sides

-  def kind
-    fail TriangleError if illegal?


Are we no longer raising on an illegal triangle? I did not see a comment about this in the discussions here.

The canonical data for the triangle problem was recently updated to test for properties instead of its type. Thoughts?

kotp · 2016-11-06T01:52:35Z

    skip
-    assert_raises(TriangleError) do
-      Triangle.new(1, 3, 1).kind
-    end


Where did the tests for illegal triangles go?

Add triangle test generator

84e6d38

bmulvihill reviewed Oct 27, 2016

View reviewed changes

Update triangle test generator

fadf0ad

* Conform to the test generator common public interface * Fix double underscores in method names * Remove unused TriangleError class

Insti reviewed Oct 27, 2016

View reviewed changes

Insti approved these changes Oct 27, 2016

View reviewed changes

Add code review suggestions to triangle test generator

0ee3a93

* Use indentation for workload method * Use tr_s instead of gsub * Use %Q() to simplify string

kotp reviewed Oct 30, 2016

View reviewed changes

Remove redundant logic

5b000fa

kotp reviewed Nov 6, 2016

View reviewed changes

kotp merged commit 6e14659 into exercism:master Nov 21, 2016

Insti mentioned this pull request Nov 27, 2016

bob: setting up generated tests #493

Closed

tommyschaefer mentioned this pull request Dec 19, 2016

wordy: Generate tests #509

Merged

Uh oh!

Conversation

favrik commented Oct 26, 2016

Uh oh!

Insti commented Oct 26, 2016

Uh oh!

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

Uh oh!

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!

kotp Oct 27, 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!

favrik commented Oct 27, 2016

Uh oh!

favrik commented Oct 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Insti Oct 27, 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 Oct 27, 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 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!

favrik commented Oct 27, 2016

Uh oh!

favrik commented Oct 27, 2016

Uh oh!

kotp left a comment

Choose a reason for hiding this comment

Uh oh!

favrik commented Oct 31, 2016

Uh oh!

Insti commented Nov 5, 2016

Uh oh!

favrik commented Nov 5, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bmulvihill commented Oct 27, 2016 •

edited

Loading

kotp Oct 27, 2016 •

edited

Loading

Insti Oct 27, 2016 •

edited

Loading

Insti Oct 27, 2016 •

edited

Loading

Insti left a comment •

edited

Loading