Skip to content

test(oxfmt): Update oxfmt --init|migrate tests#16865

Merged
graphite-app[bot] merged 1 commit intomainfrom
12-15-test_oxfmt_update_oxfmt_--init_migrate_tests
Dec 15, 2025
Merged

test(oxfmt): Update oxfmt --init|migrate tests#16865
graphite-app[bot] merged 1 commit intomainfrom
12-15-test_oxfmt_update_oxfmt_--init_migrate_tests

Conversation

@leaysgur
Copy link
Copy Markdown
Member

@leaysgur leaysgur commented Dec 15, 2025

Fixes #15849

@github-actions github-actions bot added A-cli Area - CLI A-formatter Area - Formatter C-test Category - Testing. Code is missing test cases, or a PR is adding them labels Dec 15, 2025
Copy link
Copy Markdown
Member Author

leaysgur commented Dec 15, 2025


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

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.

@leaysgur leaysgur changed the title test(oxfmt): Update oxfmt --init|migrate tests test(oxfmt): Update oxfmt --init|migrate tests Dec 15, 2025
Copy link
Copy Markdown

@charliecreates charliecreates bot left a comment

Choose a reason for hiding this comment

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

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. stderr contains "Oxfmt configuration file already exists."). The updated test only checks exitCode === 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 --init aborts when .oxfmtrc.jsonc already 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 to stderr for 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 use console.log(...) (stdout) while these use console.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 (" - ...").
  • apps/oxfmt/test/init.test.ts

    • Renames the suite from init to --init.
    • Simplifies assertions by reading/parsing .oxfmtrc.json inline.
    • Reorders setup for the $schema test.
    • Removes the separate .oxfmtrc.jsonc already exists test and narrows the remaining “already exists” coverage.
  • apps/oxfmt/test/migrate_prettier.test.ts (new)

    • Adds coverage for --migrate prettier:
      • Creates .oxfmtrc.json when no Prettier config exists.
      • Aborts when .oxfmtrc.json exists.
      • Migrates .prettierrc options.
      • Defaults printWidth to 80 when missing.
      • Migrates .prettierignore into ignorePatterns.

@charliecreates charliecreates bot removed the request for review from CharlieHelps December 15, 2025 01:41
@leaysgur leaysgur force-pushed the 12-15-test_oxfmt_update_oxfmt_--init_migrate_tests branch from 203ffa2 to a9148bd Compare December 15, 2025 02:14
@leaysgur leaysgur requested a review from Dunqing December 15, 2025 02:26
@graphite-app graphite-app bot added the 0-merge Merge with Graphite Merge Queue label Dec 15, 2025
@graphite-app
Copy link
Copy Markdown
Contributor

graphite-app bot commented Dec 15, 2025

Merge activity

@graphite-app graphite-app bot force-pushed the 12-12-feat_oxfmt_support_oxfmt_--migrate_js_side_ branch from 270f454 to d4c0bb7 Compare December 15, 2025 02:50
@graphite-app graphite-app bot force-pushed the 12-15-test_oxfmt_update_oxfmt_--init_migrate_tests branch from a9148bd to 3dcb523 Compare December 15, 2025 02:50
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Dec 15, 2025
Base automatically changed from 12-12-feat_oxfmt_support_oxfmt_--migrate_js_side_ to main December 15, 2025 02:56
@graphite-app graphite-app bot merged commit 3dcb523 into main Dec 15, 2025
19 checks passed
@graphite-app graphite-app bot deleted the 12-15-test_oxfmt_update_oxfmt_--init_migrate_tests branch December 15, 2025 02:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-cli Area - CLI A-formatter Area - Formatter C-test Category - Testing. Code is missing test cases, or a PR is adding them

Projects

None yet

Development

Successfully merging this pull request may close these issues.

oxfmt: --migrate prettier

2 participants