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

Allow fixtures YAML files to set the model class in the file itself #20574

Merged
merged 1 commit into from Sep 30, 2015

Conversation

Projects
None yet
4 participants
@repinel
Member

repinel commented Jun 16, 2015

Currently, set_fixture_class is only available using the TestFixtures concern and it is ignored for rake db:fixtures:load. Using the correct model class, it is possible for the fixture load to also load the associations from the YAML files (e.g., :belongs_to and :has_many).

If the PR goes forward, should we consider deprecating set_fixture_class?

Fixes #9516.

@repinel

This comment has been minimized.

Show comment
Hide comment
@repinel

repinel Jun 16, 2015

Member

@senny This would be the idea that we were discussing on #9516.

/cc @jeremy

Member

repinel commented Jun 16, 2015

@senny This would be the idea that we were discussing on #9516.

/cc @jeremy

@repinel

This comment has been minimized.

Show comment
Hide comment
@repinel

repinel Jul 10, 2015

Member

@senny @jeremy Any thoughts on this?

Member

repinel commented Jul 10, 2015

@senny @jeremy Any thoughts on this?

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Aug 13, 2015

Member

Using ERB within yaml to set metadata feels surprising.

How about using a special key to set fixture metadata?

_fixture:
  model_class: User
david:
  name: David
Member

jeremy commented Aug 13, 2015

Using ERB within yaml to set metadata feels surprising.

How about using a special key to set fixture metadata?

_fixture:
  model_class: User
david:
  name: David
@repinel

This comment has been minimized.

Show comment
Hide comment
@repinel

repinel Aug 13, 2015

Member

@jeremy That is close to my first thought. IMO, it seems good.

Should I update the RP?

Member

repinel commented Aug 13, 2015

@jeremy That is close to my first thought. IMO, it seems good.

Should I update the RP?

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Aug 13, 2015

Member

@repinel yes, please do.

Member

senny commented Aug 13, 2015

@repinel yes, please do.

@repinel

This comment has been minimized.

Show comment
Hide comment
@repinel

repinel Aug 13, 2015

Member

Closing and reopening to run tests.

Member

repinel commented Aug 13, 2015

Closing and reopening to run tests.

@repinel repinel closed this Aug 13, 2015

@repinel repinel reopened this Aug 13, 2015

Show outdated Hide outdated activerecord/test/schema/schema.rb
@@ -370,6 +374,11 @@ def except(adapter_names_to_exclude)
t.integer :ideal_reference_id
end
create_table :joke_comments, force: true do |t|

This comment has been minimized.

@senny

senny Aug 14, 2015

Member

is it necessary to create new tables to test this feature? Can't we reuse some tables that already exist? We try to not add more global models for single test scenarios like this.

@senny

senny Aug 14, 2015

Member

is it necessary to create new tables to test this feature? Can't we reuse some tables that already exist? We try to not add more global models for single test scenarios like this.

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Aug 14, 2015

Member

I think set_fixture_class should have priority over _fixture:model_class. This way you could configure the class inside the .yml file and still overwrite that inside a test case.

Member

senny commented Aug 14, 2015

I think set_fixture_class should have priority over _fixture:model_class. This way you could configure the class inside the .yml file and still overwrite that inside a test case.

Show outdated Hide outdated activerecord/lib/active_record/fixtures.rb
# you cannot use +set_fixture_class+, e.g., when running
# <tt>rake db:fixtures:load</tt>.
#
# To load the fixtures file `accounts.yml` as the `User` model, use:

This comment has been minimized.

@senny

senny Aug 14, 2015

Member

let's give the full path here: test/fixtures/accounts.yml.

@senny

senny Aug 14, 2015

Member

let's give the full path here: test/fixtures/accounts.yml.

Show outdated Hide outdated activerecord/lib/active_record/fixtures.rb
fixture_settings = @fixtures.delete('_fixture')
if fixture_settings && (custom_model_class = fixture_settings['model_class'])
self.model_class = custom_model_class
@fixtures.values.each { |fixture| fixture.model_class = self.model_class }

This comment has been minimized.

@senny

senny Aug 14, 2015

Member

I don't like that we create ActiveRecord::Fixture objects with the "wrong" model_class and then re-assign here. Can we find a way that moves knowledge about _fixture:model_class into Fixture::File so that we can create Fixture objects with the right model_class from the get go?

@senny

senny Aug 14, 2015

Member

I don't like that we create ActiveRecord::Fixture objects with the "wrong" model_class and then re-assign here. Can we find a way that moves knowledge about _fixture:model_class into Fixture::File so that we can create Fixture objects with the right model_class from the get go?

@repinel

This comment has been minimized.

Show comment
Hide comment
@repinel

repinel Aug 17, 2015

Member

@senny I updated the code. Please review, thanks!

Member

repinel commented Aug 17, 2015

@senny I updated the code. Please review, thanks!

Show outdated Hide outdated activerecord/lib/active_record/fixtures.rb
@@ -790,7 +814,7 @@ class FixtureError < StandardError #:nodoc:
class FormatError < FixtureError #:nodoc:
end
attr_reader :model_class, :fixture
attr_reader :fixture, :model_class

This comment has been minimized.

@senny

senny Aug 17, 2015

Member

why this change?

@senny

senny Aug 17, 2015

Member

why this change?

This comment has been minimized.

@repinel

repinel Aug 17, 2015

Member

I missed this one. I'll revert the line.

@repinel

repinel Aug 17, 2015

Member

I missed this one. I'll revert the line.

@@ -761,13 +773,25 @@ def column_names
@column_names ||= @connection.columns(@table_name).collect(&:name)
end
def read_fixture_files(path, model_class)
def model_class=(class_name)

This comment has been minimized.

@senny

senny Aug 17, 2015

Member

Do we still need this extraction? It looks like we could revert that part back to how it's working on master?

@senny

senny Aug 17, 2015

Member

Do we still need this extraction? It looks like we could revert that part back to how it's working on master?

This comment has been minimized.

@repinel

repinel Aug 17, 2015

Member

I was avoiding setting model_class in read_fixture_files directly to the instance variable and having to safe_constantize there. It's easy to revert if preferable.

@repinel

repinel Aug 17, 2015

Member

I was avoiding setting model_class in read_fixture_files directly to the instance variable and having to safe_constantize there. It's easy to revert if preferable.

Show outdated Hide outdated activerecord/lib/active_record/fixture_set/file.rb
def model_class
return @model_class if @model_class
rows.delete_if do |fixture_name, row|

This comment has been minimized.

@senny

senny Sep 7, 2015

Member

This implementation is based on the fact that model_class will be called before each, otherwise you'd get a fixture for _fixture. We should probably change it to behave consistently regardless of call order. Meaning that we have to treat the _fixture key special that it won't ever be part of rows.

This could look something like this (pseudocode):

def model_class
  raw_content["_fixture"]
  # ...
end

def rows
  raw_content.except "_fixture"
end

def raw_content
  # read yaml-file and cache contents
end
@senny

senny Sep 7, 2015

Member

This implementation is based on the fact that model_class will be called before each, otherwise you'd get a fixture for _fixture. We should probably change it to behave consistently regardless of call order. Meaning that we have to treat the _fixture key special that it won't ever be part of rows.

This could look something like this (pseudocode):

def model_class
  raw_content["_fixture"]
  # ...
end

def rows
  raw_content.except "_fixture"
end

def raw_content
  # read yaml-file and cache contents
end
Allow fixtures YAML files to set the model class in the file itself
Currently, `set_fixture_class` is only available using the
`TestFixtures` concern and it is ignored for `rake db:fixtures:load`.
Using the correct model class, it is possible for the fixture load
to also load the associations from the YAML files (e.g., `:belongs_to`
and `:has_many`).
@repinel

This comment has been minimized.

Show comment
Hide comment
@repinel

repinel Sep 11, 2015

Member

@senny I re-implemented the AR::FixtureSet::File#model_class method as suggested 😃

Member

repinel commented Sep 11, 2015

@senny I re-implemented the AR::FixtureSet::File#model_class method as suggested 😃

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca
Member

rafaelfranca commented Sep 12, 2015

r? @senny

@senny senny merged commit 2acec46 into rails:master Sep 30, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

senny added a commit that referenced this pull request Sep 30, 2015

Merge pull request #20574 from repinel/fix-db-fixtures-load
Allow fixtures YAML files to set the model class in the file itself

Conflicts:
	activerecord/CHANGELOG.md
@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Sep 30, 2015

Member

🎉

@repinel thanks for your work and patience 💛. Finally found the time to finish this up. I made some minor changes directly in the merge commit (591a0bb)

Member

senny commented Sep 30, 2015

🎉

@repinel thanks for your work and patience 💛. Finally found the time to finish this up. I made some minor changes directly in the merge commit (591a0bb)

@repinel

This comment has been minimized.

Show comment
Hide comment
@repinel

repinel Sep 30, 2015

Member

@senny Looks awesome! Thanks for reviewing it ❤️

Member

repinel commented Sep 30, 2015

@senny Looks awesome! Thanks for reviewing it ❤️

@repinel repinel deleted the repinel:fix-db-fixtures-load branch Sep 30, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment