Skip to content

test: stabilize program tests locale#3701

Open
cyphercodes wants to merge 2 commits into
mozilla:masterfrom
cyphercodes:fix-locale-program-tests
Open

test: stabilize program tests locale#3701
cyphercodes wants to merge 2 commits into
mozilla:masterfrom
cyphercodes:fix-locale-program-tests

Conversation

@cyphercodes
Copy link
Copy Markdown

Fixes #3676

Summary

  • Force a stable English locale for program.Program unit tests that assert on yargs validation messages.
  • Restore the caller's locale environment afterward so other tests and product behavior are unchanged.

Verification

  • npm ci
  • npm run build
  • LANG=de_DE.UTF-8 ./node_modules/.bin/mocha tests/unit/test.setup.js tests/unit/test.program.js --grep 'reports unknown commands|throws an error about unknown commands'

Copy link
Copy Markdown
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

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

Could we keep it simple and only set the LC_ALL environment variable? If set all others are not used. Yargs's relevant source is https://github.com/yargs/yargs/blob/437f3a4e0f4166e1f1a3ce023b0331159582746d/lib/yargs-factory.ts#L1593

Copy link
Copy Markdown
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

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

See comment above.

@cyphercodes
Copy link
Copy Markdown
Author

Updated this PR to address the review feedback from @Rob--W:

  • simplified the locale stabilization to set/restore only LC_ALL, since yargs ignores the other locale variables when LC_ALL is set.

Local verification:

  • npm ci
  • npm run build
  • npx eslint tests/unit/test.program.js
  • NODE_ENV=test npx mocha ./tests/unit/test.setup.js ./tests/unit/test.program.js
  • npx prettier --check tests/unit/test.program.js
  • git diff --check

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.

npm test with locale de_DE.UTF-8 fails 2 tests

2 participants