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

[rails] disable eager loading in make_basic_app #2434

Merged
merged 1 commit into from
Oct 17, 2024

Conversation

solnic
Copy link
Collaborator

@solnic solnic commented Oct 17, 2024

This addresses the issue with very slow test suite when running under Ruby 3.1+ and Rails 7.1+

#skip-changelog

Copy link

codecov bot commented Oct 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.97%. Comparing base (76ede87) to head (e061593).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2434   +/-   ##
=======================================
  Coverage   98.97%   98.97%           
=======================================
  Files         231      231           
  Lines       15094    15094           
=======================================
  Hits        14939    14939           
  Misses        155      155           
Components Coverage Δ
sentry-ruby 99.14% <ø> (ø)
sentry-rails 98.46% <100.00%> (ø)
sentry-sidekiq 98.67% <ø> (ø)
sentry-resque 96.85% <ø> (ø)
sentry-delayed_job 98.92% <ø> (ø)
sentry-opentelemetry 99.75% <ø> (ø)
Files with missing lines Coverage Δ
sentry-rails/spec/dummy/test_rails_app/app.rb 100.00% <100.00%> (ø)

@solnic solnic force-pushed the solnic/speed-up-rails-suite branch from 8a5fa5a to e061593 Compare October 17, 2024 09:41
@solnic solnic marked this pull request as ready for review October 17, 2024 09:48
@solnic
Copy link
Collaborator Author

solnic commented Oct 17, 2024

@st0012 hey so eager loading makes the suite super slow under rails 7.1+ - do you know why we have it enabled by default? Specs are passing when it's disabled and the test coverage remains the same.

@solnic solnic merged commit 03293ef into master Oct 17, 2024
141 checks passed
@solnic solnic deleted the solnic/speed-up-rails-suite branch October 17, 2024 10:13
@st0012
Copy link
Collaborator

st0012 commented Oct 26, 2024

@solnic The idea was to mimic the production app as much as possible, which usually have eager loading enabled. But if it's causing issues and aren't really relied on by particular tests, it's fine to turn it off too 👍

@solnic
Copy link
Collaborator Author

solnic commented Oct 28, 2024

@st0012 oh I see! I think it's a good strategy actually. In fact, I'm a fan of using actual test apps that you run tests against. Even with our setup we don't really mimic a real app setup. In some of my other projects I have test rails apps generated under spec/fixtures/app-{rails_ver} with some common setup handled in the spec_helper.rb. I also found it to be easier to update the test apps to latest versions because I can use regular Rails tools to help me with that. We could maybe consider switching to such a strategy at some point.

@st0012
Copy link
Collaborator

st0012 commented Oct 28, 2024

I think we can add a few more expensive "end-to-end" tests that way, but I wouldn't ditch the current test suites just yet.
We can probably maintain one fixture app that targets the latest Rails release and use it to test some more complicated features, like profiling and cron monitoring. But for the majority of features, the current test suite still provides lower maintenance cost and faster feedback imo.

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