You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
While reviewing PR #3539, we found an unrelated change to packages/playground/cli/tests/run-cli.spec.ts. The change does not belong in the OPFS SQLite snapshot PR, but it documents a real test-quality issue in the CLI symlink coverage.
The existing test tries to prove symlink access works in a secondary PHP runtime by firing multiple concurrent HTTP requests. That is probabilistic: concurrent requests may or may not exercise the runtime path the test claims to cover, and the old TODOs in the test already call out that weakness.
Findings
The test currently relies on request routing/concurrency to hopefully exercise a non-primary PHP instance.
It does not explicitly assert the worker topology.
It can pass without proving that a secondary PHP runtime executed the symlinked script.
Running the CLI server with workers: 1 so HTTP request routing cannot be the source of secondary runtime behavior.
Asserting workerThreadCount === 1.
Writing a wrapper PHP file that runs the symlinked script once in the primary runtime with require and once through shell_exec('php ...'), which exercises the secondary PHP runtime path directly.
Asserting both primary:Slept and secondary:Slept appear in the response.
diff --git a/packages/playground/cli/tests/run-cli.spec.ts b/packages/playground/cli/tests/run-cli.spec.ts
index 472919da..663a272a 100644
--- a/packages/playground/cli/tests/run-cli.spec.ts+++ b/packages/playground/cli/tests/run-cli.spec.ts@@ -232,8 +232,6 @@ describe.each(blueprintVersions)(
? { allow: 'follow-symlinks' }
: { followSymlinks: true };
- // TODO: Make sure test always uses a single worker.- // TODO: Is there a way to confirm we are testing use of a non-primary PHP instance?
const tmpDir = await mkdtemp(
path.join(tmpdir(), 'playground-test-')
);
@@ -266,6 +264,9 @@ describe.each(blueprintVersions)(
...testArgs,
debug: true,
command: 'server',
+ // Keep one CLI worker so the secondary PHP runtime below+ // is created by the PHP spawn handler, not request routing.+ workers: 1,
'mount-before-install': [
{
hostPath: symlinkPath,
@@ -273,23 +274,29 @@ describe.each(blueprintVersions)(
},
],
});
- // Make multiple simultaneous requests to force the use of a secondary PHP instance.- // TODO: Find way to confirm this. Maybe a custom response header that announces the worker.- const sleepUrl = new URL(- '/wp-content/test-script/sleep.php',+ expect(+ cliServer[internalsKeyForTesting].workerThreadCount+ ).toBe(1);++ await cliServer.playground.writeFile(+ '/wordpress/run-symlinked-script.php',+ `<?php+ echo 'primary:';+ require '/wordpress/wp-content/test-script/sleep.php';+ echo "\nsecondary:";+ // Running php from PHP exercises a secondary PHP runtime.+ echo shell_exec('php /wordpress/wp-content/test-script/sleep.php');+ `+ );+ const scriptUrl = new URL(+ '/run-symlinked-script.php',
cliServer.serverUrl
);
- const responses = await Promise.all([- fetch(sleepUrl),- fetch(sleepUrl),- // Test a third request to hopefully test more than one secondary instance.- fetch(sleepUrl),- ]);- for (const response of responses) {- expect(response.status).toBe(200);- const text = await response.text();- expect(text).toContain('Slept');- }+ const response = await fetch(scriptUrl);+ const text = await response.text();+ expect(response.status, text).toBe(200);+ expect(text).toContain('primary:Slept');+ expect(text).toContain('secondary:Slept');
} finally {
if (existsSync(symlinkPath)) {
unlinkSync(symlinkPath);
Notes
This patch was removed from PR #3539 to keep that PR scoped to SQLite OPFS snapshot persistence.
Problem
While reviewing PR #3539, we found an unrelated change to
packages/playground/cli/tests/run-cli.spec.ts. The change does not belong in the OPFS SQLite snapshot PR, but it documents a real test-quality issue in the CLI symlink coverage.The existing test tries to prove symlink access works in a secondary PHP runtime by firing multiple concurrent HTTP requests. That is probabilistic: concurrent requests may or may not exercise the runtime path the test claims to cover, and the old TODOs in the test already call out that weakness.
Findings
Proposed Fix
Make the test deterministic by:
workers: 1so HTTP request routing cannot be the source of secondary runtime behavior.workerThreadCount === 1.requireand once throughshell_exec('php ...'), which exercises the secondary PHP runtime path directly.primary:Sleptandsecondary:Sleptappear in the response.Patch From PR #3539 To Reapply Separately
Notes
This patch was removed from PR #3539 to keep that PR scoped to SQLite OPFS snapshot persistence.