all-your-base: add test generator #510

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

kotp merged 4 commits into exercism:master from tommyschaefer:generate-ayb-tests

Jan 4, 2017

Contributor

tommyschaefer commented Dec 20, 2016 •

edited

Loading

Hi there!

Checking off exercises from #396.

I've made some good progress converting all-your-base to use generated tests, but I've run into a bit of a snag implementing the 13 undefined tests from the shared data.

I've opened exercism/problem-specifications#473 to see if this has been solved before, or if some new information will have to be added to the shared test data.

In the future, would it be preferable for me to wait to make a PR until the changes are fully ready for review? I was thinking it may be better to just go ahead and make the PR so that people could see it had some work being done on it, but if thats not the case I won't do it in the future 😄!

Thank you so much for your time!

tommyschaefer mentioned this pull request

Exercise tests should be generated #396

Closed

94 tasks

Contributor

Insti commented Dec 20, 2016

Works in progress are definitely encouraged.

Contributor

Insti commented Dec 20, 2016 •

edited

Loading

I've run into a bit of a snag implementing the 13 undefined tests from the shared data.

You'll need to pre-process the canonical data and define the appropriate values.

The existing all-your-base example solution should give you an indication of what the appropriate values are.

Insti reviewed

View reviewed changes

exercises/all-your-base/all_your_base_test.rb

@@ @@ -119,7 +123,7 @@ def test_empty_list @@
                   digits = []
                   input_base = 2
                   output_base = 10
-                  expected = []
+                  expected = WHAT TO DO?

Contributor

Insti Dec 20, 2016

expected = []

Insti reviewed

View reviewed changes

exercises/all-your-base/all_your_base_test.rb

                   output_base = 2
-                  expected = [0]
+                  expected = WHAT TO DO?

Contributor

Insti Dec 20, 2016

expected = [0]

Insti reviewed

View reviewed changes

exercises/all-your-base/all_your_base_test.rb

                   output_base = 2
-                  expected = [0]
+                  expected = WHAT TO DO?

Contributor

Insti Dec 20, 2016

expected = [0]

Insti reviewed

View reviewed changes

exercises/all-your-base/all_your_base_test.rb

@@ @@ -175,7 +179,7 @@ def test_negative_digit @@
                   digits = [1, -1, 1, 0, 1, 0]
                   input_base = 2
                   output_base = 10
-                  expected = nil
+                  expected = WHAT TO DO?

Contributor

Insti Dec 20, 2016

raise error about invalid input

Contributor Author

tommyschaefer Dec 20, 2016

Currently we just expect the output to be nil. Should the cases with invalid data be changed to expect an error to be raised?

Insti reviewed

View reviewed changes

exercises/all-your-base/all_your_base_test.rb

@@ @@ -161,7 +165,7 @@ def test_leading_zeros @@
                   digits = [0, 6, 0]
                   input_base = 7
                   output_base = 10
-                  expected = [4, 2]
+                  expected = WHAT TO DO?

Contributor

Insti Dec 20, 2016 •

edited

Loading

expected = [4, 2]

Contributor Author

tommyschaefer Dec 20, 2016

After looking at it with some fresh eyes, I think this is the only example that I'll have trouble implementing.

For the other examples I can check whether their input and bases are valid and generate a test on that. Also the empty / 0 input cases shouldn't be hard to catch either.

Because we ignore leading zeros in the input, the expected output for the given inputs is not included in the shared test data. I'm not sure how to handle this case in a way that would allow the input or bases to change without new generations breaking. Do you have any ideas on this one?

Thank you so much for your help and time!

Insti reviewed

View reviewed changes

exercises/all-your-base/all_your_base_test.rb

@@ @@ -189,7 +193,7 @@ def test_invalid_positive_digit @@
                   digits = [1, 2, 1, 0, 1, 0]
                   input_base = 2
                   output_base = 10
-                  expected = nil
+                  expected = WHAT TO DO?

Contributor

Insti Dec 20, 2016

raise error about invalid input

Insti reviewed

View reviewed changes

exercises/all-your-base/all_your_base_test.rb

@@ @@ -203,7 +207,7 @@ def test_first_base_is_one @@
                   digits = []
                   input_base = 1
                   output_base = 10
-                  expected = nil
+                  expected = WHAT TO DO?

Contributor

Insti Dec 20, 2016 •

edited

Loading

raise error about invalid base.

Insti reviewed

View reviewed changes

exercises/all-your-base/all_your_base_test.rb

@@ @@ -217,7 +221,7 @@ def test_second_base_is_one @@
                   digits = [1, 0, 1, 0, 1, 0]
                   input_base = 2
                   output_base = 1
-                  expected = nil
+                  expected = WHAT TO DO?

Contributor

Insti Dec 20, 2016 •

edited

Loading

probably raise an error about invalid base.
I'd like to see a base 1 representation: [1] * 42 but that is not consistent with everything else.

Insti reviewed

View reviewed changes

exercises/all-your-base/all_your_base_test.rb

@@ @@ -231,7 +235,7 @@ def test_first_base_is_zero @@
                   digits = []
                   input_base = 0
                   output_base = 10
-                  expected = nil
+                  expected = WHAT TO DO?

Contributor

Insti Dec 20, 2016

raise error about invalid base.

Insti reviewed

View reviewed changes

exercises/all-your-base/all_your_base_test.rb

@@ @@ -245,7 +249,7 @@ def test_second_base_is_zero @@
                   digits = [7]
                   input_base = 10
                   output_base = 0
-                  expected = nil
+                  expected = WHAT TO DO?

Contributor

Insti Dec 20, 2016

raise error about invalid base.

Insti reviewed

View reviewed changes

exercises/all-your-base/all_your_base_test.rb

@@ @@ -259,7 +263,7 @@ def test_first_base_is_negative @@
                   digits = [1]
                   input_base = -2
                   output_base = 10
-                  expected = nil
+                  expected = WHAT TO DO?

Contributor

Insti Dec 20, 2016

raise error about invalid base.

Insti reviewed

View reviewed changes

exercises/all-your-base/all_your_base_test.rb

@@ @@ -273,7 +277,7 @@ def test_second_base_is_negative @@
                   digits = [1]
                   input_base = 2
                   output_base = -7
-                  expected = nil
+                  expected = WHAT TO DO?

Contributor

Insti Dec 20, 2016

raise error about invalid base.

Insti reviewed

View reviewed changes

exercises/all-your-base/all_your_base_test.rb

@@ @@ -287,7 +291,7 @@ def test_both_bases_are_negative @@
                   digits = [1]
                   input_base = -2
                   output_base = -7
-                  expected = nil
+                  expected = WHAT TO DO?

Contributor

Insti Dec 20, 2016

raise error about invalid base.

tommyschaefer commented

View reviewed changes

exercises/all-your-base/all_your_base_test.rb

		@@ -1,9 +1,10 @@
		gem 'minitest', '>= 5.0.0'

Contributor Author

tommyschaefer Dec 20, 2016 •

edited

Loading

@Insti Just wanted to double check that it's okay to remove this based on your comment on #509? Thanks!

Contributor

Insti Dec 20, 2016

yes. remove this line.

Contributor Author

tommyschaefer commented Dec 20, 2016

Hmm. The reason the checks are failing is because, without specifying the TargetRubyVersion in the .rubocop.yml file, it doesn't like keyword arguments.

Adding:

AllCops:
    TargetRubyVersion: 2.1

solves this problem.

Should I open an issue / PR for discussing that change, or should I include it in this PR?

Contributor

Insti commented Dec 20, 2016

specifying the TargetRubyVersion in the .rubocop.yml

Doing this as a separate PR would be great, thanks.

Insti reviewed

View reviewed changes

lib/all_your_base_cases.rb

@@ @@ -1,4 +1,8 @@ @@
               class AllYourBaseCase < OpenStruct
+                def self.build(row:, index:)
+                  new(PreProcessor.new(row: row, index: index).run)

Contributor

Insti Dec 20, 2016

You can get rid of this method completely by just calling the preprocessor directly on line 89

Insti reviewed

View reviewed changes

lib/all_your_base_cases.rb

+                attr_reader :row, :index
+                def expected_value
+                  return row['expected'] unless row['expected'].nil?

Contributor

Insti Dec 20, 2016

This is headed in the right direction.

An alternate method would be to special case each one based on the description (or the input values.)

Contributor Author

tommyschaefer Dec 20, 2016

Which do you prefer?

Personally, I like the fact that it's resilient to the test data changing how it is, but I can go both ways. It does feel a bit strange that some of the exercise logic is reproduced in the generator, though.

Member

kotp Dec 21, 2016

return row['expected'] if row['expected']

Contributor Author

tommyschaefer commented Dec 20, 2016

Just realized a couple comments were in outdated diffs, so I'll repost them here for visibility. One was:

Currently we just expect the output to be nil. Should the cases with invalid data be changed to expect an error to be raised?

The second was regarding the test for leading zeros:

Because we ignore leading zeros in the input, the expected output for the given inputs is not included in the shared test data. I'm not sure how to handle this case in a way that would allow the input or bases to change without new generations breaking. Do you have any ideas on this one? Is it okay to add a "hard-coded" special case for this?

tommyschaefer force-pushed the generate-ayb-tests branch 2 times, most recently from 696a63a to 49d4b75 Compare

December 20, 2016 20:00

Contributor Author

tommyschaefer commented Dec 20, 2016

Hmm... sorry. I unfortunately messed up the rebase ⬆️ Should be fixed now though.

Insti mentioned this pull request

[WIP] Test generation refactor Insti/exercism-ruby#1

Closed

Contributor

Insti commented Dec 22, 2016 •

edited

Loading

Currently we just expect the output to be nil. Should the cases with invalid data be changed to expect an error to be raised?

Yes these should raise an error.
(See bowling for some clues about this)

Because we ignore leading zeros in the input, the expected output for the given inputs is not included in the shared test data. I'm not sure how to handle this case in a way that would allow the input or bases to change without new generations breaking. Do you have any ideas on this one? Is it okay to add a "hard-coded" special case for this?

Hardcoding values in the pre-processor is acceptable. Changes to the canonical-data are allowed to break the test generation.

tommyschaefer commented

View reviewed changes

exercises/all-your-base/example.rb

@@ @@ -1,17 +1,22 @@ @@
               module BookKeeping
-                VERSION = 1
+                VERSION = 2

Contributor Author

tommyschaefer Dec 23, 2016

Bumped test version since we now expect invalid base / input to raise an error rather than return nil

tommyschaefer commented

View reviewed changes

exercises/all-your-base/example.rb

               end
               class BaseConverter
                 def self.convert(base_from, number_array, base_to)
-                  return if number_array.any?{|number| number < 0 || number >= base_from}
-                  return if base_from <= 1 || base_to <= 1
+                  fail ArgumentError if invalid_inputs?(base_from, number_array, base_to)

Contributor Author

tommyschaefer Dec 23, 2016

I tried to change as little as possible here to get tests to pass. Does this look okay?

Contributor

Insti Dec 23, 2016

that's fine.

Contributor

Insti commented Dec 23, 2016

This looks good @tommyschaefer.
Once @kotp or @bmulvihill have reviewed it one of them can merge it in.

tommyschaefer force-pushed the generate-ayb-tests branch from 72f3911 to 9c785ee Compare

December 28, 2016 06:41

tommyschaefer mentioned this pull request

Add phony target for test in Makefile #518

Merged

Insti reviewed

View reviewed changes

lib/all_your_base_cases.rb

-                  lines.split("\n").map do |line|
-                    ' ' * size + line
-                  end.join("\n")
+                  lines.lines.each_with_object('') { |line, obj| obj << ' ' * size + line }

Contributor

Insti Dec 28, 2016

lines.lines looks weird.
If you need to call lines on it that suggests to me that it's not lines at all, so either needs to be lines that get passed in or needs a different name.

Contributor Author

tommyschaefer Dec 28, 2016

Completely agreed.

Would text or code be better, maybe?

Contributor

Insti Dec 29, 2016

I have a slight preference for text but it doesn't make too much difference.

Insti reviewed

View reviewed changes

lib/all_your_base_cases.rb

+                    "",
+                    indent(4, "assert_raises ArgumentError do"),
+                    indent(6, "BaseConverter.convert(input_base, digits, output_base)"),
+                    indent(4, "end"),

Contributor

Insti Dec 29, 2016 •

edited

Loading

One thing that bugs me about this otherwise great solution is all the micro-management of indentation that is going on. (It's not just in this method, but throughout the whole class.)
There must be a more elegant way of handling it.

tommyschaefer force-pushed the generate-ayb-tests branch 4 times, most recently from 4a788a3 to af01f6c Compare

December 30, 2016 17:30

tommyschaefer mentioned this pull request

Run non-exercise tests in CI #523

Merged


          Generate tests for "all your base"

e9d07a7

tommyschaefer force-pushed the generate-ayb-tests branch from af01f6c to e9d07a7 Compare

December 30, 2016 18:28

Contributor

Insti commented Dec 30, 2016

Is there anything more than needs to be done here?

(@tommyschaefer you should come to the Gitter chat room: https://gitter.im/exercism/xruby)

Contributor Author

tommyschaefer commented Dec 30, 2016 •

edited

Loading

As far as I'm concerned, I don't have anything else to add 😄

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

Contributor

Insti commented Dec 30, 2016

I noticed this in the travis log:

/home/travis/build/exercism/xruby/lib/all_your_base_cases.rb:68: warning: private attribute?

Contributor Author

tommyschaefer commented Dec 30, 2016

I guess one thing to mention is that this generator isn't tested. Is that something you would like to see happen before thinking about bringing these changes in?

Contributor Author

tommyschaefer commented Dec 30, 2016

@Insti nice catch! I'll look into it 😄

tommyschaefer commented

View reviewed changes

lib/all_your_base_cases.rb


		private

		attr_reader :row

Contributor Author

tommyschaefer Dec 30, 2016

Hmm looks like the warning in Travis is coming from this line.

Contributor

Insti commented Dec 30, 2016

I guess one thing to mention is that this generator isn't tested. Is that something you would like to see happen before thinking about bringing these changes in?

You're welcome to add them if you want, but I'd be fine bringing it in without tests.

Generators can be easily tested by comparing the output with the previous version.
The whole generator process is still a bit "in flux" as we move towards a new, more integrated way of generating the tests files. Because of that everything is likely to change in the near future and the changes are likely to be directly comparable so the tests will need to get re-written anyway.


          Move private row attribute to avoid ruby warning

a23949e

This change is to remove a warning present in ruby versions
less than 2.3.0. A thread discussing this warning can be found
at https://bugs.ruby-lang.org/issues/10967. It is removed in
ruby/ruby@32a5a09.

Contributor Author

tommyschaefer commented Dec 30, 2016

You're welcome to add them if you want, but I'd be fine bringing it in without tests.

Sounds good. We can move forward without them as far as I'm concerned. I'll circle back around to these once the new generator stuff gets added.

Is it okay if I add an issue to remember this?

Contributor

Insti commented Dec 30, 2016

Is it okay if I add an issue to remember this?

Yep. That's one of the things they're for.

kotp reviewed

View reviewed changes

lib/all_your_base_cases.rb

                   def call(row)
                     @row = row
                     row.merge('expected' => expected_value)
                   end
+                  private :row
                   private

Member

kotp Jan 2, 2017

Why not:

    private

    attr_reader :row

Move the private things past the private declaration... Leaving call above that line.

Contributor Author

tommyschaefer Jan 2, 2017

This is what we were chatting about on Gitter the other day.

In ruby < 2.3.0 private reader methods made like that cause a warning. In 2.3 that warning is removed. Links to some info about this are in the last commit message.

Thanks for the feedback!

Member

kotp Jan 2, 2017 •

edited

Loading

This is the last commit, and instead of moving the line, one more line was created.
If you move the attr_reader to below the private line, there is no warning for 2.4 (in regards to that) that I am finding.

Contributor Author

tommyschaefer Jan 2, 2017

The warning was removed in ruby 2.3, so in all later versions it isn't there and you won't see it in 2.4. Since we use 2.1.2, it shows up in CI and whatnot.

I'm not sure if moving the reader under private but keeping the explicit private with method name will still make a warning. I'll give this a try when I'm home.

Sorry for the lack of code samples... aparently the iOS keyboard doesn't have a back tick key? 😞

Contributor Author

tommyschaefer Jan 2, 2017

Copying commit message here so it can be read inline:

This change is to remove a warning present in ruby versions
less than 2.3.0. A thread discussing this warning can be found
at https://bugs.ruby-lang.org/issues/10967. It is removed in
ruby/ruby@32a5a09.

Contributor Author

tommyschaefer Jan 3, 2017

Yeah, that's what I had originally but it's what causes the warning mentioned above. Ruby versions less than 2.3.0 have a warning when a private attribute is defined unless its made private explicitly with private :method_name

Member

kotp Jan 4, 2017

As it is a warning, and not functionally wrong, I would probably leave it at that. This also means as time goes on and the version of Ruby moves forward, it will eventually take care of itself.

Contributor

Insti Jan 4, 2017

I'd prefer that the code not emit warnings. If that means temporarily doing some sub-optimal things to keep the interpreter happy then so be it.

Member

kotp Jan 4, 2017

Definitely prefer warning free. How will we trigger when it should be "corrected" when the warning would naturally go away? Not that it is an important change...

Contributor

Insti Jan 4, 2017

I don't think it needs an explicit trigger. Next time that code gets edited, whoever modifies it will think it looks wrong, (unjustly) curse @tommyschaefer for being an idiot 😜 and fix it.

kotp changed the title ~~all-your-base: add test generator~~ WIP: all-your-base: add test generator

kotp and others added 2 commits

January 2, 2017 04:52


          A bit more idiomatic Ruby

0e96040

Also, the private call was ineffective.


          Merge pull request #4 from kotp/generate-ayb-tests

fc82a2c

Update all-your-base example to use more idiomatic ruby

Insti approved these changes

View reviewed changes

Contributor

Insti left a comment

I'm happy this is ready to go.

kotp merged commit 843767c into exercism:master

Contributor

Insti commented Jan 4, 2017

Thanks for all your work on this @tommyschaefer ❤️

kotp changed the title ~~WIP: all-your-base: add test generator~~ all-your-base: add test generator

Insti mentioned this pull request

DRY test generators #566

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet