Add triangle test generator #474
Conversation
|
Awesome, thanks, ❤️ I'll review this a bit later. |
|
@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:
|
| end | ||
|
|
||
| def test_isosceles_triangles_have_first_and_last_sides_equal | ||
| def test_all_zero_sides_are_illegal__so_the_triangle_is_not_equilateral |
There was a problem hiding this comment.
I see a couple test names with two underscores
There was a problem hiding this comment.
Oh yeah, decided to replace commas with underscores too, but I see is not that really necessary. xD Ty!
| 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%> |
There was a problem hiding this comment.
I would consider moving some of this logic into the workload method once it is defined
There was a problem hiding this comment.
Check out Hamming as an example as well. It will help to remove the logic in the view, at least for skip.
|
@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
|
Updated, thanks! |
| "triangle.#{triangle}?" | ||
| def workload | ||
| "triangle = Triangle.new(#{sides}) | ||
| #{assert_or_refute} triangle.#{triangle}?, #{failure_message}" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
| initial = description.downcase | ||
| replaced = initial.gsub(/(true|false)/, replacements) | ||
| if initial.eql?(replaced) && !initial.include?(triangle) | ||
| replaced = triangle + ' triangle ' + initial | ||
| end | ||
| 'test_%s' % replaced.gsub(/[ ,-]/, '_') | ||
| 'test_%s' % replaced.gsub(/[, -]/, '_').squeeze('_') |
There was a problem hiding this comment.
gsub(/[, -]/, '_').squeeze('_')
Ruby has a method that does this.
tr_s(', -','_')
| end | ||
|
|
||
| def failure_message | ||
| "Expected '#{expected}', triangle is #{expected ? '' : 'not '}#{triangle}." | ||
| "\"Expected '#{expected}', triangle is #{expected ? '' : 'not '}" + | ||
| "#{triangle}.\"" |
There was a problem hiding this comment.
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 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
|
I've added @Insti suggestions in a new commit, and also extracted some duplicate strings to a method. Thanks for the reviews!! :) |
|
Hi @kotp, I guess you are referring to the |
|
@favrik have you had a chance to revisit this? |
|
@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! 😄 |
|
|
||
| def kind | ||
| fail TriangleError if illegal? |
There was a problem hiding this comment.
Are we no longer raising on an illegal triangle? I did not see a comment about this in the discussions here.
There was a problem hiding this comment.
The canonical data for the triangle problem was recently updated to test for properties instead of its type. Thoughts?
| skip | ||
| assert_raises(TriangleError) do | ||
| Triangle.new(1, 3, 1).kind | ||
| end |
There was a problem hiding this comment.
Where did the tests for illegal triangles go?
Hello!,
this is for issue #472, thoughts? :)
Thanks!