test(oxfmt): Update oxfmt --init|migrate tests#16865
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
oxfmt --init|migrate tests
There was a problem hiding this comment.
Tests now leak temp directories on setup failures because several writeFile/mkdir calls occur outside try/finally blocks. Some CLI assertions were weakened (checking only exitCode without validating stderr), reducing confidence in user-facing diagnostics. Temp directory prefixes were also changed in a way that makes debugging less clear/consistent. Migration logging changes are likely fine, but the stdout/stderr split may become a UX/automation footgun if not intentional.
Additional notes (4)
- Maintainability |
apps/oxfmt/test/init.test.ts:7-12
fs.mkdtemp()expects a prefix, and it will append random characters to it. By removing the trailing-(and similarly in the new migrate tests), you reduce readability and make it harder to visually spot the random suffix. More importantly, using a prefix without a delimiter can be confusing when scanning temp directories.
This is also repeated across multiple tests; keeping a consistent pattern helps when debugging CI artifacts locally.
- Readability |
apps/oxfmt/test/init.test.ts:46-56
The old init tests explicitly asserted the error message on failure (e.g.stderrcontains "Oxfmt configuration file already exists."). The updated test only checksexitCode === 1, which reduces the value of the test: regressions that still exit non-zero but emit a wrong/empty message won’t be caught.
Since this is CLI behavior, verifying both exit code and a key portion of stderr is important.
- Maintainability |
apps/oxfmt/test/init.test.ts:45-56
This test case was removed, but the CLI behavior likely still needs to be validated: previously you asserted that--initaborts when.oxfmtrc.jsoncalready exists.
If the product behavior is still intended to guard against both .json and .jsonc, dropping the test reduces coverage and can allow a regression where --init overwrites or ignores an existing JSONC config.
- Maintainability |
apps/oxfmt/src-js/migration/migrate-prettier.ts:66-66
The migration script now prints indented bullet lines tostderrfor skipped options. There are no tests asserting on these messages (fine), but the change does create a subtle UX inconsistency: other messages in this function useconsole.log(...)(stdout) while these useconsole.error(...)(stderr). If the intent is “progress output”, stderr can make piping/automation noisier.
If the intent is specifically to warn, consider making that explicit (or standardizing where warnings go) so downstream tools can reliably parse stdout.
Summary of changes
Summary
This PR updates Oxfmt CLI tests and tweaks migration logging:
-
apps/oxfmt/src-js/migration/migrate-prettier.ts- Adjusts
console.error(...)messages for skipped/unsupported Prettier options to include an indented bullet prefix (" - ...").
- Adjusts
-
apps/oxfmt/test/init.test.ts- Renames the suite from
initto--init. - Simplifies assertions by reading/parsing
.oxfmtrc.jsoninline. - Reorders setup for the
$schematest. - Removes the separate
.oxfmtrc.jsonc already existstest and narrows the remaining “already exists” coverage.
- Renames the suite from
-
apps/oxfmt/test/migrate_prettier.test.ts(new)- Adds coverage for
--migrate prettier:- Creates
.oxfmtrc.jsonwhen no Prettier config exists. - Aborts when
.oxfmtrc.jsonexists. - Migrates
.prettierrcoptions. - Defaults
printWidthto80when missing. - Migrates
.prettierignoreintoignorePatterns.
- Creates
- Adds coverage for
203ffa2 to
a9148bd
Compare
Merge activity
|
270f454 to
d4c0bb7
Compare
a9148bd to
3dcb523
Compare

Fixes #15849