Skip to content

Optional ActiveRecord config #2038

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,4 @@ bundle
specs.out
spec/examples.txt
specs.out
test.db
4 changes: 4 additions & 0 deletions lib/generators/rspec/install/templates/spec/rails_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@
end
<% end -%>
RSpec.configure do |config|
# You can explicitly turn off ActiveRecord support on rspec-rails by
# uncommenting the following line.
# config.use_active_record = false

<% if RSpec::Rails::FeatureCheck.has_active_record? -%>
# Remove this line if you're not using ActiveRecord or ActiveRecord fixtures
config.fixture_path = "#{::Rails.root}/spec/fixtures"
Expand Down
1 change: 1 addition & 0 deletions lib/rspec/rails/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ def self.initialize_configuration(config)
config.add_setting :infer_base_class_for_anonymous_controllers, default: true

# fixture support
config.add_setting :use_active_record, default: true
config.add_setting :use_transactional_fixtures, alias_with: :use_transactional_examples
config.add_setting :use_instantiated_fixtures
config.add_setting :global_fixtures
Expand Down
15 changes: 8 additions & 7 deletions lib/rspec/rails/fixture_support.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@ module RSpec
module Rails
# @private
module FixtureSupport
if defined?(ActiveRecord::TestFixtures)
extend ActiveSupport::Concern
include RSpec::Rails::SetupAndTeardownAdapter
include RSpec::Rails::MinitestLifecycleAdapter
include RSpec::Rails::MinitestAssertionAdapter
include ActiveRecord::TestFixtures
extend ActiveSupport::Concern

included do
if RSpec.configuration.use_active_record? && defined?(ActiveRecord::TestFixtures)
include RSpec::Rails::SetupAndTeardownAdapter
include RSpec::Rails::MinitestLifecycleAdapter
include RSpec::Rails::MinitestAssertionAdapter
include ActiveRecord::TestFixtures

included do
self.fixture_path = RSpec.configuration.fixture_path
if ::Rails::VERSION::STRING > '5'
self.use_transactional_tests = RSpec.configuration.use_transactional_fixtures
Expand Down
3 changes: 3 additions & 0 deletions spec/rspec/rails/configuration_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@
:use_transactional_fixtures,
alias_with: :use_transactional_examples

include_examples "adds setting", :use_active_record,
:default => true

include_examples "adds setting", :use_instantiated_fixtures

include_examples "adds setting", :global_fixtures
Expand Down
55 changes: 54 additions & 1 deletion spec/rspec/rails/fixture_support_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ module RSpec::Rails
RSpec.describe FixtureSupport do
context "with use_transactional_fixtures set to false" do
it "still supports fixture_path" do
allow(RSpec.configuration).to receive(:use_transactional_fixtures) { false }
allow(RSpec.configuration).to \
receive(:use_transactional_fixtures) { false }
group = RSpec::Core::ExampleGroup.describe do
include FixtureSupport
end
Expand All @@ -21,5 +22,57 @@ module RSpec::Rails

expect { group.new.setup_fixtures }.to_not raise_error
end

context "without database available" do
let(:example_group) do
RSpec::Core::ExampleGroup.describe("FixtureSupport") do
include FixtureSupport
include RSpec::Rails::MinitestLifecycleAdapter
end
end
let(:example) do
example_group.example("foo") do
expect(true).to be(true)
end
end

RSpec.shared_examples_for "unrelated example raise" do
it "raise due to no connection established" do
expect(example_group.run).to be(false)
expect(example.execution_result.exception).to \
be_a(ActiveRecord::ConnectionNotEstablished)
end
end

RSpec.shared_examples_for "unrelated example does not raise" do
it "does not raise" do
expect(example_group.run).to be(true)
expect(example.execution_result.exception).not_to \
be_a(ActiveRecord::ConnectionNotEstablished)
end
end

before { clear_active_record_connection }

after { establish_active_record_connection }

context "with use_active_record set to false" do
before { RSpec.configuration.use_active_record = false }

after { RSpec.configuration.use_active_record = true }

include_examples "unrelated example does not raise"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This spec does nothing to demonstrate that the support works when active record is available but not configured, you should be able to demonstrate the issue without adding database support as its supposed to be broken without database support.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I don't get your point. When use_active_record is set to true and no AR connection is available, it raises an ActiveRecord::ConnectionNotEstablished. When use_active_record is set to false and no AR connection is available it does not raise. Could you explain in detail what you have in mind for another test?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its artificial because you have configured a database then manually cleared it.

The default is no configuration, the changes adding config should be removed and these spec should demonstrate that requiring' active_record' causes an error, and that setting use_active_record false prevents it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I've been trying to find time to work on this myself because I believe strongly that adding database configuration for all the specs is not the way to go! :)

end

context "with use_active_record set to true" do
before { RSpec.configuration.use_active_record = true }

if Rails.version.to_f >= 4.0
include_examples "unrelated example does not raise"
else
include_examples "unrelated example raise"
end
end
end
end
end
25 changes: 21 additions & 4 deletions spec/support/ar_classes.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,24 @@
ActiveRecord::Base.establish_connection(
:adapter => 'sqlite3',
:database => ':memory:'
)
FileUtils.rm_f('./test.db')

def establish_active_record_connection
ActiveRecord::Base.establish_connection(
:adapter => 'sqlite3',
:database => './test.db'
)
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Author

@Jack12816 Jack12816 Oct 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the spec.

    context "without database available" do
       before { clear_active_record_connection }
       after { establish_active_record_connection }
       context "with use_active_record set to true" do

I wrote a spec which tests exactly this behavior.

establish_active_record_connection takes care of creating AR connection, but when it runs for the first time the models are not yet defined and later in the middle of the suite the connection is dropped which leads to the fact that the db tables are missing and need to be migrated again.

Maybe switching to a sqlite file will be a better option, then the initial migrations stay in place after dropping the connections.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only be done in the specs necessary and not globally.


def clear_active_record_connection
ActiveRecord::Base.connection_handler.clear_all_connections!
ActiveRecord::Base.connection_handler.connection_pool_list.each do |pool|
if Rails.version.to_f >= 5.0
ActiveRecord::Base.connection_handler.remove_connection(pool.spec.name)
else
ActiveRecord::Base.connection_handler.remove_connection(ActiveRecord::Base)
end
end
end

establish_active_record_connection
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be removed, we don't want every other test having a database connection.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, but it's not a subject of the PR. Actually all tests require a database connection due to the *Model classes at ar_classes.rb. When disabling AR support on the specs overall, everything is broken. Disabling the database connection on all tests sounds like a bigger refactoring.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually all tests require a database connection due to the *Model classes at ar_classes.rb. When disabling AR support on the specs overall, everything is broken.

Currently there is no database configured so I'm not sure thats true, if your changes cause things to fail your changes are at fault, applying a global config to the specs is not a valid fix.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently there is no database configured so I'm not sure thats true

https://github.com/rspec/rspec-rails/blob/master/spec/support/ar_classes.rb#L1-L19

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm ok, well make it fail with that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have in_sub_process helpers if you need to change something, but I don't want global configuration changes for just this spec, it shouldn't be necessary.


module Connections
def self.extended(host)
Expand Down