Don't use reset_callbacks in tests

[sgrif]

The test suite attempts to modify a model, and then call
reset_callbacks to clean up after itself. However, reset_callbacks
removes all callbacks from a class, including those that Active Record
defines internally. Calling reset_callbacks on a class like this
basically blows away a lot of our internals.

What specifically Active Record uses callbacks for is not public API
that can be depended on, but a guarantee that it does make is that user
defined callbacks will occur after Active Record defined ones (with some
caveats like associations which are just in definition order).

Callbacks in Rails 3 were... interesting. I could not for the life of me
get it to properly clean up, and the code required to do it well would
be horrendous, so I've just left that version alone.

On that same note, since it looks like the next version is planned to be 6.0.0, how would you feel about dropping support for Rails 3 in that version? It has reached end of life, meaning it no longer receives security patches. We don't recommend that gem authors continue to support it. Users stuck on 3.2 for whatever reason can continue to use versions of paper_trail that are already released.

This would simplify this patch, as well as the 5.1 compatibility code.

[sgrif]

Failures are due to master, not this PR

[jaredbeck]

Failures are due to master, not this PR

#897 merged, please rebase.

On that same note, since it looks like the next version is planned to be 6.0.0, how would you feel about dropping support for Rails 3 in that version? It has reached end of life, meaning it no longer receives security patches. We don't recommend that gem authors continue to support it.

We could continue maintaining PT 5 if we had to, so I think it's fine to drop support for rails 3 in PT 6. If Ben agrees, I'd be happy to make that change. I'll just update the gemspec and drop testing support, for starters. We can clean up the conditionals in the code over time.

[sgrif]

@jaredbeck CI is green. If this is good to merge I can push up the Rails 5.1 support PR.

[jaredbeck]

@jaredbeck CI is green. If this is good to merge I can push up the Rails 5.1 support PR.

Great. I'll merge this to keep the ball rolling, but if we do decide to drop AR3 support (in #898) then we should come back to this and remove the AR3 branch of your conditional.

[jaredbeck]

Thanks for this, by the way. I always thought that our use of reset_callbacks was sketchy. The way you've done setup/teardown was interesting too, specifically the use of lexical scope to hold the original callbacks. I usually try to avoid multiple setups because their order of execution starts to matter, but in this case I think it's fine. Another technique I've used in the past for sharing state between setup and teardown is an instance variable.

[sgrif]

Yeah, the reason I did lexical scope instead of an instance variable was to avoid having to deal with namespace collisions and/or any sort of weirdness with a hash. In theory if you wanted to do something like

context "foo" do
  reset_callbacks(Foo, :bar)

  setup do
    Foo.before_bar { baz }
  end

  context "nested" do
    reset_callbacks(Foo, :bar)

    setup do
      Foo.before_bar { more_baz }
    end
  end
end

you could, which you couldn't do if you assumed a 1-to-1 mapping between callback chain and reset. Of course at that point the tests can just go die in a fire, but yeah it was just simpler.