[WIP] Test generation refactor

[Insti]

These are a big bunch of exploratory refactorings trying to arrive at a more elegant test generation process.

These changes are NOT ready to be merged in to main xruby due to incompatibility with every other existing test generator.

Isogram was the the main example used.

Extracted ExerciseTestCase and ExterciseTestCases classes.

isogram_cases.rb and the template are both now much simpler.

Other test cases should be really easy to make by just creating the relevant workload method.

  def workload
    [
      "string = '#{canonical_data.input}'",
      "#{assertion} Isogram.isogram?(string), '#{failure_message}'"
    ]
  end

Handling of test skipping and building the complete test case is handled by the parent ExerciseTestCase class.

All this has required changes to lib/generator.rb which at this point are not backwards compatible.

Added a tests directory for tests of the generator code.
As well as a Rakefile for running the tests,

rake test

and simplecov for checking test coverage. (A /coverage directory is created automatically)

This is all still a work in progress.

[bmulvihill]

@Insti I would consider leaving some of the structure in the actual view, static items like method definitions, etc. These items should all be consistent from test to test so I think they would belong in the view.

UPDATE: Ahh I see how you made bookkeeping a test_case, I think that makes sense, so here is revised view, adding a test_case.comment method.

#!/usr/bin/env ruby
gem 'minitest', '>= 5.0.0'
require 'minitest/autorun'
require_relative <%= exercise_name %>

# Test data version: <%= sha1 %>
class <%= "#{exercise_name}Test" %> < Minitest::Test
<% test_cases.each do |test_case| %>
  <%= test_case.comment %>
  def <%= test_case.test_name %>
    <%= test_case.skipped %>
    <%= test_case.render %>
  end
<% end %>
end

[bmulvihill]

Could we insert the bookkeeping test case into the cases array to always generate it at the end of the view? This would allows us to render it just like all the other test cases, since it is a ExerciseTestCase

[Insti]

Thanks for taking the time to look at this @bmulvihill ❤️

I really like your suggested template, which would allow generating (practically?) all test files from the same template. Are there any tests that deviate from this template? Should there be any?

Adding the Bookkeeping test to the end of the array of test cases and then just rendering them all at once is also a nice idea. However I do wonder if it is better to have it separate in the template to make it explicit that it is a totally different aspect we're testing.

Bias disclaimer: I would prefer that version is indicated by a single line comment in the code rather than forcing everybody to add 3 lines of otherwise useless module to every solution. But I can see how the current module based versioning can be useful so I'm not fighting very hard to change things.

I'll find some time over the next few days to implement your suggestions.
(Unless you want to have a go at it? I find writing code can be more fun than just doing reviews all the time. :) )

Maybe I should merge this into a branch so that we can both submit PRs to it.

[Insti]

Oh wait, it is already a branch, test_generation_refactor, you could submit PRs to!

(not sarcasm, just a genuine "d'oh!" moment..)

[bmulvihill]

I think all exercises should be able to use the same template. Every test file follows an identical structure, I think the only case where I have seen a major departure is the alphametics exercise, where we comment out a whole test due to long run times. I think that can be circumvented by simply skipping those cases normally and utilizing the comments method to explain any additional issues (i.e. This test may take a long time to run unless an optimal solution is written, or This test is considered an advanced case and will fail under a naive implementation).

@Insti Ill see if I can find some time later today/tomorrow to submit a PR into this branch

[Insti]

Alphametics required a different structure due to the inefficiency of the example solution.
This has now been fixed and the new example runs in a reasonable amount of time so the tests no longer need to be commented out.

So hopefully that means that all tests should match the template.

[Insti]

I do wonder if it is better to have it separate in the template to make it explicit that it is a totally different aspect we're testing.

That's not really the template's job though.
When I'm looking at 'things that are tested' I shouldn't need to look at the template to work this out.

[Insti]

Dominos looks like it will require a different test_cases template. See: https://github.com/exercism/xruby/pull/484/files
But this should be possible to implement by using a custom template for it.

[bmulvihill]

Hmm I see that, I need to think about it a bit, a different template could work and might be the best solution.

[Insti]

Wouldn't it just work if the template is:

#!/usr/bin/env ruby
gem 'minitest', '>= 5.0.0'
require 'minitest/autorun'
require_relative <%= exercise_name %>

# Test data version: <%= sha1 %>
class <%= "#{exercise_name}Test" %> < Minitest::Test
<% test_cases.each do |test_case| %>
  <%= test_case.comment %>
  def <%= test_case.test_name %>
    <%= test_case.skipped %>
    <%= test_case.render %>
  end
<% end %>

# Domino specific stuff here.
#...
#...

end

?

Nothing much has to change other than domino having its own specific template instance.

[bmulvihill]

Yeah that would work well, would we be able to have a partial template? Therefore we always use a base template and if an exercise needs to it can define a partial view to be rendered. (The code below is pseudo-code for brainstorming purposes, and not the correct interface for ERB.)

I think the generator creation process should be and straightforward as possible, so if this doesn't feel simple enough we can just write a new template.

#!/usr/bin/env ruby
gem 'minitest', '>= 5.0.0'
require 'minitest/autorun'
require_relative <%= exercise_name %>

# Test data version: <%= sha1 %>
class <%= "#{exercise_name}Test" %> < Minitest::Test
<% test_cases.each do |test_case| %>
  <%= test_case.comment %>
  def <%= test_case.test_name %>
    <%= test_case.skipped %>
    <%= test_case.render %>
  end
<% end %>

<%= render_partial "#{exercise_template}" if exercise_template %>

[Insti]

Another datapoint for unusual test cases is Queen attack: exercism#487 which uses subclasses to generate test cases for the various different types of test in the canonical-data.json. I suspect this is compatible with the way we are doing things here, but I've not yet looked really closely at what it is doing.

[bmulvihill]

@Insti can you give me contributor access to this repo so I can create a PR into this branch for discussion? I don't think I can fork two of the same repo (I already have exercism/xruby forked)

[bmulvihill]

Another datapoint for unusual test cases is Queen attack: exercism#487 which uses subclasses to generate test cases for the various different types of test in the canonical-data.json. I suspect this is compatible with the way we are doing things here, but I've not yet looked really closely at what it is doing.

I see that, I think we should be able to handle that either via subclassing, decorators, or some sort of factory method. As long as the objects respond to the interface everything should work well.

[Insti]

TODO: Support preprocessing of canonical-data.json to add track specific elements.
See: all-your-base and exercism#510

[Insti]

Generator has moved on a lot since these days.