Pass ar5 tests #689

owenr · 2016-01-12T03:17:59Z

Update test code to address deprecations/warnings and fix one legitimate issue with AR5. Not happy with several aspects of this change:

Gem rails-controller-testing does not work as advertised (possibly an issue with load order?) and I could only get it working when manually included, see test_helper.rb
HTTP request test methods in Rails 5 need parameter hash keys named 'params', so a new wrapper function is introduced twice (once in test_helper, again in spec_helper)
Addressing the AR5 issue seems to require a way of preventing #save on the has_one object, and there is almost certainly a better way than temporarily redefining #save on the instance

Having said that, the changes pass tests locally on MRI 2.3 (ar5) and 1.9.3-p551 (ar3/ar4). Comments and suggestions welcome.

@jaredbeck

owenr · 2016-01-12T04:31:14Z

Having some issues with tests intermittently failing. Any ideas on that are also welcome.

bronson · 2016-01-12T22:11:28Z

Interesting... Ten tests failed the first time I ran rake, but that was it. Now I can't get any to fail.

Here's the log: https://gist.github.com/bronson/70dd3c4aaf395df4bf93

And, if I rm -r test/dummy/db/*.sqlite3 and run rake again, I get the same ten errors. Looks to be an issue related to a fresh database?

Is that the problem you're seeing?

bronson · 2016-01-12T22:20:12Z

I'm at 25 iterations, no failures yet. I'll let it run for a few hundred.

ctr=; while rake; do echo ITERATION $((ctr += 1)); done

But, no, I'm not seeing any intermittent errors on your branch.

I'm using sqlite, are you testing on a different database?

jaredbeck · 2016-01-12T22:21:06Z

Owen, thanks for this! I'll review what I can this week.

.. if I rm -r test/dummy/db/*.sqlite3 and run rake again, I get the same ten errors. Looks to be an issue related to a fresh database?

Scott, Yeah, that's a known issue with running the local test suite. If you can fix it, that'd be great.

bronson · 2016-01-12T23:04:32Z

260 iterations, 0 failures. I don't see any intermittent issues. (sqlite, mac, ruby 2.2.2)

owenr · 2016-01-13T02:52:13Z

Initially tested using sqlite3 locally, no issues. As you can see, Travis CI failed all my builds for various reasons; it looks like Postgresql is the main issue now. Simulating locally it took me 22 runs before this failure (postgres, mac, 2.3.0):

PaperTrailCleanerTest#test_: `clean_versions!` method No options provided should removes the earliest version(s).  [/Users/owen/dev/paper_trail/test/unit/cleaner_test.rb:34]:
--- expected
+++ actual
@@ -1 +1 @@
-["Aglae Schaden", "Carley Doyle", "Karianne McDermott"]
+["Aglae Schaden", "Carley Doyle", "Jaime Witting"]

bronson · 2016-01-13T09:34:27Z

Switched to postgres and I finally got a failure after 76 iterations: https://gist.github.com/bronson/01f05504f81efe072b40

Glancing at it, maybe it died because name contained a single quote... A sql injection opportunity? :) I'll look closer tomorrow if nobody beats me to it.

Also, there might be a test order dependency. At least:

rspec --seed=0: 0 errors
rspec --seed=1: 1 error
rspec --seed=9: 10 errors

Do the tests have a required order?

owenr · 2016-01-14T04:00:18Z

Confirmed, unescaped input (only when using postgres). I've written some failing tests and will open an issue. Unfortunately it seems unrelated to the failures from Travis here and here.

I'm not familiar with the impact of seeding on this test suite and I couldn't reproduce those spec failures with seeds 1 and 10 (mac, postgres, ruby 2.2.2).

jaredbeck · 2016-01-14T15:52:16Z

When comparing gem versions, I like to use Gem::Version, e.g.

- ActiveRecord::VERSION::STRING >= '5.'
+ Gem::Version.new(ActiveRecord::VERSION::STRING) >= Gem::Version.new("5")

PS: If we didn't have to support AR3, we could use the more convenient ActiveRecord.gem_version

jaredbeck · 2016-01-14T16:01:28Z

lib/paper_trail/reifier.rb

@@ -162,6 +164,15 @@ def reify_has_ones(transaction_id, model, options = {})
        end
      end

+      # Suppress #save so we can reassociate with the reified master of a has_one relationship


Can you expand on this comment? Specifically, please explain what changed in AR5.

Also, please alphabetize methods.

jaredbeck · 2016-01-15T04:51:24Z

Owen, I've been cherry-picking the less controversial commits out of here.

I thought there was no sense in waiting, but it will mean you'll have to rebase this PR, sorry.

owenr · 2016-01-16T20:26:11Z

No problem at all; like I said, this code is not right and should be improved before merging.

Some comments:

I've reordered methods alphabetically as requested.
Switched to Gem::Version for comparisons, but note that comparison with '5' is insufficient ('5' is not >= '5.0.0.beta').
I can't find the AR5 change to has_one, but from the symptoms it looks like the associated object is persisted before the main object (which seems correct as per the AR4 documentation). I've elaborated on the comment.

jaredbeck · 2016-01-20T17:12:37Z

Owen, It looks like this needs another rebase, sorry.

jaredbeck · 2016-01-20T17:17:52Z

test/test_helper.rb

+    ActionDispatch::IntegrationTest.send(:include, Rails::Controller::Testing::Integration)
+
+    ActionView::TestCase.send(:include, Rails::Controller::Testing::TemplateAssertions)
+  end


This whole on_load could use a comment, e.g. why are we including these modules, why are we using send, why on_load, etc. I don't personally understand any of it, so the comment is mostly for me :p

Copied wholesale from rails-controller-testing. I don't know why putting it here works but requiring the library doesn't. I've also tried require: false in the Gemfile and manually requiring it instead but that doesn't help. Removing this code, I get:

NoMethodError: assigns has been extracted to a gem. To continue using it, add `gem 'rails-controller-testing'` to your Gemfile.

Not knowing if this is caused by something in PaperTrail or an issue with rails-controller-testing itself (or Rails 5), I'm not sure whether I should raise an issue over there.

jaredbeck · 2016-01-20T18:40:40Z

HTTP request test methods in Rails 5 need parameter hash keys named 'params'

Is this change documented in a changelog somewhere, or a guide?

jaredbeck · 2016-01-20T18:55:42Z

HTTP request test methods in Rails 5 need parameter hash keys named 'params'

Is this change documented in a changelog somewhere, or a guide?

Found it. It's in the release notes for ActionPack 5:

Migrating to keyword arguments syntax in ActionController::TestCase and ActionDispatch::Integration HTTP request methods. (Pull Request)
http://edgeguides.rubyonrails.org/5_0_release_notes.html#action-pack

jaredbeck · 2016-01-20T18:57:23Z

spec/spec_helper.rb

+  else
+    args
+  end
+end


Please add a comment explaining the change to ".. keyword arguments syntax in ActionController::TestCase and ActionDispatch::Integration HTTP request methods." and include a link to the ActionPack changelog (https://github.com/tgxworld/rails/blob/master/actionpack/CHANGELOG.md)

owenr · 2016-01-21T03:05:31Z

Rebased. I also fixed one more deprecation warning (use_transactional_tests) and added a note about 1-second differences in tests (for MySQL and AR5). Still not happy.

owenr · 2016-01-22T03:18:22Z

I think this is just about done. Please review and let me know; I'll squash the commits.

jaredbeck · 2016-01-24T20:49:00Z

This looks good to me. I will merge this unless Ben has any concerns @batter

I wish we didn't have to time-travel just to make MySQL happy, but it is something we've run into before (#314, 1d120ab)

owenr · 2016-01-24T23:11:01Z

Time travel is required due to a change in ActiveRecord 5 (if no attributes change, nothing is done). We could sleep instead of travel if that's preferred.
I've squashed the commits (and backed out an unneeded change to rubocop_todo picked up in review).

Pass ar5 tests

jaredbeck · 2016-01-26T18:36:28Z

Merged. Thanks for your work on this, Owen.

bronson · 2016-01-26T19:36:57Z

Wooo! Great job Owen.

jaredbeck reviewed Jan 14, 2016
View reviewed changes

owenr force-pushed the pass_ar5_tests branch from 9de02cb to 85733fe Compare January 16, 2016 20:13

owenr mentioned this pull request Jan 19, 2016

Intermittent failure when running Postgres tests: undefined method 'name' in clean_versions! #699

Closed

jaredbeck reviewed Jan 20, 2016
View reviewed changes

owenr force-pushed the pass_ar5_tests branch from 4fdc96b to 2948587 Compare January 21, 2016 03:03

owenr mentioned this pull request Jan 21, 2016

Adding rails-controller-testing** gem does not fix NoMethodError message rails/rails-controller-testing#5

Closed

owenr force-pushed the pass_ar5_tests branch from c8725a7 to 78125c1 Compare January 24, 2016 23:08

Update PaperTrail and test suite for ActiveRecord 5 compatibility

b9d9fb2

owenr force-pushed the pass_ar5_tests branch from 78125c1 to b9d9fb2 Compare January 25, 2016 00:57

jaredbeck added a commit that referenced this pull request Jan 26, 2016

Merge pull request #689 from owenr/pass_ar5_tests

d68587b

Pass ar5 tests

jaredbeck merged commit d68587b into paper-trail-gem:master Jan 26, 2016

teadur mentioned this pull request Sep 16, 2019

Upgrade paper_trail to Rails 5.x/6.X compatible version internetee/registry#1288

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass ar5 tests #689

Pass ar5 tests #689

owenr commented Jan 12, 2016

owenr commented Jan 12, 2016

bronson commented Jan 12, 2016

bronson commented Jan 12, 2016

jaredbeck commented Jan 12, 2016

bronson commented Jan 12, 2016

owenr commented Jan 13, 2016

bronson commented Jan 13, 2016

owenr commented Jan 14, 2016

jaredbeck commented Jan 14, 2016

jaredbeck Jan 14, 2016

jaredbeck commented Jan 15, 2016

owenr commented Jan 16, 2016

jaredbeck commented Jan 20, 2016

jaredbeck Jan 20, 2016

owenr Jan 21, 2016

jaredbeck commented Jan 20, 2016

jaredbeck commented Jan 20, 2016

jaredbeck Jan 20, 2016

owenr commented Jan 21, 2016

owenr commented Jan 22, 2016

jaredbeck commented Jan 24, 2016

owenr commented Jan 24, 2016

jaredbeck commented Jan 26, 2016

bronson commented Jan 26, 2016

Pass ar5 tests #689

Pass ar5 tests #689

Conversation

owenr commented Jan 12, 2016

owenr commented Jan 12, 2016

bronson commented Jan 12, 2016

bronson commented Jan 12, 2016

jaredbeck commented Jan 12, 2016

bronson commented Jan 12, 2016

owenr commented Jan 13, 2016

bronson commented Jan 13, 2016

owenr commented Jan 14, 2016

jaredbeck commented Jan 14, 2016

jaredbeck Jan 14, 2016

Choose a reason for hiding this comment

jaredbeck commented Jan 15, 2016

owenr commented Jan 16, 2016

jaredbeck commented Jan 20, 2016

jaredbeck Jan 20, 2016

Choose a reason for hiding this comment

owenr Jan 21, 2016

Choose a reason for hiding this comment

jaredbeck commented Jan 20, 2016

jaredbeck commented Jan 20, 2016

jaredbeck Jan 20, 2016

Choose a reason for hiding this comment

owenr commented Jan 21, 2016

owenr commented Jan 22, 2016

jaredbeck commented Jan 24, 2016

owenr commented Jan 24, 2016

jaredbeck commented Jan 26, 2016

bronson commented Jan 26, 2016