Thoughts on Testing

[jaredbeck]

How are we going to test this repo? In https://github.com/paper-trail-gem/paper_trail/issues/1070 I suggested the following milestones for this project:

1. paper_trail gemspec has a runtime dependency on the new gem for a year or so, and keeps all existing tests.
2. paper_trail gemspec has a development dependency on the new gem for a year or so, and keeps all existing tests.
3. Eventually, paper_trail does not depend on or test the new gem

So, theoretically this repo could live without tests for the time being. But, in practice, it'll probably be helpful to have tests anyway, if for no other reason than to give us the confidence to make releases.

It will certainly be possible to write isolated unit tests in this repo, the kind of tests that don't touch a database and rely heavily on mocks. That would be better than nothing, and we could rely on the paper_trail test suite for integration testing.

We can start a branch in the paper_trail repo for the purpose of early integration testing. In this branch, we would delete the association tracking implementation.

These are just some early thoughts. What have you been thinking?

[westonganger]

Yeah sure get that remove_association_tracking branch started. It would give me something to run my tests against.

I'm more than happy with those milestones you've stated. It offers me pretty good support until the dependency is removed.

As for the tests. That sort of seems weird. I was thinking of moving all the association tests from paper_trail itself and keeping them only in the association plugin repo. It would be nice to have to be an easy way you can trigger/call the tests from my gem in your test suite, is that possible?

[jaredbeck]

As for the tests. That sort of seems weird. I was thinking of moving all the association tests from paper_trail itself and keeping them only in the association plugin repo.

I agree that should be the eventual goal, but I'd like to keep them in the paper_trail repo for a year or two (see suggested milestones above) to give me the confidence to release new versions of paper_trail, once it starts depending on this gem.

Yeah sure get that remove_association_tracking branch started. It would give me something to run my tests against.

OK, I'll get the branch started. Once it's in place, feel free to open PRs against it.

[westonganger]

Okay fair enough but I think its important that I have these tests ready to go within this repo so I will duplicate them. If anything is added / removed related to the association tests try to make a PR or mention me if you can.

[westonganger]

I have prepped everything for the new remove_association_tracking branch. I cannot really develop/test any further until that branch has been completed. I know your busy, have you had a chance to start on this yet?

[jaredbeck]

Awesome, you're moving fast! I will probably have time to work on it this weekend.

[jaredbeck]

Branch created: https://github.com/paper-trail-gem/paper_trail/tree/remove_association_tracking

Please take a look at the diff (https://github.com/paper-trail-gem/paper_trail/compare/master...remove_association_tracking) and give me your feedback.

[westonganger]

Looks good.

One thing that sucks is that I have to create/use a fork during testing just to remove the dependency paper_trail_association_tracking. If you know a way to get around this without forking that'd be great. Seems related to this https://github.com/wycats/bundler/issues/143

I have now updated the plugin code. All specs are passing however I would like your comments on the following todo items:

• Verify patches in RecordTrail for record_* methods, would prefer an approach that utilizes super
• Verify patch of Reifier#reify, would prefer an approach that utilizes super
• Verify patch of ModelConfig#setup still works correctly given some slight method ordering differences (Diff: https://github.com/paper-trail-gem/paper_trail/compare/master...remove_association_tracking)

[jaredbeck]

One thing that sucks is that I have to create/use a fork during testing just to remove the dependency paper_trail_association_tracking. If you know a way to get around this without forking that'd be great. Seems related to this wycats/bundler#143

We could do something a little crude like adding a conditional to the PT gemspec:

# paper_trail.gemspecc
unless ENV['PT_ASSOCIATION_TRACKING'] == 'false'
  gem 'paper_trail-association_tracking'
end

Would that help?

I have now updated the plugin code. All specs are passing ..

Awesome! It took a little fiddling with Appraisal, but the remove_association_tracking branch specs are passing too.

.. however I would like your comments on the following todo items:

• Verify patches in RecordTrail for record_* methods, would prefer an approach that utilizes super

I see you are overwriting eg. record_create. Yes, I agree that approach is brittle, and super would be much better. How about something like this?

def record_create
  version = super
  update_transaction_id(version)
  save_associations(version)
end

In the same way, it should be possible to use super in Reifier#reify.

Verify patch of ModelConfig#setup still works correctly given some slight method ordering differences (Diff: paper-trail-gem/paper_trail@master...remove_association_tracking)

I think calling setup_associations in this gem is incorrect. Despite its name, it has nothing to do with association tracking. Since PT is already calling it, you'd actually be calling it a second time here, which may cause problems. Likewise, installing (a second copy of) the clear_rolled_back_versions callback seems incorrect. Otherwise, the super-based patch of ModelConfig#setup looks good. :+1: The method order seems fine.

[westonganger]

Yes I think the environment variable is a nice solution to that problem

[westonganger]

Maybe the method setup_associations could be renamed to avoid possible future confusion.

[westonganger]

I made a PR to fix the return values for the record_* methods. Otherwise all patches have been improved on my end. Looks like were getting close.

[jaredbeck]

I made a PR to fix the return values for the record_* methods. Otherwise all patches have been improved on my end. Looks like were getting close.

eae5ccf looks good, and CI for the remove_association_tracking branch (https://travis-ci.org/paper-trail-gem/paper_trail/builds/385360300) is still passing.

When you're ready, please push a gem and I'll point the remove_association_tracking branch at that.

[westonganger]

For some reason now I'm getting failing tests in Ruby 2.5 only (2.4 or 2.3 still passes normally). Do you have some insight to this? It appears to have started with eae5ccfad2b575bc48ab053174e017965cf75c94 but im having trouble determining the cause. Test seem fine on your end using the latest.

[jaredbeck]

For some reason now I'm getting failing tests in Ruby 2.5 only (2.4 or 2.3 still passes normally). Do you have some insight to this? It appears to have started with eae5ccf but im having trouble determining the cause. Test seem fine on your end using the latest.

Yes, CI of fafecfa is passing (https://travis-ci.org/paper-trail-gem/paper_trail/builds/385775323). I can't look into it further right now, need to get back to the day job :grin:

[jaredbeck]

When you're ready, please push a gem and I'll point the remove_association_tracking branch at that.

I would like to begin testing the the remove_association_tracking branch against a gem, so I pushed one. I am happy to transfer ownership of it to you once you have a rubygems.org account. In fact, I already tried to do that, but I got:

gem owner paper_trail-association_tracking --add weston@westonganger.com
Adding weston@westonganger.com: Owner could not be found.

So, please let me know when you've signed up and I'll transfer it.

[westonganger]

The test suite is failing all sorts of randomly for ruby 2.5 only. Right now I'm a little stumped, tbh I'm not a test suite ninja yet. Once that's fixed the irrelevant tests must be removed from the test suite and then the first gem can be published.

[westonganger]

My current rubygems account is westonganger@gmail.com

[jaredbeck]

My current rubygems account is westonganger@gmail.com

gem owner paper_trail-association_tracking --add westonganger@gmail.com
Owner added successfully.

The test suite is failing all sorts of randomly for ruby 2.5 only. Once that's fixed

Anything specific I can help with? PT CI, testing against the PT-AT gem I just pushed, is passing (https://travis-ci.org/paper-trail-gem/paper_trail/builds/387567399) So, from what I'm seeing I'm ready to merge the remove_association_tracking branch into PT master.

[jaredbeck]

The test suite is failing all sorts of randomly for ruby 2.5 only

What tests are failing for you? I am noticing, in any version of ruby, that the shared_examples "it delegates to request" in paper_trail_spec.rb will fail, and then other seemingly unrelated tests will fail. If I delete those shared examples, the rest of the test suite passes reliably (repeatably). Are you seeing the same?

[westonganger]

Yep that was it. The error seems to be from setting the spy on the request object which must cause problems because its global.

[jaredbeck]

I wonder if spies are incompatible with the complicated combination of class << self and Module.prepend we are using.

Making matters more confusing, why would these specs work fine in PT builds (https://travis-ci.org/paper-trail-gem/paper_trail/builds/387567399) and not in PT-AT builds?

If it comes down to it, they are some of the least important tests in the suite, so I'm fine with deleting them.

[westonganger]

I'm done with all my todo items. Once the remove_association_tracking branch is merged to PT master, then I can remove the git sources in Gemfile and gemfiles/. Pending that, I think this gem should be ready for a full v1.0.0 release.

[jaredbeck]

Once the remove_association_tracking branch is merged to PT master ..

Merged! :tada:

.. then I can remove the git sources in Gemfile and gemfiles/. Pending that, I think this gem should be ready for a full v1.0.0 release.

In the PT .gemspec, the current dependency is:

add_dependency "paper_trail-association_tracking", "0.0.1"

I can change the constraint to "< 1.1". Is that too strict? If you are planning to follow SemVer I can do "< 2". What constraint would you prefer?

[westonganger]

I would prefer "< 2"

[westonganger]

Gem v1.0.0 released