[alphametics] Warn about long running tests in the common test data. #469

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

Ignore test cases based on the complexity of the expected result.

Hardcoded commented out versions of the hard(er) tests along with
instructions on why you might want to enable them.

Refactored lib/alphametics_cases.rb.
Use the common generator method names.
Tidied up expected value string generation code.

Re-generate tests based on version based on proposed changes.
exercism/problem-specifications#425

Update example.rb to pass the current version of tests.

Ignore test cases based on the complexity of the expected result. Hardcoded commented out versions of the hard(er) tests along with instructions on why you might want to enable them. Refactored lib/alphametics_cases.rb. Use the common generator method names. Tidied up expected value string generation code. Re-generate tests based on version based on proposed changes. exercism/problem-specifications#425 Update `example.rb` to pass the current version of tests.

Insti · 2016-10-24T21:42:53Z

exercises/alphametics/alphametics_test.rb

-    actual = Alphametics.new.solve('A == B')
-    assert_equal(expect, actual)
+    input = 'A == B'
+    expected = {}


Invalid solutions now expect an empty hash rather than a nil value.
This may be controversial, but I strongly believe that a method should always return the same type of object.

Insti · 2016-10-24T21:44:13Z

exercises/alphametics/alphametics_test.rb

-    assert_equal(expect, actual)
+    input = 'I + BB == ILL'
+    expected = { 'B' => 9, 'I' => 1, 'L' => 0 }
+    assert_equal expected, Alphametics.solve(input)


Was: Alphametics.new.solve(input)
Now: Alphametics.solve(input)
Instantiating an object is unnecessary, so it now uses a ~~instance~~ class method.

Edit: It's NOT using an instance method.

In the final test, 'AND + A + STRONG + OFFENSE + AS + A + GOOD = DEFENSE', the = should be ==. Also, in the commentary, optimsing should be optimising.

Insti · 2016-10-24T21:47:57Z

exercises/alphametics/example.tt

+  # These tests have been commented out due their long runtime.  If you are
+  # interested in optimsing your solution for speed these are a good tests to
+  # try.
+  #


The 'long' tests are ignored by the test case generator but are hardcoded here for reference.
It would also be possible (but more complicated) to generate them (I think I forgot that's what I started out trying to do.)

Insti · 2016-10-24T21:49:33Z

lib/alphametics_cases.rb

    row = row.merge('index' => i)
    AlphameticsCase.new(row)
  end
+
+  # The example algorithm takes a long time to solve these.
+  testcases.reject { |testcase| (testcase.expected||{}).size > 7 }


Test cases are ignored based on how many letters there are in the expected output.
Things started slowing down on my computer after 7 so that's where I drew the line.

bmulvihill · 2016-10-25T00:07:47Z

This looks/works good to me!

kotp · 2016-10-25T05:31:46Z

@moveson will be helping with this, if I am reading the comments on his solution correctly.

Insti · 2016-10-25T06:56:04Z

The reason the tests were commented out is because it was taking the example solver too long to solve them during CI checks.

Once @moveson contributes his example.rb we will no longer need to comment out the "slow" tests since his solver is fast enough to solve them quickly.

Should they still remain commented out in order to help the people who are trying to solve the problem?

By including them, we make it a much harder problem by implying that you also need to solve the 8 and 10 letter cases, but by having them commented out we have a simpler problem but with some easily accessible extensions to suggest in the solution reviews on the site.

My vote would be to keep them commented out.

kotp · 2016-10-25T13:18:55Z

Place them at the end, so that by the time they are commenting out the skip it is apparent if they have a slow solution or not. Place a comment there near the skip that states perhaps they can submit their current solution, without the following tests having ran.

bmulvihill · 2016-10-25T14:33:20Z

I like @kotp's solution. It keeps the template the same as the others, and it follows the simple pattern of uncommenting the skip to open a test up.

Insti · 2016-10-29T07:18:09Z

Waiting on @moveson to submit the improved example.rb

moveson · 2016-10-31T03:17:00Z

The new example.rb is ready. What's the next step?

Insti · 2016-10-31T07:58:57Z

@moveson create a new pull request that only updates the example.rb file.
Once that's there I can do what needs to be done to merge it into this one.

moveson · 2016-11-02T04:28:27Z

This is ready when you are. See PR #477.

Insti · 2016-11-02T09:19:32Z

I still need to merge in @moveson's example.rb and remove the test case filtering from the *_cases.rb file. I will do this when I get some time over the next few days.

… permutations.

Re-add the "slow" test cases with a comment about them being slow.

Insti · 2016-11-02T13:46:48Z

I've pulled in @moveson's solver and re-added the slow testcases with comments warning that they might be slow.

@bmulvihill can you have a look over it and see if it all looks ok now?

Insti · 2016-11-02T13:47:29Z

exercises/alphametics/alphametics_test.rb

+
+  # The obvious algorithm can take a long time to solve this puzzle,
+  # but an optimised solution can solve it fairly quickly.
+  # (It's OK to submit your solution without getting this test to pass.)


Is the wording of this comment OK?

Insti mentioned this pull request Oct 24, 2016

All generators use common interface #463

Closed

Insti commented Oct 24, 2016

View reviewed changes

Insti added the in progress label Oct 24, 2016

Fix typos

336a911

This was referenced Oct 31, 2016

Fast alphametics solver (uses simplified partial equations to limit… #476

Closed

Fast alphametics solver that uses partials to limit the set of viable… #477

Closed

kotp added ready and removed in progress labels Nov 2, 2016

Insti added in progress and removed ready labels Nov 2, 2016

moveson and others added 4 commits November 2, 2016 12:31

Fast alphametics solver that uses partials to limit the set of viable…

ea2ad66

… permutations.

Re-add slow cases with warning comment.

a270ac3

Re-add the "slow" test cases with a comment about them being slow.

Slow test cases re-inserted as real tests.

6286608

Move newlines out of indentation method.

3ded5e9

Insti commented Nov 2, 2016

View reviewed changes

Insti changed the title ~~[alphametics] Ignore long running tests in the common test data.~~ [alphametics] Warn about long running tests in the common test data. Nov 2, 2016

Remove redundant code/comments

a5dbabe

This was referenced Nov 3, 2016

new exercise alphametics exercism/haskell#410

Merged

Alphametics exercism/go#159

Closed

Insti merged commit 5ac769e into exercism:master Nov 5, 2016

Insti deleted the alphametics_generator branch November 5, 2016 09:54

Insti removed the in progress label Nov 5, 2016

petertseng mentioned this pull request Nov 8, 2016

dominoes: Add generator and example #484

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[alphametics] Warn about long running tests in the common test data. #469

[alphametics] Warn about long running tests in the common test data. #469

Insti commented Oct 24, 2016

Insti Oct 24, 2016

Insti Oct 24, 2016 •

edited

Loading

moveson Oct 28, 2016 •

edited

Loading

Insti Oct 24, 2016

Insti Oct 24, 2016

bmulvihill commented Oct 25, 2016

kotp commented Oct 25, 2016

Insti commented Oct 25, 2016 •

edited

Loading

kotp commented Oct 25, 2016

bmulvihill commented Oct 25, 2016

Insti commented Oct 29, 2016

moveson commented Oct 31, 2016

Insti commented Oct 31, 2016

moveson commented Nov 2, 2016

Insti commented Nov 2, 2016

Insti commented Nov 2, 2016

Insti Nov 2, 2016

kotp Nov 2, 2016

[alphametics] Warn about long running tests in the common test data. #469

[alphametics] Warn about long running tests in the common test data. #469

Conversation

Insti commented Oct 24, 2016

Insti Oct 24, 2016

Choose a reason for hiding this comment

Insti Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

moveson Oct 28, 2016 • edited Loading

Choose a reason for hiding this comment

Insti Oct 24, 2016

Choose a reason for hiding this comment

Insti Oct 24, 2016

Choose a reason for hiding this comment

bmulvihill commented Oct 25, 2016

kotp commented Oct 25, 2016

Insti commented Oct 25, 2016 • edited Loading

kotp commented Oct 25, 2016

bmulvihill commented Oct 25, 2016

Insti commented Oct 29, 2016

moveson commented Oct 31, 2016

Insti commented Oct 31, 2016

moveson commented Nov 2, 2016

Insti commented Nov 2, 2016

Insti commented Nov 2, 2016

Insti Nov 2, 2016

Choose a reason for hiding this comment

kotp Nov 2, 2016

Choose a reason for hiding this comment

Insti Oct 24, 2016 •

edited

Loading

moveson Oct 28, 2016 •

edited

Loading

Insti commented Oct 25, 2016 •

edited

Loading