acc: run cmd/workspace/apps/run-local-node locally - #6172
Draft
radakam wants to merge 5 commits into
Draft
Conversation
The test was disabled in #4765 for using fixed ports and being slow, so apps run-local has had no coverage on PRs since. Both problems come from the fixture rather than the command: the ports were hardcoded, and the test asked for --prepare-environment, which installs the seventeen default app libraries via uv. That install can no longer work at all, because acceptance tests now run with UV_OFFLINE and only a handful of vendored wheels. Ports are now allocated per run from a new acceptance/bin/free_port.py, and the app is a standard-library HTTP server started directly instead of Flask under a freshly built venv, so the run needs no package index. The test covers the same ground otherwise: valueFrom rejection, --entry-point, app.yml env vars, and the proxy's identity headers. The --debug variant is dropped because debugpy is only reachable from PyPI. The debugpy invocation it exercised is asserted in libs/apps/runlocal unit tests.
The fixture answered /shutdown and then called os._exit(0) from inside the handler, so the response reached the client only because the kernel drained the socket buffer on exit. An abortive close would surface as a curl failure, and the script no longer tolerates one: unlike the version this test was modelled on, it has neither "|| true" on the shutdown request nor on the final wait. Stopping the serve loop from a second thread closes the connection cleanly and lets the process exit on its own, which also makes "wait $PID" assert that the CLI exited zero. The two ports now go through add_repl.py, so the output tells the proxy port and the app port apart instead of rendering both as $(port); that makes the generic 127.0.0.1 replacement dead, so it goes. With a stable placeholder for the proxy port the test can assert the "To access your app go to" line, which the previous version only waited for. Both run-local invocations are traced so each LOG file records the command that produced it.
The trap killed $PID, which is not the CLI: backgrounding "trace" forks a subshell, so the CLI and the Python app it starts are grandchildren and survive the script. A failure anywhere after the app comes up left both processes running, reparented to init, holding the two ports and an open handle on LOG.run. Windows reaches the same state by another route, because kill terminates the CLI outright instead of giving it the chance to stop the app, and there a leaked handle also blocks the temp directory cleanup. Cleanup now falls back to kill_port.py, which is what this script used before and what run-local-node still does. It runs only when the script failed: on success both processes have exited through /shutdown already, and the ports are free and may have been handed to another test by the time the trap fires.
The trap explained that $PID is a subshell whose CLI and app survive being killed, then killed $PID anyway. Reaping by port already covers both, so the kill goes and the comment says why the port is the handle we have. Also reach the server through self.server in the fixture rather than the module global declared below the handler.
The test has been disabled since #4875, which took NPM registry access away from the runners. It needed the registry only for the app fixture: express, and the --prepare-environment run that installs it. The fixture is now a standard-library HTTP server started directly with node, so nothing is installed at test time. /shutdown answers with Connection: close so server.close() can complete and the app exits on its own, which lets wait $PID assert that the CLI exited zero. Ports come from free_port.py, a failed run reaps the app and the CLI by port, and the Timeout overrides go away because the run takes well under a second rather than 2m. --debug stays, unlike in the python sibling where it needs debugpy from PyPI: node debugging is only NODE_OPTIONS=--inspect. The test asserts both the port the CLI reports and that node listens on it, so --debug-port keeps its coverage. npm is out of the fixture too. The rejecting proxy catches npm run contacting registry.npmjs.org for its update notifier, which npm skips when CI is set, so keeping npm would have failed locally only and needed a config workaround.
Collaborator
Integration test reportCommit: 13a315f
22 interesting tests: 14 flaky, 4 RECOVERED, 4 SKIP
Top 33 slowest tests (at least 2 minutes):
|
radakam
force-pushed
the
acc-run-apps-run-local-locally
branch
from
August 5, 2026 16:53
7da0161 to
e1cf319
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #6169.
Changes
Re-enable
acceptance/cmd/workspace/apps/run-local-node, disabled since #4875 for needing NPM registry access. The app fixture is now anode:httpserver started directly withnode, so nothing is installed at test time andexpress,--prepare-environmentand the 2m/10m timeouts are gone. Ports come fromfree_port.py,/shutdownends the connection so the app exits on its own andwait $PIDcan assert the CLI's exit code, and a failing run reaps both by port.--debugstays: node debugging is onlyNODE_OPTIONS=--inspect, so the test asserts the port the CLI reports and that node listens on it — the--debug-portcoverage the python sibling drops.Why
Only the fixture ever needed the registry. npm is out of it too: the rejecting proxy catches
npm runcontactingregistry.npmjs.orgfor its update notifier, which npm skips whenCIis set, so keeping npm would have failed locally only.