Skip to content
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

Loading fixtures without a 'fixture_path' globs `/` for yml #19414

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
4 participants
@eins78
Copy link

commented Mar 20, 2015

  1. ActiveSupport::TestCase.fixture_path is not guaranteed to be defined (When using a non-default test-suite, in our case rspec)
  2. active_record/fixtures.rb#fixtures does expect it to always be set; it builds a path to glob like this "#{fixture_path}/{**,*}/*.{yml}".
  3. So, when loading fixtures :all, without a set fixture_path, the filesystem root is scanned for yml files (/{**,*}/*.yml).

https://github.com/rails/rails/blob/master/activerecord/lib/active_record/fixtures.rb#L880-L881

A fix would be easy, but I am not sure how to handle this condition. I think not having set the variable in the first place is an error, so it would be simplest to check for it and raise an error when it's missing?

@carlosantoniodasilva

This comment has been minimized.

Copy link
Member

commented Mar 20, 2015

In theory if you're using rspec-rails it should be setting fixture_path properly, but if you're not, then I agree it might be unexpected. I think it makes sense to raise in case fixture_path is not set when we ask for it.

@eins78

This comment has been minimized.

Copy link
Author

commented Mar 20, 2015

@carlosantoniodasilva You are right that it should be set in theory, the code you've linked shows that the error is actually in rspec, where the (safe) default value from Rails is override to whatever is configured (or not!) in Rspec (which has no default for it…).

Since this can still happen with other test suites I still attached a simple patch which checks for the variable and raises an error.

@eins78 eins78 changed the title Loading fixtures with a 'fixture_path' globs `/` for yml Loading fixtures without a 'fixture_path' globs `/` for yml Mar 20, 2015

@@ -878,6 +878,9 @@ def set_fixture_class(class_names = {})

def fixtures(*fixture_set_names)
if fixture_set_names.first == :all
unless fixture_path # protect against globbing `/`
raise StandardError, "No 'fixture_path' configured'"

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Mar 24, 2015

Member

Perhaps we should use ArgumentError, wdyt?

@carlosantoniodasilva

This comment has been minimized.

Copy link
Member

commented Mar 24, 2015

Thanks @eins78. Do you think you can provide a test case so that we don't end up removing this in the future?

@eins78

This comment has been minimized.

Copy link
Author

commented Mar 24, 2015

@carlosantoniodasilva I am looking into the existing tests right now to get a sense of where it would fit in. Regarding the raised error class, I just greped through the codebase to get a feeling for the conventions. I have no strong opinion one way or the other, but strictly speaking it is not the argument that is invalid, it's the config.

eins78 added a commit to eins78/rails that referenced this pull request Apr 8, 2015

@eins78 eins78 force-pushed the eins78:master branch from abef2ff to 07d900c Apr 8, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.