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

Only reload if cache_classes is disabled, reload_classes_only_on_change enabled #418

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stanhu
Copy link

@stanhu stanhu commented Aug 3, 2023

https://guides.rubyonrails.org/configuring.html#config-reload-classes-only-on-change states:

3.2.30 config.reload_classes_only_on_change

Enables or disables reloading of classes only when tracked files change. By default tracks everything on autoload paths and is set to true. If config.cache_classes is true, this option is ignored.

It was surprising to us that even though cache_classes were set to true that factory_bot_rails still instantiated file watchers.

…ge enabled

https://guides.rubyonrails.org/configuring.html#config-reload-classes-only-on-change
states:

>>>
3.2.30 config.reload_classes_only_on_change

Enables or disables reloading of classes only when tracked files
change. By default tracks everything on autoload paths and is set to
true. If config.cache_classes is true, this option is ignored.
>>>

It was surprising to us that even though `cache_classes` were set to
true that factory_bot_rails still instantiated file watchers.
Copy link
Contributor

@mathieujobin mathieujobin left a comment

Choose a reason for hiding this comment

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

interesting, I wonder if this isn't the fix I need for failing to load factories with Rails 7.1

@mike-burns
Copy link
Contributor

The test suite passes without this patch on Rails 7.1.0-beta2. If this does fix an issue you're seeing on Rails 7.1, @mathieujobin , I'm interested in hearing more about that so we can write a test to catch it.

@stanhu this patch needs a test. I can try to write one later, but maybe you can help write one first.

Here's my sketch but I think it needs more finesse:

    context "when cache_classes is true and reload_classes_only_on_change is true" do
      it "does not reload factory definitions" do
        Rails.application.config.cache_classes = true
        Rails.application.config.reload_classes_only_on_change = true
        allow(FactoryBot).to receive(:reload)

        reload_rails!

        expect(FactoryBot).not_to have_received(:reload)
      end
    end

    context "when cache_classes is true and reload_classes_only_on_change is false" do
      it "does not reload factory definitions" do
        Rails.application.config.cache_classes = true
        Rails.application.config.reload_classes_only_on_change = false
        allow(FactoryBot).to receive(:reload)

        reload_rails!

        expect(FactoryBot).not_to have_received(:reload)
      end
    end

    context "when cache_classes is false and reload_classes_only_on_change is true" do
      it "reloads the factory definitions" do
        Rails.application.config.cache_classes = false
        Rails.application.config.reload_classes_only_on_change = true
        allow(FactoryBot).to receive(:reload)

        reload_rails!

        expect(FactoryBot).to have_received(:reload).at_least(1).times
      end
    end

    context "when cache_classes is false and reload_classes_only_on_change is false" do
      it "does not reload factory definitions" do
        Rails.application.config.cache_classes = false
        Rails.application.config.reload_classes_only_on_change = false
        allow(FactoryBot).to receive(:reload)

        reload_rails!

        expect(FactoryBot).not_to have_received(:reload)
      end
    end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants