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

@kevinsjoberg
Copy link
Contributor

@kevinsjoberg kevinsjoberg 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?
@kevinsjoberg kevinsjoberg force-pushed the ignore-file-fixtures-on-load branch from d68be01 to f54cd5a May 5, 2021
@@ -382,11 +382,14 @@ db_namespace = namespace :db do
base_dir
end

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

@ghiculescu ghiculescu Jun 2, 2021

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

Copy link
Contributor Author

@kevinsjoberg kevinsjoberg Jun 16, 2021

@ghiculescu addressed in 4cd5a9b.

@ghiculescu
Copy link
Member

@ghiculescu ghiculescu commented Jun 2, 2021

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.

@kevinsjoberg kevinsjoberg force-pushed the ignore-file-fixtures-on-load branch 2 times, most recently from ac0a257 to 4cd5a9b Jun 16, 2021
@kevinsjoberg
Copy link
Contributor Author

@kevinsjoberg kevinsjoberg commented Jun 16, 2021

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

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

@ghiculescu
Copy link
Member

@ghiculescu 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

Can you squash your commits?

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

@kevinsjoberg kevinsjoberg commented Jun 23, 2021

@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
4 checks passed
rafaelfranca added a commit that referenced this issue Jun 23, 2021
@kevinsjoberg kevinsjoberg deleted the ignore-file-fixtures-on-load branch Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants