Fix slow translatable concern test. #68942
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes a slow test by removing unnecessary I18n backend reloading that was causing significant performance degradation during unit test runs. The solution replaces the expensive backend reloading with a memoization pattern to ensure translations are loaded only once.
- Removed I18n backend reload operation which was the suspected performance bottleneck
- Implemented memoization using class variable to load translations only once
- Cleaned up unnecessary comments while preserving essential setup
Comments suppressed due to low confidence (1)
dashboard/test/models/concerns/localizable_test.rb:1
- The memoization pattern may cause test isolation issues. If tests modify I18n state or if parallel test execution occurs, the shared class variable could lead to unpredictable test behavior. Consider using a test-specific setup method or ensuring proper cleanup between test runs.
# frozen_string_literal: true
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| # Setup translations once when the test class loads | ||
| I18n.backend.store_translations( | ||
| :es, { | ||
| data: { | ||
| described_models: { | ||
| described_key: { | ||
| display_name: 'Nombre de Prueba', | ||
| description: 'Descripción de Prueba' | ||
| }, | ||
| another_key: { | ||
| display_name: 'Otro Nombre', | ||
| description: 'Otra Descripción' | ||
| } | ||
| } | ||
| } | ||
| } | ||
| ) |
There was a problem hiding this comment.
I think we want to perform this setup logic in a setup_all block rather than at require time. We might not care about rolling back transactions in this case, but we do still want to benefit from stacktrace capturing and adhere to consistent patterns within our unit tests
There was a problem hiding this comment.
I agree, however, setup_all performs quite poorly with rspec -- the setup_all block appears to be run once per rspec directive (context, before, it, etc). I've been meaning to add a lint rule to prevent use of setup_all and rspec in the same test. feel free to confirm whether this is a performance problem or not in this case. assuming it is, can this setup code be run once using rspec syntax? I think that would be before(:all) based on the docs, but I could be mistaken.
There was a problem hiding this comment.
some notes on the setup_all + rspec problem here: #66251
There was a problem hiding this comment.
I'm pretty sure it was the reload! call causing the slowdown, not the store_translations call; testing locally, I can't identify any difference in performance between this implementation and one with setup_all
But either way, the standard rspec solution for this pattern sounds like a fine alternative. I don't love that we're mixing two different standards in our test suite, but two's better than none 🙃
There was a problem hiding this comment.
I moved it into the before block, which uses setup under the hood.
We still use Minitest, but Specs syntax, and some additional syntax sugar that Artem added.
There was a problem hiding this comment.
We could also consider the minitest-hooks gem.
|
Thanks for tackling this Mario! Once the setup issue is resolved, can you please hold off on merging this for a couple of days? I've just updated the drone infrastructure to beefier EC2 instance types, and I'd like to get 1-2 days of performance data after that change before another big change to drone performance like this goes in. |
3 participants
This test was slowing down the Unit test runs significantly.
Reloading the i18n backend is the suspected culprit of the slowdown, as this call can be extremely slow, especially if it's scanning and reloading all translation files from disk.
The solution is to remove the I18n backend reload as it is unnecessary. Instead, we load translations only once when the class is loaded.
PR Creation Checklist: