Conversation
|
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
itsjustriley
force-pushed
the
e2e-upgrade-tests
branch
10 times, most recently
from
8df9450 to
eed6a44
Compare
itsjustriley
force-pushed
the
e2e-upgrade-tests
branch
from
eed6a44 to
db4b831
Compare
|
We detected some changes in |
| import {exec} from '@shopify/cli-kit/node/system'; | ||
| import {execAsync} from '../../lib/process.js'; | ||
| import * as upgradeModule from './upgrade.js'; | ||
| import {spawn, ChildProcess} from 'node:child_process'; |
There was a problem hiding this comment.
(threading)
Issue/bug: scaffolding only downgrades @shopify/hydrogen, not other dependencies
What: scaffoldHistoricalProject() copies the current skeleton and only changes version and dependencies['@shopify/hydrogen']. All other dependencies remain at HEAD
versions.
Why it matters: The upgrade command thinks it's upgrading from the previous version, but most dependencies are already at the latest version. This means the test can pass without going through the full upgrade flow that merchants experience. Also, this may cause issues down the line (eg: Remix --> RR7 is an example where this bug would've caused issues)
Fix: Use previousRelease.dependencies and previousRelease.devDependencies from the changelog to set ALL starting dependency versions, not just hydrogen:
if (previousRelease.dependencies) {
for (const [dep, ver] of Object.entries(previousRelease.dependencies)) {
packageJson.dependencies[dep] = ver;
}
}
if (previousRelease.devDependencies) {
for (const [dep, ver] of Object.entries(previousRelease.devDependencies)) {
packageJson.devDependencies[dep] = ver;
}
}
| renderConfirmationPrompt: vi.fn(() => Promise.resolve(true)), // Always confirm | ||
| renderInfo: vi.fn(() => {}), // Mock renderInfo to silence output | ||
| renderSuccess: vi.fn(() => {}), // Mock renderSuccess to silence output | ||
| renderConfirmationPrompt: vi.fn(() => Promise.resolve(true)), |
There was a problem hiding this comment.
(threading)
This also eliminates (without replacement) a few other validations/checks of the changelog.json file:
I definitely think we should have these validations, though this test doesn't feel like the place to put it imo. I think we should just have a CI step that ensures the JSON formatting is correct.
Or even better but less trivial would be to use a Zod schema to validate this JSON file
There was a problem hiding this comment.
agree on the schema validation for the json file
but i think our tests were too flaky to begin with
what do we want to test?
my educated guess is:
- whether changelog.json matches a specific format
- whether we can read the package.json from a project
- whether we can detect what "hydrogen packages" version a package.json has
- whether we can detect if they are on the latest
- whether we can modify package versions in a package.json
- whether we can call the install command on behalf of the user
for none of those we need to fetch an external changelog from a backend
but our functions in upgrade.ts are so bound together and filesystem dependant, that makes testing really hard
pseudocode for what i imagine would be easy to test for all those cases:
function upgrade(packageJsonService, changelog) {
const versions = getHydrogenPackageVersions(packageJsonService);
const needUpgrading = getOutdatedPackageVersions(versions, changelog);
if (needUpgrading.length === 0) return;
await updatePackageVersions(packageJsonService);
await installPackages();
return;
}(extra points if we use Effect, or at least errors as values)
maybe we should push a refactor of upgrade as tech debt?
| ); | ||
| } | ||
| } | ||
| process.env.FORCE_CHANGELOG_SOURCE = 'local'; |
There was a problem hiding this comment.
these lines set FORCE_CHANGELOG_SOURCE, SHOPIFY_HYDROGEN_FLAG_FORCE, and CI on process.env but never restore them.
Why it matters: These leak to subsequent tests in the same worker. The sibling upgrade.test.ts handles this correctly with save/restore.
Fix: Use vi.stubEnv() (auto-restores on teardown) or save/restore in afterEach.
| async function scaffoldHistoricalProject(commit: string): Promise<string> { | ||
| const tempDir = await mkdtemp(join(tmpdir(), 'hydrogen-upgrade-test-')); | ||
| const projectDir = join(tempDir, 'test-project'); | ||
| const projectDir = await scaffoldHistoricalProject(fromVersion); |
There was a problem hiding this comment.
The function no longer scaffolds a historical project — it copies the current skeleton and changes the version. I think we should rename this to something like scaffoldProjectAtVersion(version) or createTestProjectWithVersion(version).
| } | ||
| } | ||
| async function scaffoldHistoricalProject(version: string): Promise<string> { | ||
| const tempDir = await mkdtemp(join(tmpdir(), 'hydrogen-upgrade-test-')); |
There was a problem hiding this comment.
from Claude, seems reasonable
mkdtempcreates directories that are never cleaned up. UseinTemporaryDirectoryfrom@shopify/cli-kit/node/fs(like the sibling test does) or add cleanup inafterAll.
| if (!reinstalledDeps[dep]) { | ||
| expect(packageJson.dependencies?.[dep]).toBeUndefined(); | ||
| } | ||
| if (!hasUpgradeSteps) { |
There was a problem hiding this comment.
Let's add a comment here (explaining that projects with manual upgrade steps may not build until those steps are applied) about why we are doing this since it's not necessarily obvious. .
| }); | ||
| } | ||
| } | ||
| }, 180000); |
There was a problem hiding this comment.
(threading)
If someone submitted a changelog.json PR before the package(s) being upgraded have been released, the upgrade tests would skip and CI would appear successful, but we would have zero guarantees the change actually works.
Imo this test should fail if any packages specified haven't been released yet. This could be due to a genuine bug/typo, or because our packages haven't been released yet. Either way, I think this test failing if the package version doesn't exist would be helpful
3 participants
WHY are these changes introduced?
Fixes https://github.com/Shopify/developer-tools-team/issues/1045
The existing upgrade flow tests were unreliable. Tests would intermittently fail in CI due to dev server flakiness (port conflicts, spawn/kill race conditions, HTTP timeouts), making it difficult to validate changelog.json entries/upgrade flow. This should reduce toil for future releases.
WHAT is this pull request doing?
This PR refactors the upgrade flow tests to be more focused, and fail faster to ensure that new changelog.json entries enable successful upgrades.
The test validates:
Key Decisions:
h2 upgradecommandHOW to test your changes?
Run test locally:
Expected: Both tests pass
Verify CI workflow:
- Modify docs/changelog.json (add a comma or whitespace)
- Push to a branch
- The test-upgrade-flow.yml workflow should trigger automatically
- Check that it passes
Checklist