All generators use common interface #463

bmulvihill · 2016-10-18T00:50:07Z

Finishes #452
Starts the work of creating a common interface. This PR attempts to standardize 3 public methods

test_name - The method will output the name of the test
workload - This method will output the body of the test
skipped - This method will output skip syntax

I ran all the generators locally and they appear to be working as intended. Once this gets standardized, we may be able to utilize a single example.tt file.

Future ideas:

Utilize a single example.tt for all generated tests
Implement some sort of Generator abstract class/module (Generatable?) where subclasses are required to implement two methods: test_name && workload. We can then move common methods i.e. skipped into this class/module

kotp · 2016-10-18T07:30:30Z

Quick visual scan says "YAY!"

Insti · 2016-10-18T10:38:08Z

.ruby-version

@@ -0,0 +1 @@
+2.3.0


This file probably shouldn't be updated as part of this.

Whoops! yeah I'll remove this

Insti · 2016-10-18T10:41:16Z

lib/hello_world_cases.rb

-  def do
-    defined?(name) ? "HelloWorld.hello('#{name}')" : 'HelloWorld.hello'
+  def work_load
+    assertion =


Can you extract this to a method?

Insti · 2016-10-18T10:42:54Z

lib/two_bucket_cases.rb

@@ -8,15 +8,15 @@ def do
    "TwoBucket.new(#{bucket_one}, #{bucket_two}, #{goal}, '#{start_bucket}')"
  end

-  def test_body
+  def work_load
    "two_bucket = #{self.do}
    assert_equal #{expected['moves']}, two_bucket.moves
    assert_equal '#{expected['goal_bucket']}', two_bucket.goal_bucket
    assert_equal #{expected['other_bucket']}, two_bucket.other_bucket"


I'd like to see work_load itself not have to be responsible for whitespace/indenting.

I agree, I tried not to do too much refactoring though and keep the scope of this PR narrow - just focusing and getting the API in place

OK, that's sensible.

Insti · 2016-10-18T10:51:46Z

The first one I tried bin/generate acronym didn't work correctly. 😞
The test methods had no names.

Insti · 2016-10-18T10:52:29Z

The way to test this would be to regenerate all the test files and ensure that the output was identical to the old versions.

bmulvihill · 2016-10-18T11:11:09Z

@Insti I did regenerate all of them locally, but obviously I missed something 🙂, Ill fix it in my next push!

bmulvihill · 2016-10-18T13:05:28Z

@Insti @kotp Made some additional changes, its a lot of files so hopefully I haven't missed anything else. As mentioned above I tried not to do any refactoring and just implement the API.

kotp · 2016-10-18T13:35:54Z

I just tried to do the generate on word-count as it is on #448 which is rebased on master since #451 has been brought in, getting errors there as well. In that case, it is due to single quotes inside of strings that have single quotes as the delimiters. (That is of course the first cause, once fixed the other errors may fix, I did not look that closely)

Of course, this may not reflect work done on this PR, since this is not in master yet.

Insti · 2016-10-19T19:52:10Z

@bmulvihill the conversions are still very broken. I think you need to go through them all individually and check that the new generated file is the same as the old one.

Both of the ones I "randomly" looked at: leap, acronym were very broken.
Running: rubocop exercises will detect syntax errors in the files.

Maybe it would be better to split this PR into smaller PRs that look at each generator individually?

Insti · 2016-10-19T19:53:41Z

I'd like to see a PR with some documentation of what the common interface is, you've given a good summary in the OP here, but it probably belongs in the README.md

Insti · 2016-10-19T19:57:33Z

I'd rather work_load be renamed workload if that's the name we're going with.

'work' and 'load' are 2 words with many different meanings, 'workload' is one word with a more specific meaning.

My but preference for name would be assertion which would make the generic template:

  def <%= test_case.test_name %>
    <%= test_case.skipped %>
    <%= test_case.assertion %>
  end

Insti · 2016-10-19T20:04:34Z

Those last 2 probably should be separate issues so that they can be discussed individually.

I really appreciate the effort you're putting into this @bmulvihill ❤️ and agree that it is an important thing to work on, so if we're going to do a lot of work getting it right, we should try to make sure we're doing the right thing and I want to try to help that be as easy as possible.

bmulvihill · 2016-10-19T20:28:29Z

@Insti I agree with the name assertion over work_load, I believe in the original issue work_load was suggested. Another name which might make more sense is test_body, since it could technically include more than just an assertion.

I can make that change if you want and add a README update to this PR, since I think that makes sense. I will also go back through each generator and compare outputs.

Insti · 2016-10-19T20:58:19Z

@kotp, @bmulvihill is there anywhere other than #452 where naming was discussed?

test_body [...] it could technically include more than just an assertion.

Good point. I'd prefer just body over test_body since the test_ prefix is redundant.

  def <%= test_case.test_name %>
    <%= test_case.skipped %>
    <%= test_case.body %>
  end

Either that or go fully prefixed to try and avoid the situation where one of the json files in the future has a property called 'skipped' or 'body', or 'test_body'

  def <%= test_case.test_case_name %>
    <%= test_case.test_case_skipped %>
    <%= test_case.test_case_body %>
  end

But that seems a bit redundant.

I guess we could discuss it forever.

As the person making the PR you have the power to decide what you call it 😉 so choose what you think is best.

kotp · 2016-10-19T21:30:30Z

I do prefer the one word workload as it is a single word, and means what it says.

Here is where any mention of workload or work_load was discussed or mentioned.

bmulvihill · 2016-10-20T12:12:13Z

@Insti @kotp I went back through each exercise and generated/ran the tests and everything appears to be working. What I hadn't done before, which I should have, was actually run the tests for each newly generated file. I also renamed work_load to body.

kotp · 2016-10-20T13:29:21Z

The name "body" I think in the discussions that we did have was determined to be too generic. Body of what? I think this was an important reason why workload or work_load was finally chosen. I can't look at the discussions right now, but I hope you did at least scan through them via the link I gave, if so, you could verify, better than I can at the moment.

I can't find it at the moment though, even with that search.

bmulvihill · 2016-10-20T13:39:51Z

@kotp I scanned through the discussions, but nothing jumped out at me besides ours and one where @kytrinyx mentioned work_load. Its an easy switch to workload if you find that more descriptive, I think consistency is the most important thing here

Insti · 2016-10-20T17:13:05Z

I looked through that search result and also couldn't find any discussion on naming.
If we need to compromise, I'm OK with using workload

kotp · 2016-10-20T23:05:02Z

I would go with what @kytrinyx mentioned, because it is likely to be the same in other tracks.

bmulvihill · 2016-10-21T00:13:26Z

@Insti @kotp This has been rebased, and the method has been renamed to workload - I also added in the two new generators that have been added since this opened, word-count and anagram

kotp · 2016-10-21T09:34:19Z

I think I sent you a message on Gitter @bmulvihill Let me know if you can't get to it.

Can you look at this error, when I generated all files, ran make to run the tests (and this takes a while due to alphametics) but got this error.

running tests for: alphametics
Run options: --seed 62490

# Running:

.F.....

Finished in 306.960573s, 0.0228 runs/s, 0.0228 assertions/s.

  1) Failure:
AlphameticsTest#test_solve_puzzle_with_many_words [/tmp/alphametics.7i3sjoNLoE/alphametics_test.rb:60]:
--- expected
+++ actual
@@ -1 +1 @@
-{"A"=>5, "D"=>3, "E"=>4, "F"=>7, "G"=>8, "N"=>0, "O"=>2, "R"=>1, "S"=>6, "T"=>9}
+nil


7 runs, 7 assertions, 1 failures, 0 errors, 0 skips

bmulvihill · 2016-10-21T12:34:10Z

@kotp That test is currently commented out - and when I run it prior to these changes it fails the same way

kotp · 2016-10-21T13:43:17Z

Alright. I will look into it more tonight. Wonder why it tests if it is commented out.

bmulvihill · 2016-10-21T13:58:57Z

@kotp I think maybe the tests were manually commented out, so when it was regenerated it doesn't have the comments

Insti · 2016-10-21T16:51:47Z

tests were manually commented out

I think this is quite likely.

The generator needs to be updated to do the commenting out itself.

kotp · 2016-10-22T00:13:31Z

Yep, I think we hit this before, where we would have to manually comment them out, which is not something I think we should have to remember to do. Unfortunately, there is nothing in the canonical to indicate these are "long running tests" to help to programmatically trigger this commenting behavior.

Insti · 2016-10-22T09:12:10Z

It is possible to detect and special case them in the *_cases.rb file though.

bmulvihill · 2016-10-22T19:20:23Z

@Insti @kotp I updated the README as well. As far as programmatically triggering the comments, I believe that should be a different PR, do you agree?

Insti · 2016-10-24T17:53:24Z

@bmulvihill it probably is a different PR, but it needs to happen before this one can be merged as the tests need to run at a reasonable speed.

bmulvihill · 2016-10-24T18:32:06Z

@Insti hmmm ok - I'll take a look at that

Insti · 2016-10-24T18:57:38Z

@bmulvihill I'm currently working on the alphametics generator.

Insti · 2016-10-24T21:39:17Z

Alphametics generator: #469

bmulvihill · 2016-12-07T01:42:50Z

@Insti @kotp Im going to close this PR - I think since the README is updated and the generator implementation is still kind of up in the air we can tackle this on a smaller scale

Insti · 2016-12-07T07:56:59Z

@bmulvihill fine, thanks for getting us going on this. It started a lot of useful discussion.

I need to find some time to work on my new generator branch again.

Insti reviewed Oct 18, 2016

View reviewed changes

bmulvihill mentioned this pull request Oct 18, 2016

Add word count generator #448

Merged

Insti added the in progress label Oct 19, 2016

All generators use common interface

745ffcf

bmulvihill force-pushed the common-interface branch from 192abe8 to 3adf16b Compare October 20, 2016 01:53

Ensuring all templates call test_name, fixes

3fd2c27

bmulvihill force-pushed the common-interface branch from 3adf16b to 3fd2c27 Compare October 21, 2016 00:11

Update README with Exercise Generators

80ea27d

Insti mentioned this pull request Oct 23, 2016

Add tournament #467

Merged

This was referenced Oct 27, 2016

Add triangle test generator #474

Merged

Update README with Exercise Generators #478

Merged

petertseng mentioned this pull request Nov 8, 2016

dominoes: Add generator and example #484

Merged

bmulvihill closed this Dec 7, 2016

tommyschaefer mentioned this pull request Dec 20, 2016

wordy: Generate tests #509

Merged

Insti removed the in progress label May 19, 2017

Insti removed the ready label Jun 8, 2017

		@@ -0,0 +1 @@
		2.3.0

Uh oh!

All generators use common interface #463

All generators use common interface #463

Uh oh!

Conversation

bmulvihill commented Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kotp commented Oct 18, 2016

Uh oh!

Insti Oct 18, 2016

Choose a reason for hiding this comment

Uh oh!

bmulvihill Oct 18, 2016

Choose a reason for hiding this comment

Uh oh!

Insti Oct 18, 2016

Choose a reason for hiding this comment

Uh oh!

Insti Oct 18, 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!

bmulvihill Oct 18, 2016

Choose a reason for hiding this comment

Uh oh!

Insti Oct 18, 2016

Choose a reason for hiding this comment

Uh oh!

Insti commented Oct 18, 2016

Uh oh!

Insti commented Oct 18, 2016

Uh oh!

bmulvihill commented Oct 18, 2016

Uh oh!

bmulvihill commented Oct 18, 2016

Uh oh!

kotp commented Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Insti commented Oct 19, 2016

Uh oh!

Insti commented Oct 19, 2016

Uh oh!

Insti commented Oct 19, 2016

Uh oh!

Insti commented Oct 19, 2016

Uh oh!

bmulvihill commented Oct 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Insti commented Oct 19, 2016

Uh oh!

kotp commented Oct 19, 2016

Uh oh!

bmulvihill commented Oct 20, 2016

Uh oh!

kotp commented Oct 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmulvihill commented Oct 20, 2016

Uh oh!

Insti commented Oct 20, 2016

Uh oh!

kotp commented Oct 20, 2016

Uh oh!

bmulvihill commented Oct 21, 2016

Uh oh!

kotp commented Oct 21, 2016

Uh oh!

bmulvihill commented Oct 21, 2016

Uh oh!

kotp commented Oct 21, 2016

Uh oh!

bmulvihill commented Oct 21, 2016

Uh oh!

Insti commented Oct 21, 2016

Uh oh!

kotp commented Oct 22, 2016

Uh oh!

bmulvihill commented Oct 18, 2016 •

edited

Loading

Insti Oct 18, 2016 •

edited

Loading

kotp commented Oct 18, 2016 •

edited

Loading

bmulvihill commented Oct 19, 2016 •

edited

Loading

kotp commented Oct 20, 2016 •

edited

Loading