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

Ignore file fixtures on db:fixtures:load #42153

Merged
merged 1 commit into from Jun 23, 2021

Conversation

hovsater
Copy link
Contributor

@hovsater hovsater commented May 5, 2021

Summary

In commit, 8ec085b, support for namespaced fixtures where introduced. When file fixtures where introduced their default path was set to test/fixtures/files. This caused any .yml file placed in test/fixtures/files to be treated as a test fixture. This was addressed in #36884. However, we're still unconditionally loading everything within test/fixtures when you run rake db:fixtures:load.

Other Information

The current solution is fairly naive, but I thought it's good enough as a starting point. I can iterate on the solution based on feedback. Some thoughts:

  • Do we want to support an environment variable such as FILE_FIXTURES_DIR given we're already supporting FIXTURES_DIR?
  • The default value for the fixtures directory is fetched from ActiveRecord::Tasks::DatabaseTasks.fixtures_path. Does it make sense to define a file_fixtures_path as well?

@hovsater hovsater force-pushed the ignore-file-fixtures-on-load branch from d68be01 to f54cd5a Compare May 5, 2021 18:23
@@ -382,11 +382,14 @@ db_namespace = namespace :db do
base_dir
end

file_fixtures_dir = File.join(fixtures_dir, "files")
Copy link
Member

Choose a reason for hiding this comment

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

this could be inlined on line 391. it's only needed in one branch of the if/else.

Copy link
Contributor Author

@hovsater hovsater Jun 16, 2021

Choose a reason for hiding this comment

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

@ghiculescu addressed in 4cd5a9b.

@ghiculescu
Copy link
Member

Hey @kevinsjoberg, thanks for working on this. Some of the test failures seem intermittent, but this seems like it could be related:

image

Can you please check / rebase against main?

In terms of the solution, I think copying the approach from #36884 is fine.

@hovsater hovsater force-pushed the ignore-file-fixtures-on-load branch 2 times, most recently from ac0a257 to 4cd5a9b Compare June 16, 2021 19:46
@hovsater
Copy link
Contributor Author

[...] but this seems like it could be related

@ghiculescu you're absolutely right. The glob pattern was wrong. Fixed in c3359d2.

@ghiculescu ghiculescu added the ready PRs ready to merge label Jun 16, 2021
@ghiculescu
Copy link
Member

ghiculescu commented Jun 16, 2021

This looks good, can you please squash your commits and add a changelog entry?

Copy link
Member

@rafaelfranca rafaelfranca left a comment

Choose a reason for hiding this comment

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

Can you squash your commits?

@hovsater hovsater force-pushed the ignore-file-fixtures-on-load branch 2 times, most recently from 66dc5eb to 9959272 Compare June 23, 2021 18:41
@rails-bot rails-bot bot added the actionpack label Jun 23, 2021
@hovsater hovsater force-pushed the ignore-file-fixtures-on-load branch from 9959272 to f0f067a Compare June 23, 2021 18:42
@hovsater
Copy link
Contributor Author

@ghiculescu @rafaelfranca thanks for the reminder. 🙂 I've squashed the commits and added a changelog entry as suggested. See f0f067a.

@rafaelfranca rafaelfranca merged commit f5c092c into rails:main Jun 23, 2021
rafaelfranca added a commit that referenced this pull request Jun 23, 2021
@hovsater hovsater deleted the ignore-file-fixtures-on-load branch June 23, 2021 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants