grains: add test generator #514

tommyschaefer · 2016-12-24T19:18:44Z

Hi there!

Checking off exercises from #396.

I tried to include tests for the generator this time around. Is this okay? If so, should I add a cases (or another name) test directory to group the tests? Or is it okay as it is?

I also had a couple other questions, but I'll add them as comments on specific lines.

Thank you so much for your time!

tommyschaefer · 2016-12-24T19:21:24Z

exercises/grains/example.rb

 module Grains
  def self.square(number)
+    fail ArgumentError if number <= 0 || number > 64


I had to make an adjustment to the implementation because the shared test data includes some error cases that weren't in the original ruby implementation.

How does this work with book keeping when it wasn't there originally? Normally, I'm assuming we would bump the BookKeeping::VERSION, but this exercise wasn't using book keeping yet.

The adjustment looks reasonable.

BookKeeping is related to the tests, so you might need to add it in so that the example.rb still passes the tests.

tommyschaefer · 2016-12-24T19:22:58Z

exercises/grains/example.rb

    2**(number - 1)
  end

  def self.total
-    square(65) - 1
+    2**64 - 1


Because of the change to the square method, calling square(65) would raise an ArgumentError, so I just made the smallest change possible to make the tests pass. Is this okay?

This is fine.

tommyschaefer · 2016-12-24T19:26:45Z

exercises/grains/grains_test.rb

    skip
-    assert_equal 32_768, Grains.square(16)
+    assert_equal 32768, Grains.square(16)


I elected to use the32769 notation rather than 32_768 for simplicities sake. Is this okay, or should I update it to use the other notation?

This is fine.

Although supporting the underscore version should just require a simple helper method.

Insti · 2016-12-25T09:28:31Z

exercises/grains/grains_test.rb

    skip
-    assert_equal 9_223_372_036_854_775_808, Grains.square(64)
+    assert_equal 9223372036854775808, Grains.square(64)


When we get to numbers like this the underscore representation is REALLY helpful.

Insti · 2016-12-25T09:39:02Z

lib/grains_cases.rb

+      input: row['input'],
+    )
+
+    GrainsCase.new(row.merge('assertion' => assertion, 'index' => i))


Rather than constructing a GrainsCase here, how about subclassing it and just doing

GrainsCase::SquareMethod.new(row: row, index: i)

Insti · 2016-12-25T09:39:41Z

lib/grains_cases.rb

+    GrainsCase.new(row.merge('assertion' => assertion, 'index' => i))
+  end
+
+  assertion = GrainsCase::TotalAssertion.new(


Why does this look different to the square method code?

This is because of the format of the canonical data:

{ "square": { "cases": [ { "description": "...", "input": 1, "expected": 1 }, ... ] }, "total": { "description": "...", "expected": "..." } }

Insti · 2016-12-28T21:06:23Z

Sorry my review pace has slowed down, the holidays seem to leave me with less time rather than more!. I've still got my eye on these PRs, I'm just trying to find some time to give them a final test/review before merging them.

tommyschaefer · 2016-12-28T21:08:28Z

@Insti No problem at all! I totally understand 😄

Insti · 2016-12-29T10:33:37Z

lib/grains_cases.rb

+    GrainsCase::SquareMethod.new(row.merge('index' => i))
+  end
+
+  cases << GrainsCase::TotalMethod.new(data['total'])


This case doesn't know its index.

btw: This looks much nicer than it did last time I looked at it.

Insti · 2016-12-29T10:34:52Z

lib/grains_cases.rb

+  private
+
+  def error_assertion
+      "assert_raises(ArgumentError) { Grains.square(#{input}) }"


dodgy indentation.

Insti · 2016-12-29T10:38:44Z

lib/grains_cases.rb

+  private
+
+  def formatted_expected
+    expected.to_s.reverse.gsub(/...(?=.)/,'\&_').reverse


This would be nicer as a function that took an argument.

def underscore_format(number)

Then you use it as: underscore_format(expected)

Insti · 2016-12-29T10:43:43Z

test/grains_cases_test.rb

@@ -0,0 +1,87 @@
+require_relative 'test_helper'
+
+class GrainsTest < Minitest::Test


I really like that you're unit testing the Case classes. 👍

Hopefully doing this will help you realise which methods should be part of some kind of BaseCase class that could be inherited from rather than always starting from OpenStruct

Insti · 2016-12-29T10:44:12Z

test/grains_cases_test.rb

+    assert_equal '# skip', GrainsCase.new(index: 0).skipped
+  end
+
+  def test_skipped_with_nil_index


Should a case ever have a nil index?
What is the point of an index?

This is a little bit of a tricky one that I'm not sure on the best way to handle.

In my (limited) experience, when the canonical data has more than one "type" of test case, they both each follow the same JSON structure (e.g. allergies). This lets you do something like this when assembling the cases (not an optimal solution necessarily, just demonstrating):

i = -1 JSON.parse(data).flat_map do |type, test_data| test_data['cases'].map do |test_case| i += 1 # Determine which kind of Case to make based on type Case.new(...) end end

But in this case the JSON is structured like:

{ "square": { "cases": [ { "description": "...", "input": 1, "expected": 1 }, ... ] }, "total": { "description": "...", "expected": "..." } }

Because of this, I'm not too sure how to get a real index for this case.

Do you have any thoughts on this? Would it be better to explicitly pass index: false for that case? It's still pretty strange though 😞

What does the index represent? What is it the index of?
Does it mean different things in different places?

(I know the answer, I'm just trying to help you get there.)
(index: false is not the correct answer)

It's the index of the set of test cases. Would something like this work:

data = JSON.parse(data) i = -1 cases = data['square']['cases'].map do |row| i += 1 GrainsCase::SquareMethod.new(row.merge('index' => i)) end cases << GrainsCase::TotalMethod.new(data['total'], index: i+1)

It adds a bit of complexity, but at least the index is correct.

Thank you so much for your help and feedback!

Getting closer 😄

In the above example, how does i relate to cases.size?

Insti · 2016-12-30T18:44:15Z

Is this ready to merge?

tommyschaefer · 2016-12-30T18:46:56Z

As far as I'm concerned, this should be good to go 😄

If there are any other changes you all would like to see, I'm happy to keep working at it!

Insti · 2017-01-02T11:17:24Z

Thanks for all your work on this @tommyschaefer ❤️

tommyschaefer commented Dec 24, 2016

View reviewed changes

tommyschaefer mentioned this pull request Dec 24, 2016

Exercise tests should be generated #396

Closed

94 tasks

Insti reviewed Dec 25, 2016

View reviewed changes

tommyschaefer mentioned this pull request Dec 28, 2016

Add phony target for test in Makefile #518

Merged

Insti reviewed Dec 29, 2016

View reviewed changes

tommyschaefer force-pushed the generate-grains-tests branch 2 times, most recently from 883d1f9 to 577fa7e Compare December 30, 2016 17:34

Generate tests for "grains"

b3e62ca

tommyschaefer force-pushed the generate-grains-tests branch from 577fa7e to b3e62ca Compare December 30, 2016 18:28

Insti merged commit 745d35a into exercism:master Jan 2, 2017

		@@ -0,0 +1,87 @@
		require_relative 'test_helper'

		class GrainsTest < Minitest::Test

Uh oh!

grains: add test generator #514

grains: add test generator #514

Uh oh!

Conversation

tommyschaefer commented Dec 24, 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!

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!

Insti commented Dec 28, 2016

Uh oh!

tommyschaefer commented Dec 28, 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!

Insti Dec 29, 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!

Insti Dec 29, 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 Dec 30, 2016

Uh oh!

tommyschaefer commented Dec 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Insti commented Jan 2, 2017

Uh oh!

Uh oh!

Insti Dec 29, 2016 •

edited

Loading

Insti Dec 29, 2016 •

edited

Loading

tommyschaefer commented Dec 30, 2016 •

edited

Loading