fix: strengthen upgrade tests and show removed packages in UX#3630
Open
itsjustriley wants to merge 4 commits intomainfrom
Open
fix: strengthen upgrade tests and show removed packages in UX#3630itsjustriley wants to merge 4 commits intomainfrom
itsjustriley wants to merge 4 commits intomainfrom
Conversation
Contributor
|
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
itsjustriley
force-pushed
the
fix/cli-upgrade-logic
branch
from
bbaafb4 to
3432021
Compare
Contributor
Author
|
CI failure unrelated to this PR |
kdaviduik
approved these changes
Apr 2, 2026
kdaviduik
reviewed
Apr 2, 2026
fredericoo
reviewed
Apr 7, 2026
Contributor
|
couple unit tests are failing, maybe try merging/rebasing main back in? i think we fixed those a lil bit ago |
itsjustriley
force-pushed
the
fix/cli-upgrade-logic
branch
from
9540a0e to
d6a9eb7
Compare
Merchants upgrading across versions that remove packages (e.g. the Remix → React Router migration) had no visibility into what was being cleaned up. Now they see a "Removed packages" section in both the upgrade confirmation prompt and the generated instructions markdown. - Extract getAllRemovedPackages() helper to avoid duplicating the removeDependencies + removeDevDependencies concatenation across displayConfirmation and generateUpgradeInstructionsFile - Extend displayConfirmation to show removed packages list when present - Extend generateUpgradeInstructionsFile to include a "## Removed packages" section with backtick-wrapped package names - Export generateUpgradeInstructionsFile for testability Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
The upgrade command tests had weak assertions that could pass vacuously, and changelog validation coverage was lost when tests were deleted from upgrade-flow.test.ts. Upgrade test improvements: - Verify renderTasks receives specific task titles instead of just toHaveBeenCalled() - Assert removal task is absent when no deps to remove (direct mock inspection pattern) - Add tests for removed packages display in confirmation prompt and generated markdown instructions New changelog-schema.test.ts (~290 lines): - Validates changelog.json schema: allowed fields, required fields, URL/version/date formats, semver deps, base64 steps, dependenciesMeta - Uses semver.validRange() instead of a permissive regex for version validation — the regex accepted trailing garbage like 1.0.0!@#$% - Precondition assertions on required-field loops prevent vacuous passes (e.g. if all releases lost their features/fixes arrays, the test would still pass green without the precondition) - Optional fields (dependenciesMeta, removeDependencies) intentionally have no preconditions — a valid changelog may have zero instances Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
- Replace fragile `mock.calls[0]?.[0]` traversal with
`expect.not.arrayContaining` per kdaviduik's suggestion; the
built-in matcher is resilient to call-order changes and removes
the need for the inline `{title: string}` cast
- Add negative test for `generateUpgradeInstructionsFile` asserting
the "Removed packages" section is omitted when there are no removals
- Add edge-case test for both `displayConfirmation` and
`generateUpgradeInstructionsFile` when a release has only removals
and no features/fixes — covering the new UX path noted by fredericoo
- Extract `assertDefined<T>` type assertion helper in
`changelog-schema.test.ts` to replace the 6x repeated
`expect(release).toBeDefined(); if (!release) continue;` pattern;
after calling `assertDefined`, TypeScript narrows the type so the
manual guard is no longer needed
- Hoist `getChangelog()` into `beforeAll` in `changelog-schema.test.ts`
to parse the static JSON once instead of once per test (~9 calls)
- Update changeset wording per kdaviduik's nit
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When `allRemovedPackages` is present but `fixes` is absent, the prior section (h1/breaking/features) already ends with `\n----\n`. The old code unconditionally prepended `\n----\n\n` before "## Removed packages", producing a `----\n\n----` double horizontal rule in the output. Fix: conditionally add the separator only when `fixesMd.length > 0` (fixes emits no trailing separator, so we supply one) vs. `'\n'` when fixes is absent (prior section already terminated with `----\n`). Also replace silent `if (!item) continue` / `if (!step) continue` guards in changelog-schema.test.ts with `assertDefined()` so null/ undefined entries in changelog.json cause an explicit test failure rather than a silent skip that masks malformed data.
itsjustriley
force-pushed
the
fix/cli-upgrade-logic
branch
from
d6a9eb7 to
1198f9c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WHY are these changes introduced?
Fixes https://github.com/Shopify/developer-tools-team/issues/1113
Fixes https://github.com/Shopify/developer-tools-team/issues/1138
Fixes https://github.com/Shopify/developer-tools-team/issues/1107
Upgrade command tests had weak assertions that could pass vacuously, changelog validation coverage was lost when tests were deleted, and the upgrade UX didn't surface removed packages.
WHAT is this pull request doing?
renderTasksassertions: Verify correct task titles instead of justtoHaveBeenCalled(); verify removal task is absent when no deps to removeupgrade-flow.test.tsinto newchangelog-schema.test.ts— validates allowed fields, URL/version/date format, semver deps, base64 steps, dependenciesMetadisplayConfirmationand markdown list ingenerateUpgradeInstructionsFile. Uses extractedgetAllRemovedPackages()helper to avoid duplication.HOW to test your changes?
npx vitest run packages/cli/src/commands/hydrogen/upgrade.test.ts --pool=forks --poolOptions.forks.singleForknpx vitest run packages/cli/src/commands/hydrogen/changelog-schema.test.ts --pool=forks --poolOptions.forks.singleForkChecklist