chore: replace hardcoded strings with i18n message references in tests#39859
Open
DDDDDanica wants to merge 40 commits intomainfrom
Open
chore: replace hardcoded strings with i18n message references in tests#39859DDDDDanica wants to merge 40 commits intomainfrom
DDDDDanica wants to merge 40 commits intomainfrom
Conversation
Contributor
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ui/components/app/modals/hold-to-reveal-modal/hold-to-reveal-modal.test.js
Show resolved
Hide resolved
DDDDDanica
force-pushed
the
chore/6395-i18n
branch
from
7f5a5c8 to
1499bf2
Compare
DDDDDanica
force-pushed
the
chore/6395-i18n
branch
2 times, most recently
from
3615649 to
3ec08ca
Compare
Contributor
✨ Files requiring CODEOWNER review ✨💎 @MetaMask/metamask-assets (5 files, +41 -19)
🧪 @MetaMask/qa (4 files, +18 -20)
|
Co-authored-by: Cursor <cursoragent@cursor.com>
DDDDDanica
force-pushed
the
chore/6395-i18n
branch
from
3ec08ca to
f0b46c9
Compare
DDDDDanica
had a problem deploying
to
pr-comment
GitHub Actions
Failure
DDDDDanica
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
DDDDDanica
dismissed stale reviews from micaelae, lwin-kyaw, seaona, hjetpoluru, and pedronfigueiredo
via
41bac7b
DDDDDanica
requested a deployment
to
pr-comment
GitHub Actions
In progress
DDDDDanica
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
| // Report ALL violations for this file so the developer can identify | ||
| // which ones are new (we can't know positionally which are "old"). | ||
| newViolations.push(...violations); | ||
| } |
There was a problem hiding this comment.
Locale fitness test misses non-UI tests
Medium Severity
The Rule 2 fitness function claims to scan all test files, but it only searches under ui/ (uiDir) when collecting testFiles. This can let new hardcoded-locale query strings slip into non-UI tests without being caught, weakening the intended quality gate.
Contributor
Author
There was a problem hiding this comment.
Tests outside ui/ almost never use getByText with locale-matching strings. So this comment is okay to ignore
Gudahtt
reviewed
Feb 24, 2026
test/lib/i18n-helpers.ts
Outdated
| import { getMessage } from '../../ui/helpers/utils/i18n-helper'; | ||
| import en from '../../app/_locales/en/messages.json'; | ||
|
|
||
| const enMessages = en as unknown as I18NMessageDict; |
Member
There was a problem hiding this comment.
It doesn't look like a type assertion is needed here, especially not one from unknown
Contributor
Author
There was a problem hiding this comment.
You're right, dropped the double assertion entirely here: 147df35
Gudahtt
reviewed
Feb 24, 2026
test/lib/i18n-helpers.ts
Outdated
| export const enLocale = enMessages; | ||
|
|
||
| export function tEn(key: string, substitutions: string[] = []): string { | ||
| return getMessage('en', enMessages, key, substitutions) as string; |
Member
There was a problem hiding this comment.
This type assertion doesn't seem to be correct. getMessage can return a React component or null according to the type definition. We know this will never return a React component in this case (because we never pass in one as a substitution), but null is a real possibility.
We should either check for null here, or update the return type of tEn to include null. We can also get rid of the type violation about the React component return type by using the underlying shared getMessage function instead (from shared/modules/i18n.ts)
Contributor
Author
There was a problem hiding this comment.
Good catch! switched to the shared getMessage from shared/modules/i18n.ts and updated the return type to string | null. No more type assertion now. Addressed in 147df35
DDDDDanica
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
Contributor
Builds ready [41bac7b]
⚡ Performance Benchmarks (1466 ± 151 ms)
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
Gudahtt
reviewed
Feb 24, 2026
|
|
||
| const enLocale: Record<string, { message: string }> = | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/no-var-requires | ||
| require('../../app/_locales/en/messages.json'); |
Member
There was a problem hiding this comment.
Why use require here? We should be able to import it
Member
There was a problem hiding this comment.
It seemed to work when I tried it locally. Initially it caused a type error because the type was too exact, and it didn't know how to handle keys of type string. But this was easily fixed by loosening the type so that it treats all keys the same, e.g.
import enLocaleLiteral from '../../app/_locales/en/messages.json';
// Loosen locale type to simplify access via `string` key
const enLocale: Record<string, { message: string }> = enLocaleLiteral;
Contributor
Builds ready [147df35]
⚡ Performance Benchmarks (1377 ± 94 ms)
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
DDDDDanica
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
| const HARDCODED_QUERY_OPTIONS_PATTERN_R2 = new RegExp( | ||
| `(?:${QUERY_FN_GROUP})\\s*\\([^)]*\\{\\s*name:\\s*(?:'([^']{3,})'|"([^"]{3,})")`, | ||
| 'gu', | ||
| ); |
There was a problem hiding this comment.
Fitness function Rule 2 regex misses template literal queries
Low Severity
The Rule 2 HARDCODED_QUERY_DIRECT_PATTERN_R2 and HARDCODED_QUERY_OPTIONS_PATTERN_R2 regexes only match single-quoted or double-quoted strings. They miss template literal strings (backtick-delimited), such as getByText(`${messages.tokenAddress.message}:`), which are used in this very PR (e.g., detected-token-address.test.js). More importantly, they also miss backtick-based hardcoded strings like getByText(`Token address:`) that a developer might add in the future. This means the fitness function won't catch all hardcoded query strings that match locale values.
DDDDDanica
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
|
DDDDDanica
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
Contributor
Builds ready [b1b7da9]
⚡ Performance Benchmarks (1362 ± 101 ms)
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
DDDDDanica
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
| `(?:${QUERY_FN_GROUP})\\s*\\([^)]*\\{\\s*name:\\s*(?:'([^']{3,})'` + | ||
| '|`([^`$]{3,})`)', | ||
| 'gu', | ||
| ); |
There was a problem hiding this comment.
Fitness function regex misses double-quoted hardcoded strings
Low Severity
HARDCODED_QUERY_DIRECT_PATTERN_R2 and HARDCODED_QUERY_OPTIONS_PATTERN_R2 match single-quoted and backtick strings but omit double-quoted strings entirely. While Prettier currently enforces singleQuote: true, any test file that bypasses formatting or uses a different config would silently pass the fitness function despite having hardcoded locale-matching strings in double quotes. Adding a "([^"]{3,})" branch would close the gap.
Additional Locations (1)
DDDDDanica
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
Contributor
Builds ready [1ae22cc]
⚡ Performance Benchmarks (1332 ± 97 ms)
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
13 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
1. i18n Test Robustness
Test files were using hardcoded plaintext strings in assertions (
getByText('Cancel'),getByRole('button', { name: 'Confirm' }), etc.) instead of referencing the actual i18n locale messages. This makes tests fragile and harder to maintain when translations change.2. Unused fireEvent Import Cleanup
Some test files import
{ fireEvent }from../../../../../test/jestbut can be simplified to import directly from@testing-library/react.What is the improvement/solution?
i18n Changes (214 files):
enLocaleexport totest/lib/i18n-helpers.jsfor easy importingapp/_locales/en/messages.jsonimports withimport { enLocale as messages } from 'test/lib/i18n-helpers'eslint-disable-next-line import/no-restricted-pathscomments for locale importsmessages.<key>.messagepattern.replace('$1', value)syntaxfireEvent Cleanup:
import { fireEvent } from '../../../../../test/jest'toimport { fireEvent } from '@testing-library/react'test/jest/index.jsChangelog
CHANGELOG entry: null
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Low Risk
Test-only changes that mainly adjust assertions/imports and add a new test gate; low runtime risk but may cause CI failures if the new fitness rules are too strict or the baseline misses existing cases.
Overview
Migrates many E2E/integration/unit tests away from hardcoded English strings and
as stringcasts, instead asserting against shared i18n message sources (via newtest/lib/i18n-helpers.tsexports likeenLocaleand strictertEn).Adds a new global “fitness function” test (
locale-query-mismatch.test.ts) that scans UI test files to prevent (1) querying locale messages when the UI is hardcoded, and (2) hardcoding query strings that match locale values beyond an explicit per-file baseline.Cleans up test utilities by removing unused re-exports (
test/jest/index.js) and switching some tests to importfireEventdirectly from@testing-library/react; also updatesRewardsErrorBannerto use i18n (t('dismiss')/t('confirm')) for button labels and adjusts its tests accordingly.Written by Cursor Bugbot for commit 1ae22cc. This will update automatically on new commits. Configure here.