fix: strengthen upgrade tests and show removed packages in UX by itsjustriley · Pull Request #3630 · Shopify/hydrogen

itsjustriley · 2026-03-26T15:39:16Z

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?

Strengthen renderTasks assertions: Verify correct task titles instead of just toHaveBeenCalled(); verify removal task is absent when no deps to remove
Port deleted changelog validation tests: ~270 lines from upgrade-flow.test.ts into new changelog-schema.test.ts — validates allowed fields, URL/version/date format, semver deps, base64 steps, dependenciesMeta
Show removed packages in upgrade UX: New "Removed packages" section in displayConfirmation and markdown list in generateUpgradeInstructionsFile. Uses extracted getAllRemovedPackages() helper to avoid duplication.

HOW to test your changes?

npx vitest run packages/cli/src/commands/hydrogen/upgrade.test.ts --pool=forks --poolOptions.forks.singleFork
npx vitest run packages/cli/src/commands/hydrogen/changelog-schema.test.ts --pool=forks --poolOptions.forks.singleFork

Checklist

I've read the Contributing Guidelines
I've considered possible cross-platform impacts (Mac, Linux, Windows)
I've added a changeset if this PR contains user-facing or noteworthy changes
I've added tests to cover my changes
I've added or updated the documentation

shopify · 2026-03-26T15:39:35Z

Oxygen deployed a preview of your fix/cli-upgrade-logic branch. Details:

Storefront	Status	Preview link	Deployment details	Last update (UTC)
Skeleton (skeleton.hydrogen.shop)	✅ Successful (Logs)	Preview deployment	Inspect deployment	March 27, 2026 2:00 PM

Learn more about Hydrogen's GitHub integration.

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>

itsjustriley · 2026-03-27T14:11:51Z

CI failure unrelated to this PR

kdaviduik · 2026-04-02T01:06:15Z

packages/cli/src/commands/hydrogen/upgrade.test.ts

+
+          const selectedRelease = releases.find(
+            (release) => release.version === '2023.10.0',
+          ) as (typeof releases)[0];


note: these as type assertions are a pre-existing pattern in this test file (~9 other occurrences). In test code, as for narrowing .find() results is a common and accepted pattern since the test execution itself validates the shape at runtime - if the release doesn't exist, the test will fail. Not worth blocking on given it's consistent with existing test conventions.

kdaviduik · 2026-04-02T01:06:15Z

packages/cli/src/commands/hydrogen/upgrade.test.ts

          );

+          // With empty removeDependencies, the "Removed packages" section should NOT appear
+          expect(info).not.toMatch('Removed packages');


praise: the expect(info).not.toMatch('Removed packages') assertion is a nice addition. This confirms the "Removed packages" section only appears when there are packages to remove - the kind of complementary negative assertion that makes the test suite more robust.

kdaviduik · 2026-04-02T02:31:33Z

packages/cli/src/commands/hydrogen/upgrade.test.ts

+          const tasks = vi.mocked(renderTasks).mock.calls[0]?.[0] ?? [];
+          const removalTask = tasks.find(
+            (t: {title: string}) =>
+              t.title === 'Removing deprecated dependencies',
+          );
+          expect(removalTask).toBeUndefined();


non-blocking: I think we could use expect.not.arrayContaining instead of reaching into mock.calls directly.

This works, but reaching into mock.calls[0]?.[0] is more fragile than using vitest's built-in matchers. The inline {title: string} type annotation is also a bit noisy. I think this reads more clearly as:

Suggested change

const tasks = vi.mocked(renderTasks).mock.calls[0]?.[0] ?? [];

const removalTask = tasks.find(

(t: {title: string}) =>

t.title === 'Removing deprecated dependencies',

);

expect(removalTask).toBeUndefined();

expect(renderTasks).toHaveBeenCalledWith(

expect.not.arrayContaining([

expect.objectContaining({ title: 'Removing deprecated dependencies' }),

]),

expect.anything(),

);

kdaviduik · 2026-04-02T03:00:35Z

.changeset/removed-packages-ux.md

+'@shopify/cli-hydrogen': patch
+---
+
+Show removed packages in the upgrade confirmation prompt and upgrade instructions file so merchants can see which dependencies will be cleaned up during the upgrade.


nit:

Suggested change

Show removed packages in the upgrade confirmation prompt and upgrade instructions file so merchants can see which dependencies will be cleaned up during the upgrade.

Show removed packages in the upgrade confirmation prompt and upgrade instructions file so that it's visible which dependencies will be cleaned up during the upgrade.

or something like that

fredericoo · 2026-04-07T14:23:11Z

packages/cli/src/commands/hydrogen/upgrade.ts

 * Generates a markdown file with upgrade instructions
 */
-async function generateUpgradeInstructionsFile({
+export async function generateUpgradeInstructionsFile({


question: is this export solely for the new test, or is there a planned consumer?

If it's purely for testability, that's fine - it's consistent with other exports in this file (displayConfirmation, getChangelog, getCumulativeRelease, etc.).

ideally though we would only test exposed interfaces – test coverage for this function would be inside the exported ones

fredericoo · 2026-04-07T14:23:11Z

packages/cli/src/commands/hydrogen/upgrade.ts

  const {features, fixes} = cumulativeRelease;
-  if (features.length || fixes.length) {
+  const allRemovedPackages = getAllRemovedPackages(cumulativeRelease);
+


non-blocking: nice catch expanding the guard to include removed packages. This means a release with only removals (no features/fixes) will now show the info block and generate the instructions file - which is the correct behaviour.

worth noting this is a net-new UX path that the current tests don't exercise directly (the test for "displays removed packages" uses CUMULATIVE_RELEASE which IIRC has features/fixes in it).

a quick test with empty features/fixes + non-empty removals would cover the edge case, but not blocking.

fredericoo · 2026-04-07T14:23:11Z

packages/cli/src/commands/hydrogen/upgrade.test.ts

+          );
+
+          expect(mdContent).toContain('## Removed packages');
+          expect(mdContent).toContain('- `@remix-run/react`');


non-blocking: there's a test for "markdown includes removed packages when present" but no corresponding negative test for "markdown omits Removed packages when there are none".

The code guard is simple (if (allRemovedPackages.length)), and displayConfirmation already has the negative assertion. would be nice to have the pair complete for generateUpgradeInstructionsFile too.

fredericoo · 2026-04-07T14:23:11Z

packages/cli/src/commands/hydrogen/changelog-schema.test.ts

+
+    for (const release of changelog.releases) {
+      // Fail fast on malformed entries rather than silently skipping them
+      expect(release).toBeDefined();


non-blocking: the expect(release).toBeDefined(); if (!release) continue; pattern repeats 6 times in this file - worth extracting.

The expect fails fast if undefined and the continue narrows for TypeScript - I get why both are there. But an assertion function would let us drop the guard:

function assertDefined<T>(value: T): asserts value is NonNullable<T> { expect(value).toBeDefined(); }

we have a similar one for playwright!

then each test becomes assertDefined(release) and TypeScript narrows automatically. i dont mind repetition but the type narrowing could be useful too

fredericoo · 2026-04-07T14:23:11Z

packages/cli/src/commands/hydrogen/changelog-schema.test.ts

+      );
+    }
+
+    const changelog = await upgradeModule.getChangelog();


non-blocking: getChangelog() is called independently in every test - since this is a static JSON file, a shared beforeAll parse would reduce boilerplate and be slightly faster.

probably carried over from the deleted tests, and porting is an opportunity to tidy up.

itsjustriley and others added 2 commits March 27, 2026 09:57

itsjustriley force-pushed the fix/cli-upgrade-logic branch from bbaafb4 to 3432021 Compare March 27, 2026 13:58

itsjustriley marked this pull request as ready for review March 27, 2026 14:13

itsjustriley requested a review from a team as a code owner March 27, 2026 14:13

itsjustriley requested review from andguy95, fredericoo and kdaviduik March 27, 2026 14:13

kdaviduik approved these changes Apr 2, 2026

View reviewed changes

kdaviduik reviewed Apr 2, 2026

View reviewed changes

fredericoo reviewed Apr 7, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: strengthen upgrade tests and show removed packages in UX#3630

fix: strengthen upgrade tests and show removed packages in UX#3630
itsjustriley wants to merge 2 commits intomainfrom
fix/cli-upgrade-logic

itsjustriley commented Mar 26, 2026 •

edited

Loading

Uh oh!

shopify bot commented Mar 26, 2026 •

edited

Loading

Uh oh!

itsjustriley commented Mar 27, 2026

Uh oh!

kdaviduik Apr 2, 2026

Uh oh!

kdaviduik Apr 2, 2026

Uh oh!

kdaviduik Apr 2, 2026

Uh oh!

kdaviduik Apr 2, 2026

Uh oh!

fredericoo Apr 7, 2026

Uh oh!

fredericoo Apr 7, 2026

Uh oh!

fredericoo Apr 7, 2026

Uh oh!

fredericoo Apr 7, 2026

Uh oh!

fredericoo Apr 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	Show removed packages in the upgrade confirmation prompt and upgrade instructions file so merchants can see which dependencies will be cleaned up during the upgrade.
	Show removed packages in the upgrade confirmation prompt and upgrade instructions file so that it's visible which dependencies will be cleaned up during the upgrade.

Conversation

itsjustriley commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHY are these changes introduced?

WHAT is this pull request doing?

HOW to test your changes?

Checklist

Uh oh!

shopify bot commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itsjustriley commented Mar 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

itsjustriley commented Mar 26, 2026 •

edited

Loading

shopify bot commented Mar 26, 2026 •

edited

Loading