-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,3 +22,4 @@ bundle | |
specs.out | ||
spec/examples.txt | ||
specs.out | ||
test.db |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately, I don't get your point. When There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Maybe switching to a sqlite file will be a better option, then the initial migrations stay in place after dropping the connections. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, if your changes cause things to fail your changes are at fault, applying a global config to the specs is not a valid fix. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
https://github.com/rspec/rspec-rails/blob/master/spec/support/ar_classes.rb#L1-L19 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm ok, well make it fail with that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have |
||
|
||
module Connections | ||
def self.extended(host) | ||
|
Uh oh!
There was an error while loading. Please reload this page.