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

Run a load hook when TestFixtures is included #47690

Conversation

andrewn617
Copy link
Contributor

@andrewn617 andrewn617 commented Mar 16, 2023

Motivation / Background

This Pull Request has been created as a follow up to #47675. In the example I gave in that PR, I showed that you can define an active_model_test_case load hook and shovel onto the fixture_paths. However, I now realize this won't actually work, because TestFixtures itself is included in ActiveSupport::TestCase from a load hook. Since there is no way to control the order the hooks are run in, the fixture_paths method may be undefined at that the time the hook is run.

Detail

This Pull Request adds an :active_record_fixtures load hook. This load hook is run every time TestFixtures is included. So the fixture paths can be added to from this hook. Eg:

module UserManagement
  class Engine < Rails::Engine

    initializer("user_management.fixture_path") do
      ActiveSupport.on_load(:active_record_fixtures) do
        self.fixture_paths << "#{Rails.root}/user_management/test/fixtures}"
      end
    end
  end
end

Additional information

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@andrewn617 andrewn617 force-pushed the add-on-load-callback-for-active-record-fixtures branch from 8f7356e to 6f8e088 Compare March 16, 2023 13:34
@@ -25,6 +25,8 @@ def after_teardown # :nodoc:
class_attribute :pre_loaded_fixtures, default: false
class_attribute :lock_threads, default: true
class_attribute :fixture_sets, default: {}

ActiveSupport.run_load_hooks(:active_record_fixtures, self)
Copy link
Member

Choose a reason for hiding this comment

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

We need to document this new hook in engines.md

### Available Load Hooks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it. Though.. the column header is "Class" and TestFixtures is is a mixin.. does it warrant it's own section or explanation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried something more descriptive, let me know what you think.

@andrewn617 andrewn617 force-pushed the add-on-load-callback-for-active-record-fixtures branch from 6f8e088 to f0da985 Compare March 16, 2023 21:15
@rails-bot rails-bot bot added the docs label Mar 16, 2023
@andrewn617 andrewn617 force-pushed the add-on-load-callback-for-active-record-fixtures branch 3 times, most recently from b6bfbb0 to 982cd23 Compare March 16, 2023 22:34
@skipkayhil
Copy link
Member

I'm a little late since #47675 was merged already, but would it make sense to utilize Engine#paths for this? It seems like this is a very similar pattern to how other paths get added from engines:

initializer :add_view_paths do
views = paths["app/views"].existent
unless views.empty?
ActiveSupport.on_load(:action_controller) { prepend_view_path(views) if respond_to?(:prepend_view_path) }
ActiveSupport.on_load(:action_mailer) { prepend_view_path(views) }
end
end
initializer :add_mailer_preview_paths do
previews = paths["test/mailers/previews"].existent
unless previews.empty?
ActiveSupport.on_load(:action_mailer) { self.preview_paths |= previews }
end

@andrewn617
Copy link
Contributor Author

andrewn617 commented Mar 17, 2023

@skipkayhil Thanks for pointing this out, I wasn't familiar with this. I don't think we need all the functionality provided by ActionView::PathSet - at least for now. But as a forward looking thing, it might be helpful to get rid of fixture_paths= in and implement prepend_fixture_paths, even if the underlying object is just an array for now. Thoughts @rafaelfranca?

Edit: hmm on second thought, I think we need fixture_paths= no matter what, since apps should have the ability to replace the fixture path whole cloth (as they already can do).

@rafaelfranca
Copy link
Member

We will probably end up there with the feature I'm working on, but right now I don't think we need to use engine paths just yet.

@@ -1462,7 +1462,11 @@ end

### Available Load Hooks

These are the load hooks you can use in your own code. To hook into the initialization process of one of the following classes use the available hook.
These are the load hooks you can use in your own code.
Copy link
Member

Choose a reason for hiding this comment

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

Let's just merge the two and change classes to constants. i18n isn't a class and we put in the same table.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@andrewn617 andrewn617 force-pushed the add-on-load-callback-for-active-record-fixtures branch from 982cd23 to 49283ad Compare March 17, 2023 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants