Make the object column optional

[seanlinsley]

Part 1 of #1099

[jaredbeck]

Hi Sean. Just FYI that I haven't had a chance to review this yet because I've been focusing on #1108.

[jaredbeck]

This looks great, Sean. My only concern is the one little thing with the linter.

[jaredbeck]

Should we try to do this without offending the linter? We could use before(:each), right?

[seanlinsley]

Normally I'd avoid doing this sort of thing in a before(:each) b/c that'd mean doing four extra database requests per test (removing the column, adding the column, and 2x reloading the schema).

It appears that the Rubocop error is concerned with transactional fixtures in Rails:

spec/paper_trail/model_spec.rb:882:5: C: RSpec/BeforeAfterAll: Beware of using before :all as it may cause state to leak between tests. If you are using rspec-rails, and use_transactional_fixtures is enabled, then records created in before :all are not automatically rolled back.
    before :all do
    ^^^^^^^^^^^
spec/paper_trail/model_spec.rb:886:5: C: RSpec/BeforeAfterAll: Beware of using after :all as it may cause state to leak between tests. If you are using rspec-rails, and use_transactional_fixtures is enabled, then records created in after :all are not automatically rolled back.
    after :all do

So perhaps we should disable this cop globally.

[seanlinsley]

Ah, apparently we do use transactional fixtures:

paper_trail/spec/spec_helper.rb

Lines 62 to 74 in 7b1d19c

	if active_record_gem_version < ::Gem::Version.new("5")
	require "database_cleaner"
	DatabaseCleaner.strategy = :truncation
	RSpec.configure do |config|
	config.use_transactional_fixtures = false
	config.before { DatabaseCleaner.start }
	config.after { DatabaseCleaner.clean }
	end
	else
	RSpec.configure do |config|
	config.use_transactional_fixtures = true
	end
	end

Disallowing before(:all) seems to be preferable if your test suite is run in parallel: rubocop/rubocop-rspec#108. Is that something that's needed for this test suite? Probably not, since the tests on CI take 1-4 minutes.

[seanlinsley]

That research ended up being fairly inconclusive. I'll update this to use before(:each) if you'd still prefer that approach.

[jaredbeck]

That research ended up being fairly inconclusive. I'll update this to use before(:each) if you'd still prefer that approach.

I’m not sure. It seems the cop has some value but not in this case. Maybe leave the in-code exception for now?