Optional ActiveRecord config #2038

Jack12816 · 2018-10-25T14:09:45Z

- What is it good for

This PR ships a solution for #1876.

- What I did

I added a new option use_active_record to the RSpec config. This option is also added to the generator for new spec/rails_helper.rb files. The config option is read on the FixtureSupport module before the ActiveRecord loading happens. Tests with and without this setting were added in the case a database connection is not established.

- A picture of a cute animal (not mandatory but encouraged)

spec/rspec/rails/matchers/has_spec.rb

lib/rspec/rails/fixture_support.rb

JonRowe · 2018-10-25T19:54:28Z

spec/support/ar_classes.rb

+    next unless Kernel.const_defined? host
+    Connections.extended(Kernel.const_get(host))
+  end
+end


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.

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

spec/support/ar_classes.rb

fables-tales · 2019-01-07T15:19:08Z

@Jack12816 any chance that you take a look at the travis build and get it to a passing state?

Thank you!

mrgordon · 2019-03-25T22:37:18Z

This is a workaround for ActiveStorage bugs. Ideally it should be fixed there instead of adding new code to rspec-rails but I'd support this to save people time if we don't think they'll fix it

JonRowe · 2019-03-26T11:04:12Z

@mrgordon If you'd like to use this as a base and fix it up I'm sure @Jack12816 won't mind, its gone rather stale. Please note the existing feedback still needs taking into account and a build passing.

benoittgt · 2019-05-14T20:47:49Z

@mrgordon you said

This is a workaround for ActiveStorage bugs

Do you have more infos? Maybe it is fixed and we don't need this patch anymore? Especially with rspec-rails 4

mrgordon · 2019-05-14T23:46:57Z

I have no other info. Just seems logical that ActiveStorage should either not require ActiveRecord at all or should automatically require it if necessary so it functions. Instead there is an unstated (?) / unclear dependency that is only noticed if you remove ActiveRecord because things break.

benoittgt · 2019-05-15T04:47:58Z

@mrgordon thanks

@Jack12816 Do you think you can rebase.

Jack12816 · 2019-05-27T06:37:48Z

@benoittgt I've rebased the branch and did some changes to make it work with Rails 3.

spec/rspec/rails/fixture_support_spec.rb

pirj · 2020-01-08T20:51:50Z

@Jack12816 are you up to wrap this up?
Just in case, Rails 3 support has been dropped.

pirj · 2020-02-04T20:54:32Z

@Jack12816 a friendly reminder.

Signed-off-by: Hermann Mayer <hermann.mayer92@gmail.com>

Jack12816 · 2020-02-05T10:56:30Z

@pirj I've rebased it to upstream master and now all tests passing.

JonRowe

I had actually started looking at this myself to pick up the slack and I'm not convinced this solution works as I'm not convinced the specs work. I'd like to see a test that fails without modifying the code, and without modifying the global specs.

JonRowe · 2020-02-05T12:54:15Z

spec/support/ar_classes.rb

+    next unless Kernel.const_defined? host
+    Connections.extended(Kernel.const_get(host))
+  end
+end


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

JonRowe · 2020-02-05T12:54:23Z

spec/support/ar_classes.rb

+  end
+end
+
+establish_active_record_connection


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

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.

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.

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

Hmm ok, well make it fail with that.

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.

JonRowe · 2020-02-05T12:56:51Z

spec/rspec/rails/fixture_support_spec.rb

+
+        after { RSpec.configuration.use_active_record = true }
+
+        include_examples "unrelated example does not raise"


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.

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?

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.

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! :)

JonRowe · 2020-03-13T08:24:45Z

Thanks for your hard work getting this started, closing in favour of #2266, would you be able to check that works for you? Once green I can ship as a beta if you'd like, this is one of the last things we'd like included in 4.0.0 final.

Jack12816 mentioned this pull request Oct 25, 2018

Add a global switch for ActiveRecord #1876

Closed

Jack12816 force-pushed the master branch from 39b003b to e9ca56f Compare October 25, 2018 16:26

JonRowe reviewed Oct 25, 2018

View reviewed changes

Jack12816 force-pushed the master branch 6 times, most recently from 9cde8aa to 85e23f3 Compare May 24, 2019 12:18

benoittgt reviewed May 27, 2019

View reviewed changes

spec/rspec/rails/fixture_support_spec.rb Outdated Show resolved Hide resolved

spec/rspec/rails/fixture_support_spec.rb Outdated Show resolved Hide resolved

Jack12816 force-pushed the master branch 2 times, most recently from 89ba3a0 to f5740fc Compare May 28, 2019 07:40

JonRowe mentioned this pull request Sep 16, 2019

Rspec-Rails assumes I am using ActiveRecord, but I am not #2172

Closed

JonRowe mentioned this pull request Jan 21, 2020

Add configuration to surpress active_record checks #2266

Merged

Jack12816 force-pushed the master branch 3 times, most recently from e0d8a8b to 562940e Compare February 5, 2020 09:35

Optional ActiveRecord config.

6ab934c

Signed-off-by: Hermann Mayer <hermann.mayer92@gmail.com>

Jack12816 force-pushed the master branch from 562940e to 6ab934c Compare February 5, 2020 10:24

JonRowe requested changes Feb 5, 2020

View reviewed changes

JonRowe closed this Mar 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optional ActiveRecord config #2038

Optional ActiveRecord config #2038

Jack12816 commented Oct 25, 2018

JonRowe Oct 25, 2018

Jack12816 Oct 26, 2018 •

edited

Loading

JonRowe Feb 5, 2020

fables-tales commented Jan 7, 2019

mrgordon commented Mar 25, 2019

JonRowe commented Mar 26, 2019

benoittgt commented May 14, 2019

mrgordon commented May 14, 2019

benoittgt commented May 15, 2019

Jack12816 commented May 27, 2019

pirj commented Jan 8, 2020

pirj commented Feb 4, 2020

Jack12816 commented Feb 5, 2020

JonRowe left a comment

JonRowe Feb 5, 2020

JonRowe Feb 5, 2020

Jack12816 Feb 5, 2020

JonRowe Feb 5, 2020

Jack12816 Feb 5, 2020

JonRowe Feb 5, 2020

JonRowe Feb 5, 2020

JonRowe Feb 5, 2020

Jack12816 Feb 5, 2020

JonRowe Feb 5, 2020

JonRowe Feb 5, 2020

JonRowe commented Mar 13, 2020


		after { RSpec.configuration.use_active_record = true }

		include_examples "unrelated example does not raise"

Optional ActiveRecord config #2038

Optional ActiveRecord config #2038

Conversation

Jack12816 commented Oct 25, 2018

Choose a reason for hiding this comment

Jack12816 Oct 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fables-tales commented Jan 7, 2019

mrgordon commented Mar 25, 2019

JonRowe commented Mar 26, 2019

benoittgt commented May 14, 2019

mrgordon commented May 14, 2019

benoittgt commented May 15, 2019

Jack12816 commented May 27, 2019

pirj commented Jan 8, 2020

pirj commented Feb 4, 2020

Jack12816 commented Feb 5, 2020

JonRowe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonRowe commented Mar 13, 2020

Jack12816 Oct 26, 2018 •

edited

Loading