Skip to content

Refactor fixture tests and fix declaration spacing idempotency - #240

Open
Sha1rholder wants to merge 6 commits into
nushell:mainfrom
Sha1rholder:refactor-tests
Open

Sha1rholder wants to merge 6 commits into
nushell:mainfrom
Sha1rholder:refactor-tests

Conversation

@Sha1rholder

@Sha1rholder Sha1rholder commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

This is a major refactor. In both the Rust and Nushell test runners, manually registered fixture tests have been replaced with automatic discovery. Adding a new fixture no longer requires modifying the test code.

  • Organize fixtures using the tests/fixtures/<category>/<case>/ structure. Each case must contain expected.nu, and may optionally contain input.nu and config.noun. Redundant inputs have been removed, and the basic, complex, and indentation-related fixtures are now included in the full test suite.
  • Verify that format(expected) == expected; if an input file exists, also verify that format(input) == expected. Each file is formatted only once, and comparisons are byte-for-byte exact, including whitespace, line-ending type, and the final newline at EOF.
  • When output does not match, save the result as either not_idempotent.nu or unexpected.nu. Once the corresponding check passes, the generated artifact is automatically cleaned up. Failures from all cases are collected instead of stopping at the first failure. Runner regression tests have also been added to cover fixture discovery, configuration, byte-level comparison, error handling, and artifact files.
  • The two special cases that previously skipped idempotency checks have been fixed automatically as a result of updates to the Nushell AST parser.
  • Fix the formatting behavior of complex.nu: determine spacing between adjacent top-level declarations based on the formatted layout. Expanded tables and collapsed collections now produce stable spacing on the first formatting pass. Regression tests have also been added for margins and surrounding comments.
  • Integration tests now use the binary path provided by Cargo, eliminating duplicate fixture execution.
  • Update the Nushell test runner so that it can apply per-case configuration and support automatically discovered category and case names.

Example

A deliberately broken test:

image

After running the tests once, if a test fails, a Git-visible temporary file is created directly inside the corresponding test case. Its filename indicates the type of failure: unexpected.nu means that formatting input.nu did not produce expected.nu, while not_idempotent.nu means that expected.nu itself is not idempotent. Now you can check the idempotency of expected.nu without a same input.nu.

image

After fixing the issue and running the tests again, these temporary artifacts are automatically removed if the tests pass, so no manual cleanup is required. This makes fixture testing more convenient.


It should also make fixtures more "discoverable", as input, expectations, and error outputs are now stored together.

image

I did design it but I have to commit that there's a LOT of llm in this PR : (
I'm also not confident with the expression in README because it's translated fron Chinese.

- Discover fixtures exactly two directory levels deep in both runners
- Apply per-case configs and compare single-pass output byte for byte
- Support optional inputs and save or clear failure artifacts
- Remove manual registration, idempotency exceptions and duplicate tests
- Populate basic and complex expectations and add missing final newlines
- Add runner regression tests and update testing documentation
Base declaration spacing on formatted layout instead of source newlines.
Cover table expansion, collection collapse, comments, and margin
settings. Update the complex fixture to match the stable output.
@fdncred

fdncred commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

This is too hard to read and needs to be broken up into more than 1 PR.

  1. pr1 move fixtures around, no code changes
  2. pr2 code changes

@Sha1rholder

Copy link
Copy Markdown
Contributor Author

This is too hard to read and needs to be broken up into more than 1 PR.

  1. pr1 move fixtures around, no code changes
  2. pr2 code changes

I've tried my best to reduce the size of the PR, but this is already the smallest PR that can make all CI pass.

Alternatively could you create a new temporary branch from main branch, and I'll submit multiple PRs in logical order to that temporary branch for your reference? That way at least I won't break the CI on main branch. Only the last PR would pass all CI.

@Sha1rholder

Copy link
Copy Markdown
Contributor Author

@fdncred Would it be okay if I split this PR into several smaller PRs but merge them into a branch in my own fork, and re-submit a combined PR to nufmt:main? It won't break any CI then. Otherwise I can just create some stacked PRs.

@fdncred fdncred left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Notes from my clanker.

I ran this locally. cargo test passes, and the Nushell runner passes all 272 checks. I also formatted about 4,000 .nu files under my ~/src with main and with this branch. 23 files come out different on the first pass, and in every one the change is blank lines around a let/const that expands to several lines. Files that change on a second format went from 50 to 27, and nothing that was stable on main became unstable. So the spacing fix does what it says.

Before merge

  1. config.noun looks like a typo for config.nuon. Is it on purpose? It's in the README in 3 places, both runners, the runner tests and the 6 fixtures that have one. Anyone who adds a config.nuon today gets it silently ignored, and the case runs with default settings.
  2. Fixtures without their own config pick up any nufmt.nuon that nufmt finds walking up from the repo root, because format_via_stdin only passes --config when the case has one. main's harness had the same problem, so this isn't new. It matters more now because a failure writes unexpected.nu or not_idempotent.nu into the tree, and someone with a ~/nufmt.nuon would get a pile of bogus failures and files. Always passing --config with a committed default file, or running nufmt from a temp dir, would fix it. run_ground_truth_tests.nu needs the same change since it does cd $PROJECT_DIR.
  3. With every fixture inside one #[test], cargo test let_statement can't pick out a single case anymore. The old macro allowed that. An env var filter checked in run_fixtures would be enough, e.g. NUFMT_FIXTURE=core_language_constructs/let_statement cargo test fixtures. libtest-mimic would bring back real per-case tests, but that's more work.

A question

Should the new declaration spacing have a nufmt.nuon option? I lean no. The old rule looked at the source layout, which is why it wasn't idempotent, so there's no stable behavior worth keeping. It does change first-pass output for existing files, which is where the 23 files above come from.

Nits

  • The doc comment on separator_newlines_between_top_level_pipelines (blocks.rs:88) still says it decides how many newlines to emit. With the new caller it's a minimum, because newlines that comment handling already wrote are never removed.
  • When a case has no input.nu, an old unexpected.nu never gets cleaned up. The runner test asserts this, so I assume it's intended, but I'd rather remove it. It's one remove_file call that ignores NotFound.
  • The splice in format_block looks safe to me, since it only shifts bytes after separator_start. A short comment saying why would help the next person who touches it. It also copies the just-formatted pipeline for every top-level pair when margin is 1 or more. I doubt that's measurable, so no change needed.
  • The Nushell runner uses whatever is in target/release/nufmt, even if it's stale. That isn't new, but now a stale binary also writes files into the tree. A release binary left over from main gave me a false FAIL other/complex.

Dropping the flake.nix patch in favor of CARGO_BIN_EXE_nufmt is a nice cleanup.

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.

2 participants