generators: Auto extract cases #568

hilary · 2017-04-24T05:14:34Z

Motivation and Context

While working on #566, I noticed that a number of the existing generators would not run because the loop which extracts cases from the data did not match the current structure. I also noticed a lot of consistency in the loops for both failing and successful generators. Automatically extracting cases from the JSON feels like a big win.

Description

Added Generator::CaseValues (in lib/generator/case_values.rb, which extracts the cases from the canonical data. It uses a custom extractor if such an extractor exists, otherwise it extracts the cases automagically.

How Has This Been Tested?

I wrote tests for Generator::CaseValues for both custom extractor and not custom extractor and for both simple and complex canonical data.
I wrote a test for a public helper method I introduced to Generator::Files::GeneratorCases.
The existing developer tests pass.

Types of changes

No breaking changes.
New feature: auto extraction of cases.

References and Closures

Checklist:

My change requires a change to the documentation.
~~My change relies on a pending issue/pull request~~
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

Insti · 2017-04-24T05:18:28Z

lib/generator/template_values.rb

@@ -31,8 +31,40 @@ def cases_require_name
      Files::GeneratorCases.filename(exercise_name)
    end

+    def test_cases


There is a LOT of untested logic here. That scares me.

Insti · 2017-04-24T05:35:48Z

lib/generator/template_values.rb

+    end
+
+    def test_cases_proc?
+      Object.const_defined?(test_cases_procname)


Use the method you already have: test_cases_proc.exists?

test_cases_proc returns NameError if the constant is not defined. I dislike rescuing an error in this situation, but would prefer to check to see if it is there.

It was desirable before, for the whole thing to blow up if the test case class did not exist. Since that's no longer the case it suggests to me that test_cases_proc needs changing to something like:

def test_cases_proc Object.const_get(test_cases_procname) if Object.const_defined?(test_cases_procname) end

(This may be moot due to upcoming object extraction.)

Insti · 2017-04-24T05:36:46Z

test/generator/files/track_files_test.rb

@@ -25,6 +25,10 @@ def test_filename
        assert_equal 'two_parter_cases', GeneratorCases.filename(exercise_name)
      end

+      def test_class_name
+        assert_equal 'TwoParterCase', GeneratorCases.class_name('two-parter')
+      end


This is a nice test

Insti · 2017-04-24T05:38:00Z

lib/generator/files/track_files.rb

      def proc_name(exercise_name)
-        filename(exercise_name).split('_').map(&:capitalize).join
+        "#{class_name(exercise_name)}s"


If you're just adding an 's' do class_name(exercise_name) + 's'

Insti · 2017-04-24T05:40:16Z

I'd like to see an example of how class_name and proc_name are used as part of this PR.

Insti · 2017-04-24T05:43:14Z

lib/generator/template_values.rb

+        test_cases_proc.call(canonical_data.to_s)
+      else
+        test_cases_with_index
+      end


There's another object hiding in here.
Replace all this with:

test_cases_foobar.call(canonical_data.to_s)

Then work out what test_cases_foobar is.

This will also help solve your (my) "bunch of untested private methods" problem.

💯 Win, win, win. I'll refactor and dig that object out tomorrow.

hilary · 2017-04-24T05:48:34Z

proc_name is an existing method. I'm preserving it for backwards compatibility. Once we've converted we can ditch it.

hilary · 2017-04-24T05:51:56Z

lib/generator/template_values.rb

+    end
+
+    def test_case_class
+      Object.const_get(Files::GeneratorCases.class_name(exercise_name))


@Insti here is where class_name is used.

~~So proc_name is never used externally and should be private?~~
I read the comment above.

hilary · 2017-04-24T23:14:59Z

Refactoring to split apart the adapter and the two solutions

Insti · 2017-04-25T15:13:45Z

test/generator/template_values_test.rb

+      simple_canonical_data
+    end
+
+    def complex_canonical_data


These (simple|complex) can live in /fixtures somewhere.

We discussed this in gitter and have decided to try a data section at the end of the test file.
The main thing is removing the big heredoc from the middle of the test file.

Insti · 2017-04-25T17:08:54Z

test/generator/template_values_test.rb

+
+  class CaseValuesTest < Minitest::Test
+    def setup
+      $LOAD_PATH.unshift 'test/fixtures/xruby/lib'


Modifying the load path for every test should not be necessary.
But there might be something else to do so that everything still works.

I was following the existing pattern.

I probably should fix that then 😊

Insti · 2017-04-25T17:12:47Z

test/generator/template_values_test.rb

+    end
+
+    def simple_canonical_data
+      simple_canonical_data = Minitest::Mock.new


~~Why is this a Mock?~~
Nevermind.

Insti · 2017-04-25T20:40:53Z

test/generator/case_values_auto_extractor_test.rb

+        complex_canonical_data = Minitest::Mock.new
+        complex_canonical_data.expect(
+          :to_s,
+          <<-TEXT


Were you going to move this data out of the tests?

They're at the bottom of the file now.

Where I'd really like to put these is in a module I could include.

In which case why not use a fixture?
How is a module any different, or even preferable when the data should be non-executable?

A module can easily be included in multiple places, but (may be/is ?) easier to be surgical than a fixture.

I guess the question is, what are you suggesting? That I create fixtures for each of these cases, in which case I will end up duplicating data or reusing fixtures across tests, neither of which is desirable.

While data is non-executable, being able to manipulate the data via code makes the test suite far more manageable.

Insti · 2017-04-25T20:45:06Z

test/generator/case_values_auto_extractor_test.rb

@@ -0,0 +1,116 @@
+require_relative '../test_helper'


case_values_auto_extractor_test.rb has too many _auto_extractors in the filename

Insti · 2017-04-26T07:39:34Z

test/generator/template_values_test.rb

@@ -56,4 +56,5 @@ def teardown
      $LOAD_PATH.delete 'test/fixtures/xruby/lib'
    end
  end
+


An unnecessary blank line change has snuck in.

Insti · 2017-04-26T08:41:08Z

test/generator/case_values_auto_extractor_test.rb

+        $LOAD_PATH.unshift 'test/fixtures/xruby/lib'
+      end
+
+      def test_simple_auto_extraction


I believe this test is redundant.

Just load the alpha_cases file direcly, we know where it is.

test/fixtures/metadata/exercises/alpha/canonical-data.json We are the only user of this fixture data.

Renamed 'gamma' to 'complex'.

Add test that template_values loads problem cases Remove commented out code. Make everything that loads AlphaCase(s) clean up after itself. Undefine AlphaCase(s) during teardown. Remove redundant setup. Check for both classes. Add tests for class based test generation squash Remove TODO comment

all generators converted to use auto extraction in exercism#567

(We're not making use of it yet, but now it is there we can.)

Refactor Extractor

hilary mentioned this pull request Apr 24, 2017

generators: convert generators to use auto extract #567

Merged

3 tasks

Insti reviewed Apr 24, 2017

View reviewed changes

hilary commented Apr 24, 2017

View reviewed changes

Insti reviewed Apr 25, 2017

View reviewed changes

Insti reviewed Apr 26, 2017

View reviewed changes

hilary and others added 15 commits April 26, 2017 14:17

add class_name to GeneratorCases

e30946e

programmatically generate cases

77cac07

extract CaseValues from TemplateValuesFactory

6134c48

separate logic for proc and auto extraction

982e9b3

add #proc? to GeneratorCases

263e233

fold Extract into TemplateValuesFactory

068d1f5

Minor: Remove unnecessary whitespace changes.

75b81d7

Remove redundant test.

9f6a9e3

Inline the GammaCase rather than reading it from a file.

1700748

Move case values tests into a single file.

33e0881

Remove redundant Mocks.

f088d24

Add test coverage for unimplmented extract method.

001e608

Eliminte LOAD_PATH modifications.

012ce13

Just load the alpha_cases file direcly, we know where it is.

Use fixture data we already have.

9547152

test/fixtures/metadata/exercises/alpha/canonical-data.json We are the only user of this fixture data.

Remove unnecessary inclusion of CaseValues

d29bceb

Insti added 3 commits April 26, 2017 14:17

Extract complex json to fixture file.

8b2ae2a

Renamed 'gamma' to 'complex'.

Remove unnecessary string conversions.

8fb05bf

hilary force-pushed the auto_extract_cases branch from 9a14c4d to 45511ff Compare April 26, 2017 21:18

hilary and others added 8 commits April 26, 2017 14:28

remove proc support (not needed)

0b47df3

all generators converted to use auto extraction in exercism#567

Pass the data to be extracted to the extract method.

d85ace4

(We're not making use of it yet, but now it is there we can.)

Pass the 'case_class'name in as an argument to Extractor

4818a04

Don't use exercise_data in the initializer anymore.

bbde073

exercise_name is not required by AutoExtractor anymore.

96dbd2b

Rename 'extract' method to 'call'

42ff562

Merge pull request #4 from Insti/Refactor_Extractor

b52da2b

Refactor Extractor

rename Extractor#call to #cases

557997c

Insti merged commit f56c429 into exercism:master Apr 27, 2017

Uh oh!

generators: Auto extract cases #568

generators: Auto extract cases #568

Uh oh!

Conversation

hilary commented Apr 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

Description

How Has This Been Tested?

Types of changes

References and Closures

Checklist:

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 Apr 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Insti Apr 24, 2017 • 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!

hilary commented Apr 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Insti Apr 24, 2017 • 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!

hilary commented Apr 24, 2017

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 Apr 25, 2017 • 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 Apr 26, 2017 • 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 Apr 26, 2017 • 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!

hilary commented Apr 24, 2017 •

edited

Loading

Insti Apr 24, 2017 •

edited

Loading

Insti Apr 24, 2017 •

edited

Loading

Insti Apr 25, 2017 •

edited

Loading

Insti Apr 26, 2017 •

edited

Loading

Insti Apr 26, 2017 •

edited

Loading