Skip to content

Restore faker for development environments#11786

Closed
eileen-nava wants to merge 4 commits intomainfrom
em-sc/restore-faker
Closed

Restore faker for development environments#11786
eileen-nava wants to merge 4 commits intomainfrom
em-sc/restore-faker

Conversation

@eileen-nava
Copy link
Contributor

🛠 Summary of changes

Shane and I worked on this during our 1:1. The user mailer was broken because faker was missing. I included a screenshot of the error. We restored faker and the user mailer looks a-okay now.

📜 Testing Plan

👀 Screenshots

Error:

UserMailerBrokeDueToFaker

@eileen-nava eileen-nava requested review from a team, gina-yamada and mitchellhenke January 22, 2025 21:19
@eileen-nava eileen-nava changed the title Em sc/restore faker Restore faker for development environments Jan 22, 2025
@eileen-nava
Copy link
Contributor Author

Gemfile Outdated
gem 'bullet', '~> 7.0'
gem 'capybara-webmock', git: 'https://github.com/hashrocket/capybara-webmock.git', ref: 'd3f3b7c'
gem 'erb_lint', '~> 0.7.0', require: false
gem 'faker'
Copy link
Contributor

@zachmargolis zachmargolis Jan 22, 2025

Choose a reason for hiding this comment

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

let's add an explicit require in the mailer preview too? or maybe a comment here noting why it's here?

Suggested change
gem 'faker'
gem 'faker', require: false # used in mailer previews

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the motivation making the bundle smaller, so that performance is improved? Or ensuring that faker doesn't mistakenly get removed from development in the future?
I'm not against the change, just want to be sure I understand the benefit.

Copy link
Contributor

Choose a reason for hiding this comment

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

the comment is to make sure it doesn't get removed in the future

the require: false is because we don't strictly need that in memory for our "normal" production flow so this just helps keep things small and explicit

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I'm misunderstanding something, moving from :test to the :development, :test group shouldn't impact production?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the idea is to avoid auto-requiring the gem via Bundler when running the application locally, which wouldn't have happened previously but will happen if it's moved into the development group.

Bundler.require(*Rails.groups)

$ rails c
[1] pry(main)> Rails.groups
=> [:default, "development"]

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that aspect makes sense. I wasn't sure I understood where the production aspect fit in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, I'm not sure the juice is worth the squeeze on this change. After adding require: false, I realized that it caused NameError: uninitialized constant Faker errors when running specs that uses faker, e.g. bundle exec rspec spec/forms/idv/address_form_spec.rb.

I tried to add require 'faker' to the rails_helper below line 16, but that didn't resolve the error. Then I tried adding require 'faker' to the spec_helper, but that also didn't resolve the error.

Is there a way to get faker required everywhere it's used in the codebase specs without having to add require 'faker' to the top of each individual file?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am surprised that spec_helper wasn't enough, let me check out this branch and noodle on it. But for the sake of expediency and fixing the build, I wouldn't hold up this PR on require: false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @zachmargolis! Yeah, I was also really surprised that including it in spec_helper didn't help.... 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried another approach entirely in #11791, which is just to remove usage of Faker in the mailer preview

@eileen-nava
Copy link
Contributor Author

I'm closing this PR in favor of #11791.

@eileen-nava eileen-nava deleted the em-sc/restore-faker branch March 5, 2025 16:29
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.

5 participants