Skip to content

Core: Make csf-tools printing and CLI dispatcher output Windows-stable - #36346

Merged
ghengeveld merged 3 commits into
nextfrom
ci-fix/tests-unit-windows-daily-generated
Sep 18, 2026
Merged

ghengeveld merged 3 commits into
nextfrom
ci-fix/tests-unit-windows-daily-generated

Conversation

@obvious-autobuild

@obvious-autobuild obvious-autobuild Bot commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Closes #

What I did

tests-unit--windows in daily-generated failed 4 consecutive runs (SHAs bf830863e, 4f66ceeb2 x3) with three Windows-specific failure signatures, all verified mechanically:

  1. CRLF in reprinted config/CSF output (most of the 19 failures). printConfig and printCsf delegate to recast.print without a lineTerminator, and recast defaults that to os.EOL — so every reprinted line gained \r on Windows. Both wrappers now pin lineTerminator: '\n', matching what babelPrint in code/core/src/babel/babelParse.ts already does (including its comment explaining exactly this).
  2. Quote-style flips on freshly generated nodes (codemod tests expecting import preview from './preview'; received "...";). _inferQuotes() indexed token offsets into the raw source string; when source lines end in CRLF the offsets no longer align and the count falls back to double quotes. _inferQuotes now normalizes CRLF before counting (a no-op for LF sources).
  3. Benign wmic stderr leaked into the CLI help test (No Instance(s) Available.\r\r\n instead of empty stderr). dispatcher.ts calls getProcessAncestry() for every external-CLI command since CLI: Accept Node.js prerelease builds in the version gate #36320 landed 5f32ebb8, and process-ancestry's Windows implementation runs wmic via execSync without a stdio option, so wmic's stderr forwards to the CLI's stderr. process-ancestry@0.1.0 does not fix this, so the PR vendors the ancestry lookup as getProcessAncestry in storybook/internal/common — preserving the upstream Unix and Windows traversal, discarding child stderr via stdio: ['ignore', 'pipe', 'ignore'] — and drops the process-ancestry dependency entirely.

An earlier revision fixed #3 with a Yarn patch instead. That was unsafe for published packages: Yarn wires patches by writing patch:process-ancestry@... descriptors into the dependency manifests of core, cli-storybook, and create-storybook, and those descriptors ship with the published packages while the patch file only exists in the monorepo. Generated sandboxes install published packages from Verdaccio, so every sandbox install failed with ENOENT on .yarn/patches/process-ancestry-npm-0.0.2-a5c4b74866.patch. Vendoring removes the failure class instead of making the patch resolvable: no descriptor, no patch file, one less external dependency.

Adds two regression tests (ConfigObject.test.ts, CsfObject.test.ts) that feed CRLF sources to both printers and assert LF output and correct quote inference. Verified red against the pre-fix code and green after.

Adds a process-ancestry.test.ts unit suite covering PID validation, Unix traversal, Windows CSV parsing, stderr suppression, and cycle detection; TelemetryService.test.ts now mocks the vendored storybook/internal/common export instead of the removed package.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

Caution

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

No manual testing is needed: the failures are only observable on Windows (os.EOL = CRLF, wmic present), and the daily-generated workflow's tests-unit--windows job is itself the verification path. On Linux the new regression tests exercise both mechanisms via an os.EOL override and CRLF fixtures.

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

No documentation changes apply — internal CI/printing behavior only.

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts

  • Declare whether manual QA will be needed for this PR during the next release, through qa:needed or qa:skip

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary Release - 🚫 Not run

This PR does not have a canary release associated.

In-repo PRs: add the ci:canary label. Later pushes republish while the label remains.

Fork PRs: the label does nothing (a later push must not auto-publish). A maintainer publishes from this repository with Run workflow and the pr input. branch and sha are optional; if more than one is set, they must be the same commit. The fork author does not need to do anything.


Human author: Gert Hengeveld (gert@chromatic.com)

🔗 Obvious Project · 🧵 Obvious Thread

tests-unit--windows in daily-generated failed 4 consecutive runs
(bf83086, 4f66cee x3) with three failure signatures:

1. Reprinted config/CSF output contained CRLF on Windows. recast
   defaults lineTerminator to os.EOL; printConfig and printCsf now
   pin LF, matching the existing babelPrint wrapper.
2. _inferQuotes indexed token offsets into the raw source, which
   misreads when source lines end in CRLF; normalize CRLF before
   counting so quote inference is platform-independent.
3. The CLI help test captured benign wmic stderr ("No Instance(s)
   Available.") leaked by process-ancestry's execSync on Windows;
   patch the dependency to silence stderr while keeping stdout.

Adds regression tests covering CRLF sources for both printers.

Co-authored-by: Gert Hengeveld <gert@chromatic.com>
@obvious-autobuild obvious-autobuild Bot added bug ci:normal Run our default set of CI jobs (choose this for most PRs). qa:skip Pull Requests that do not need any QA. (e.g. documentation) labels Sep 17, 2026
@obvious-autobuild
obvious-autobuild Bot marked this pull request as ready for review September 17, 2026 02:00
The Yarn patch for process-ancestry leaked a patch: descriptor into the
published dependency manifests of core, cli-storybook, and
create-storybook, while the patch file itself only exists in the
monorepo. Generated sandboxes install published packages via Verdaccio
and failed with ENOENT when Yarn could not resolve the patch file.

Replace the patch with a repository-owned implementation in
storybook/internal/common that preserves the Unix and Windows ancestry
traversal, discards child stderr (wmic prints benign
"No Instance(s) Available." noise on Windows), and removes the external
dependency entirely. TelemetryService tests are updated to mock the new
barrel export.
The normal-generated run on 1f6cd6b had a single failure out of 10,734
tests: a beforeEach hook timing out at 10s in
code/frameworks/vue3-vite/src/plugins/vue-component-meta.test.ts (star
case). The suite passes locally (20/20), its sibling cases passed in the
same CI run, and the failure is a timeout, not an assertion. Re-running
through a fresh pipeline since the CircleCI retry API is not permitted.
@ghengeveld
ghengeveld merged commit f79f2e8 into next Sep 18, 2026
154 checks passed
@ghengeveld
ghengeveld deleted the ci-fix/tests-unit-windows-daily-generated branch September 18, 2026 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug ci:normal Run our default set of CI jobs (choose this for most PRs). qa:skip Pull Requests that do not need any QA. (e.g. documentation)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

localstorage QuotaExceededError

2 participants