test(xsnap): Verify npm install build process #11051
Conversation
Deploying agoric-sdk with
|
| Latest commit: |
3035728
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d5d85289.agoric-sdk.pages.dev |
| Branch Preview URL: | https://kriskowal-xsnap-install-test.agoric-sdk.pages.dev |
| @@ -29,7 +29,7 @@ const ModdableSDK = { | |||
| * spawn: typeof import('child_process').spawn, | |||
| * }} io | |||
| */ | |||
| function makeCLI(command, { spawn }) { | |||
| export function makeCLI(command, { spawn }) { | |||
There was a problem hiding this comment.
This is an unabashèd shortcut, punting on the effort and thought that should go into extracting a utility for use in other packages or committing to a shared dependency that replaces the functionality. Whatever form that takes, we need to consider that the dependency will be a full dependency, not a devDependency, and assess risk accordingly. Given that there are intersecting changes in concurrent PRs, punt punt punt.
| const npm = makeCLI('npm', { spawn }); | ||
| const tar = makeCLI('tar', { spawn }); | ||
| const npmRes = await npm.pipe(['pack', '--json']); | ||
| const { | ||
| 0: { filename }, | ||
| } = JSON.parse(npmRes); | ||
| await tar.run(['xf', filename]); | ||
| await npm.run(['install'], { cwd: 'package' }); |
There was a problem hiding this comment.
Rather than extracting makeCLI, I recommend following userland's use of execa for testing as seen in e.g. fast-usdc.
| const npm = makeCLI('npm', { spawn }); | |
| const tar = makeCLI('tar', { spawn }); | |
| const npmRes = await npm.pipe(['pack', '--json']); | |
| const { | |
| 0: { filename }, | |
| } = JSON.parse(npmRes); | |
| await tar.run(['xf', filename]); | |
| await npm.run(['install'], { cwd: 'package' }); | |
| const { stdout: npmRes } = await execa`npm pack --json`; | |
| const { | |
| 0: { filename }, | |
| } = JSON.parse(npmRes); | |
| await execa`tar xf ${filename}`; | |
| await execa({ cwd: 'package' })`npm install`; |
| const { | ||
| 0: { filename }, | ||
| } = JSON.parse(npmRes); |
There was a problem hiding this comment.
Making this more idiomatic would improve readability.
| const { | |
| 0: { filename }, | |
| } = JSON.parse(npmRes); | |
| const [{ filename }] = JSON.parse(npmRes); |
The clean worktree CI test occurs before this test generates the junk. Unless I’m confused. But, I will be moving the junk to a temporary directory for idempotence. |
#9804 was supposed to have fixed #5140. If that's not the case, I'd like to understand how it failed. The test in which |
I see. I did not know we did our dirty check after running tests. The reason it succeeded is that this change, for unrelated reasons, adds |
Ah and the tgz is excluded in the root gitignore: Lines 42 to 43 in a27f724 |
refs: #9674
Description
In support of #9674, this change adds a regression test for
npm installto increase our confidence that changes tobuild.jssupport both local development and eventual installation from npm.Security Considerations
None.
Scaling Considerations
A design question for this is whether to use a temporary directory for the extracted package. I chose to use the default
packagesince this speeds up non-initial test runs. Sincetar xfoverlays the package over the previous version, there’s a small chance that the deletion of a sensitive file might go unnoticed locally, but not in CI.Documentation Considerations
None.
Testing Considerations
It’s a test, Bob.
Upgrade Considerations
None.