fix: repair post-release dev-preview NUTs (W-23400737)#676
Conversation
The dev-preview e2e NUTs added in #626 fail in the post-CLI-release integration pipeline due to three layered issues: 1. API-version drift: scratch orgs are provisioned on the platform's current default API version (e.g. 68.0), but the plugin only bundles dev-server dists for the versions in package.json apiVersionMetadata (66.0/67.0). The dev server rejects the unsupported version and exits before printing a preview URL, so every browser test hits the 30s getPreviewURL timeout. Pin the spawned dev server to the plugin's max supported API version (getMaxSupportedApiVersion) so the tests are decoupled from platform version drift. 2. Sinon cwd cascade: TestSession stubs process.cwd before creating orgs; if org creation throws, the stub leaks and every later file dies with a misleading "Attempted to wrap cwd which is already wrapped", masking the real error. Restore the leaked stub on setup failure, and add retries (TESTKIT_SETUP_RETRIES, default 3) for intermittent scratch-org signup. 3. Secret redaction: `sf org display user --json` now redacts accessToken by default, breaking frontdoor SSO with a 401. Read the token directly from the org connection via @salesforce/core instead. Full NUT suite (12 tests) verified passing locally against a healthy DevHub. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| const pkg = JSON.parse(fs.readFileSync(pkgPath, 'utf8')) as { apiVersionMetadata?: Record<string, unknown> }; | ||
| const versions = Object.keys(pkg.apiVersionMetadata ?? {}); | ||
| if (versions.length === 0) { | ||
| throw new Error(`No apiVersionMetadata found in ${pkgPath}`); |
There was a problem hiding this comment.
nit: should we try a default version?
There was a problem hiding this comment.
Intentional fail-fast here rather than a default. apiVersionMetadata is a hard requirement for the plugin — the shipping dependencyLoader.ts reads packageJson.apiVersionMetadata directly and would break too if it were ever empty, so an empty map means the plugin is misconfigured, not in a transient state a test should paper over.
There's also no existing "default API version" constant anywhere in the source to fall back to, so a default would have to be a hardcoded magic number (e.g. '67.0'). That would silently rot as versions move, and could pin the tests to a version with no matching @lwc/sfdx-local-dev-dist-<v> dist — producing a more confusing downstream import failure than the clear "No apiVersionMetadata found" we throw at the source.
Happy to revisit if we ever introduce a canonical default version the plugin resolves against.
wjhsf
left a comment
There was a problem hiding this comment.
What happens when the platform gets a new default API version, but this package doesn't get updated?
| * @returns The session ID string. | ||
| */ | ||
| export function getAccessToken(session: TestSession): string { | ||
| export async function getAccessToken(session: TestSession): Promise<string> { |
There was a problem hiding this comment.
| export async function getAccessToken(session: TestSession): Promise<string> { | |
| async function getAccessToken(session: TestSession): Promise<string> { |
not used outside this file, doesn't need to be exported
There was a problem hiding this comment.
Good catch — addressed in follow-up #677. getAccessToken is now non-exported (it's only used by getPreview in this file).
| // Number of times TestSession will retry scratch org creation before failing. Scratch org | ||
| // signup is intermittently flaky (RemoteOrgSignupFailed / C-9999); retrying avoids spurious | ||
| // failures in the post-release pipeline. Overridable via TESTKIT_SETUP_RETRIES. | ||
| const SETUP_RETRIES = Number.parseInt(process.env.TESTKIT_SETUP_RETRIES ?? '', 10) || 3; |
There was a problem hiding this comment.
Curious use of Number.parseInt instead of global parseInt.
There was a problem hiding this comment.
Switched to the global parseInt in #677 for consistency — the rest of the repo (src/configMeta.ts, src/shared/previewUtils.ts, and devServerUtils.ts right next door) uses the global form, so Number.parseInt was the odd one out here. No behavioral difference either way.
That's precisely the drift this change is built to absorb, so the answer is "the e2e tests keep passing." Before this fix they broke on exactly that scenario. The mechanism:
The one thing this deliberately does not do is give the plugin real support for a newer API version than it ships — that's a separate product task (adding the dist + metadata + types), not something a test should silently paper over. The tests validate what the plugin actually supports today, decoupled from platform drift. Full root-cause writeup is in the #676 description. |
What & Why
The dev-preview e2e NUTs added in #626 (
test/commands/lightning/dev/component-preview/*.nut.ts) pass in this repo's own CI but fail in the post-CLI-release integration pipeline (which runs them against a released, bundled CLI). This PR fixes them. Tracked by @W-23400737@.Three layered failures were diagnosed, each masking the next:
1. API-version drift (the real, deterministic bug)
Scratch orgs are provisioned on the platform's current default API version (e.g.
68.0), but the plugin only bundles LWC dev-server dists for the versions inpackage.jsonapiVersionMetadata(66.0/67.0).dependencyLoader.tsdoes an exact-match check and throws, so the dev server exits before printing a preview URL, so every browser test hits the 30sgetPreviewURLtimeout.Fix: added
getMaxSupportedApiVersion()(readspackage.json apiVersionMetadata) and spawn the dev server with--api-version <max>. This decouples the e2e tests from platform version drift and auto-tracks new versions as they're added topackage.json. (Adding real68.0support to the shipping plugin is a separate product task.)2. Sinon cwd cascade (masking bug)
TestSession's constructor stubsprocess.cwdbefore creating scratch orgs. If org creation throws,create()rejects without returning, the stub leaks, and every subsequent file dies with a misleadingAttempted to wrap cwd which is already wrapped— hiding the true error.Fix: restore the leaked stub on setup failure before rethrowing, so the real failure surfaces in the file that actually failed. Also added
retries(default 3, overridable viaTESTKIT_SETUP_RETRIES) for intermittent scratch-org signup (RemoteOrgSignupFailed/ C-9999).3. Secret redaction, frontdoor 401
sf org display user --jsonnow redactsaccessTokenby default (returns a[REDACTED] ...placeholder), so frontdoor SSO fails with HTTP 401 and the component never renders.Fix: read the token directly from the org connection via
@salesforce/core(Org.create().getConnection().accessToken) instead of shelling out.Testing
Full NUT suite verified passing locally against a healthy DevHub, and green in CI on both
ubuntu-latestandwindows-latest:Scope: test-helpers only (
test/commands/lightning/dev/helpers/) — no changes to shipping plugin code.🤖 Generated with Claude Code