DRY test generators #566

hilary · 2017-04-22T22:19:35Z

Motivation and Context

The test generators have a lot of duplicate code. Do we need to discuss why that's not good? I decided to take a pass at DRYing up the test generators before moving on to writing a test generator scaffold generator...

Design principles:

backwards compatible (while most of the generators I saw don't currently work 😭 ), these change can't break any that do.
facilitate, don't dictate. Some test generators have different needs. These changes can't make those test generators a nightmare to write.
take small, bite-sized steps that can be merged one at a time.

This effort:

is part of Generator improvements. #485
is meant to facilitate ruby getting to and staying up to date with canonical data canonical-data.json: Compliance with JSON Schema problem-specifications#625
takes direction from @Insti 's test generator master design [WIP] Test generation refactor Insti/exercism-ruby#1

Description

I introduced a base class, ExerciseCase into lib/exercise_cases.rb from which ProblemNameCase can inherit. ExerciseCase provides public methods name and skipped as well as a variety of protected helper methods.

I converted three generators I wrote, and one I didn't write, to use ExerciseCase. The generated test files remain (almost) the same while the generators exhibit far more consistency. (The only changes to the test files are some small changes in how the descriptions are processed to produce the test names.

I updated the README to reflect the changed workflow.

How Has This Been Tested?

I can regenerate the tests with no changes except for the small changes noted above.
The tests pass under rake test.

Next Steps

There's no need for the person writing a generator to write the ProblemNameCases logic. We can inspect and extract the cases from the JSON programmatically.
Fall back to a default template for exercises which don't provide a customized template. The four exercises I have converted thus far can all use the same template and I'm sure that many, many other exercises can as well. I would much rather not generate a template by default, as we should be actively discouraging putting logic into views.

converted: - hamming - ocr-numbers - luhn - pig-latin to use it

kotp · 2017-04-23T02:20:36Z

lib/exercise_cases.rb

+
+class ExerciseCase < OpenStruct
+  def name
+    'test_%s' % description.gsub(/[^\w ?!]/, '').gsub(/[- ]/, '_')


Does this change cause the test names to change by not presenting hyphenated words as individual words, but rather as shown in this diff:

- def test_large_distance_in_off_by_one_strand + def test_large_distance_in_offbyone_strand

I would like to place the change back in, due to things like "non_unique" being acceptable and readable, even if it technically isn't a separate word, and to avoid "off-by-one" becoming "offbyone".

Will do (in the morning!)

I think it is a separate commit, so it may be a simple process. That was a great idea to separate it out like that!

I'm not sure if this is premature:

We will need a underscore_format(string)* method that we can have a bunch of unit tests for so we can keep track of all the weird cases we encounter.

So this line becomes:
'test_' + underscore_format(description)

* name and whether it should be a String refinement TBD.

kotp · 2017-04-23T02:39:02Z

The design principles are very much in line with what we want to do, I think.

while most of the generators I saw don't currently work

These places would definitely benefit from being named.

Insti · 2017-04-23T05:04:51Z

Firstly: Awesome PR description ❤️ thanks.

Design principles:
backwards compatible

This is not a requirement, feel free to break as much stuff as you like.
There are a significant number of already broken generators (as you have discovered.)
Once we work out what the "true" generation process is we can update all the old generators.

I introduced a base class, ExerciseCase into lib/exercise_cases.rb

Excellent.

I converted [four] generators [..] to use ExerciseCase.

Excellent.

I updated the README to reflect the changed workflow.

Excellent.

Insti · 2017-04-23T05:07:03Z

lib/exercise_cases.rb

@@ -1,2 +1,30 @@
 require 'ostruct'


This file should be in `lib/generator'

Indeed! I'll go ahead and move it (also in the morning).

Insti · 2017-04-23T05:11:50Z

lib/exercise_cases.rb

@@ -1,2 +1,30 @@
 require 'ostruct'
 require 'json'
+
+class ExerciseCase < OpenStruct


Not an issue for this PR, but ExerciseCase should not be an OpenStruct and the canonical data should be kept separate from the methods of this class.

See: my imagined version of ExerciseCase for an example.

Insti · 2017-04-23T05:14:23Z

lib/luhn_cases.rb

-
-  def assertion
-    expected ? 'assert' : 'refute'
+    %Q(#{assert_or_refute} Luhn.valid?(#{input.inspect}))


I like that all these need to do now is implement workload.

Insti · 2017-04-23T05:36:16Z

All these TODOs probably deserve their own issue.

TODO

extract the cases from the JSON programmatically.

Yes.
Be aware that some problems need to pre-process the canonical_data.
See: all-your-base and #510

Fall back to a default template for exercises which don't provide a customized template.

Yes.

Now some of the things from the grand design in my head:

Note: "problem" in all these examples is the variable exercise name.

Generator problem_cases.rb should live in exercises/problem/.meta/generator/
Templates should live in problem/.meta/generator/ be called test_file_template.erb (or similar). Once they are under .meta they don't need to contain the word example
There definitely should be a default test_file_template.erb - probably in /lib/generator
ExerciseCase should not be an OpenStruct and the canonical data should be kept separate from the methods of this class.
Remove requirement for a test case object to know its index.
canonical_data version should be added to the test file - probably immediately to the left of the abbreviated commit hash.
.version should be updated to be the canonical_data version (e.g. 2.1.0) plus an x-ruby version. (3) e.g.: 2.1.0.3

Some of this will require changes to the bin/generator code.

By creating the new modern generators under .meta/generator it will be easy to track which exercises have been upgraded as the lib/problem_cases.rb are deleted with the end goal being all generators living under .meta/generator and lib being mostly empty.

Everything needed to create a generator for an exercise will be in exercises/problem/.meta/generator/

Insti · 2017-04-23T05:39:01Z

For reference, here is my proof of concept for this from a few months ago: Insti#1

Insti · 2017-04-23T05:41:44Z

lib/exercise_cases.rb

+    expected.to_i == -1
+  end
+
+  def assert_or_refute


Can this be just assert it doesn't REALLY matter if the final test file contains refute and

"#{assert} Problem.call(#{input.inspect})"

looks better in the readme than:

"#{assert_or_refute} Problem.call(#{input.inspect})"

Future:
It may be possible to add logic into the assert method that works out the correct assert to use including assert_equal or assert_error

Insti · 2017-04-23T05:57:21Z

I think that's all for now.
I'm happy for this PR to be merged as-is, and have further work be continued in other ones.
(but also ok with adding some of the minor changes to this one.)

tommyschaefer

This looks awesome!! Thank you so much for your work on this!

tommyschaefer · 2017-04-23T16:19:36Z

exercises/ocr-numbers/ocr_numbers_test.rb

    skip
    assert_equal "1", OcrNumbers.convert("   \n  |\n  |\n   ")
  end

-  def test_unreadable_but_correctly_sized_inputs_return_?
+  def test_Unreadable_but_correctly_sized_inputs_return_?


I think ExerciseCase#name should probably also downcase the description.

What to do about punctuation characters is another issue.

But this one is probably a job for a pre-processor, since it's problem dependent.
s/"?"/question mark string/ as the json description is being read in and before ExerciseCase#name sees it.

Heh. I had downcase in there at one point, then took it out. Shoulda trusted my first instincts on that one 😄

Insti · 2017-04-23T21:41:16Z

lib/pig_latin_cases.rb

@@ -1,5 +1,3 @@
-require 'exercise_cases'


Not entirely sure I agree with removing this line.
Sure require_all is awesome, but there's only one dependency and it's nice to be able to see where ExerciseCase comes from.

And it simply doesn't break anything... if that dependency goes away, no problem, it is a documentation value at this point.

Insti · 2017-04-23T21:44:00Z

test/generator/string/underscore_test.rb

+    assert_equal 'A string with TLA'.underscore, 'a_string_with_tla'
+  end
+
+  def hyphenated_text


def test_hyphenated_text

Insti · 2017-04-23T21:47:32Z

lib/luhn_cases.rb

 class LuhnCase < ExerciseCase
  def workload
-    %Q(#{assert_or_refute} Luhn.valid?(#{input.inspect}))
+    "#{assert} Luhn.valid?(#{input.inspect})"


It's a good idea to keep your commits atomic.
This has nothing to do with underscores and should be in its own commit.

It is a discipline that needs to be trained. Commit early, commit often.

Insti · 2017-04-23T21:48:04Z

lib/ocr_numbers_cases.rb

@@ -1,5 +1,3 @@
-require 'exercise_cases'


Should have been another separate commit.

kotp · 2017-04-23T22:55:59Z

exercises/hamming/example.tt

-    assert_raises(ArgumentError) { <%= test_case.workload %> }<% else %>
-    assert_equal <%= test_case.expected %>, <%= test_case.workload %><% end %>
+    <%= test_case.skipped %>
+    <%= test_case.workload %>


kotp · 2017-04-23T22:57:03Z

lib/ocr_numbers_cases.rb

    else
-      "assert_equal #{expected.inspect}, #{test_case}"
+      assert_equal { test_case }
    end


Definitely a better place for this.

hilary · 2017-04-24T00:10:54Z

Putting this here so we don't forget:

We currently create exercises/$PROBLEM/.meta/.version and fill it with 1 if it doesn't exist. Do we create the .meta directory if needed? If not, we should. Once we do, we should remove the instruction to do so from the README.

hilary added 2 commits April 22, 2017 14:47

add base class ExerciseCase

cd1c83b

converted: - hamming - ocr-numbers - luhn - pig-latin to use it

fix regex for cleaning test names

33e6026

kotp reviewed Apr 23, 2017

View reviewed changes

Insti approved these changes Apr 23, 2017

View reviewed changes

Insti reviewed Apr 23, 2017

View reviewed changes

tommyschaefer approved these changes Apr 23, 2017

View reviewed changes

add underscore refinement

fd10aa8

Insti reviewed Apr 23, 2017

View reviewed changes

lib/ocr_numbers_cases.rb

@@ -1,5 +1,3 @@

require 'exercise_cases'

Copy link

Contributor

Insti Apr 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have been another separate commit.

namespace underscore refinement

9297e0a

hilary changed the title ~~[WIP] DRY test generators~~ DRY test generators Apr 23, 2017

hilary added 2 commits April 23, 2017 15:25

update README (doh!)

f2cb3bb

move underscore to lib/generator/

b028f22

Insti approved these changes Apr 23, 2017

View reviewed changes

kotp reviewed Apr 23, 2017

View reviewed changes

hilary mentioned this pull request Apr 24, 2017

generators: convert generators to use auto extract #567

Merged

3 tasks

kotp merged commit e9d3c95 into exercism:master Apr 24, 2017

hilary mentioned this pull request Apr 24, 2017

generators: Auto extract cases #568

Merged

5 tasks

Uh oh!

DRY test generators #566

DRY test generators #566

Uh oh!

Conversation

hilary commented Apr 22, 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?

Next Steps

Uh oh!

kotp Apr 23, 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!

Choose a reason for hiding this comment

Uh oh!

kotp commented Apr 23, 2017

Uh oh!

Insti commented Apr 23, 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!

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

Uh oh!

TODO

Uh oh!

Insti commented Apr 23, 2017

Uh oh!

Insti Apr 23, 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!

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

Uh oh!

Uh oh!

tommyschaefer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Insti Apr 23, 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!

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!

hilary commented Apr 24, 2017

Uh oh!

Uh oh!

hilary commented Apr 22, 2017 •

edited

Loading

kotp Apr 23, 2017 •

edited

Loading

Insti commented Apr 23, 2017 •

edited

Loading

Insti Apr 23, 2017 •

edited

Loading

Insti commented Apr 23, 2017 •

edited

Loading

Insti Apr 23, 2017 •

edited

Loading