test: add package installation integration tests#1052
Conversation
…xy image The Dockerfile only copied server.js but server.js now requires logging.js, metrics.js, and rate-limiter.js. Without these files the container exits immediately on startup, causing all agentic workflow CI jobs to fail. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add a "Package Installation" describe block to chroot-package-managers tests that verifies actual package installation through the AWF proxy: - pip install requests with pypi.org + files.pythonhosted.org allowed - npm install chalk@4 with registry.npmjs.org allowed - cargo build with a dependency (cfg-if) with crates.io domains allowed Each test verifies the package was actually installed (import/require/build). Closes #1044 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (1 files)
Coverage comparison generated by |
There was a problem hiding this comment.
Pull request overview
Adds integration coverage for real package installation/build workflows in chroot mode (via AWF’s domain allowlisting), intended to validate the most common “download + install” paths rather than only registry queries.
Changes:
- Add a new “Package Installation” describe block to
tests/integration/chroot-package-managers.test.tscovering pip, npm, and cargo installs/builds. - Update
containers/api-proxy/Dockerfileto copy additional JS modules alongsideserver.js.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tests/integration/chroot-package-managers.test.ts | Adds new integration tests that perform actual installs/builds through the proxy and verify the results. |
| containers/api-proxy/Dockerfile | Modifies the image build to copy additional JS files into the api-proxy image. |
Comments suppressed due to low confidence (2)
tests/integration/chroot-package-managers.test.ts:394
- Temporary directory cleanup only happens if every prior command succeeds (
... && rm -rf $TESTDIR). Ifcargo init/cargo buildfails, the directory is leaked. Use an EXIT capture / trap pattern (as done elsewhere in this file) so cleanup happens on both success and failure.
'TESTDIR=$(mktemp -d) && cd $TESTDIR && ' +
'cargo init --name awftest 2>&1 && ' +
// Add a small dependency (cfg-if is tiny with no transitive deps)
'echo \'cfg-if = "1"\' >> Cargo.toml && ' +
'cargo build 2>&1 && echo "cargo_build_ok" && ' +
'rm -rf $TESTDIR',
tests/integration/chroot-package-managers.test.ts:358
- This test writes into a fixed path (/tmp/pip-test) and never cleans it up. That can cause cross-run contamination (already-installed packages) and disk bloat on CI runners. Use a per-test mktemp directory (and remove it afterward) instead of a shared /tmp path, and consider disabling pip cache if you want to ensure a real download occurs.
const result = await runner.runWithSudo(
'pip3 install --target /tmp/pip-test requests 2>&1 && ' +
'PYTHONPATH=/tmp/pip-test python3 -c "import requests; print(requests.__version__)"',
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const result = await runner.runWithSudo( | ||
| 'cd /tmp && mkdir -p npm-test && cd npm-test && npm init -y 2>&1 && ' + | ||
| 'npm install chalk@4 2>&1 && ' + |
There was a problem hiding this comment.
This test creates /tmp/npm-test and leaves it behind. That can make later runs faster/flakier (install may not hit the network) and can grow disk usage over time. Prefer a unique mktemp directory (and ensure it’s removed even if npm/node fails).
This issue also appears in the following locations of the same file:
- line 389
- line 356
| const result = await runner.runWithSudo( | ||
| 'cd /tmp && mkdir -p npm-test && cd npm-test && npm init -y 2>&1 && ' + | ||
| 'npm install chalk@4 2>&1 && ' + | ||
| 'node -e "require(\'/tmp/npm-test/node_modules/chalk\')" && echo "npm_install_ok"', |
There was a problem hiding this comment.
PR description/issue acceptance criteria mention verifying require('chalk') works, but this uses an absolute path require into /tmp/npm-test/node_modules. Running the require via Node’s module resolution from within the project dir (or via NODE_PATH) would better match the stated behavior being tested.
| { | ||
| allowDomains: ['crates.io', 'static.crates.io', 'index.crates.io'], | ||
| logLevel: 'debug', |
There was a problem hiding this comment.
This relies on Cargo using the sparse index (index.crates.io). On environments where Cargo still uses the git index, it will attempt to reach GitHub and this test will fail under the current allowDomains. To make the test deterministic, explicitly force sparse protocol (e.g., via CARGO_REGISTRIES_CRATES_IO_PROTOCOL=sparse) or expand the domain allowlist to cover the git index host your Cargo version uses.
containers/api-proxy/Dockerfile
Outdated
|
|
||
| # Copy application files | ||
| COPY server.js ./ | ||
| COPY server.js logging.js metrics.js rate-limiter.js ./ |
There was a problem hiding this comment.
Docker build will fail: this COPY lists logging.js/metrics.js/rate-limiter.js but those files don’t exist in containers/api-proxy/ (only server.js, package*.json, README). Either add these files to the image context or revert the COPY to only include server.js (or copy the whole directory).
| COPY server.js logging.js metrics.js rate-limiter.js ./ | |
| COPY server.js ./ |
|
|
||
| expect(result).toSucceed(); | ||
| // The last line of output should be the version string | ||
| expect(result.stdout).toMatch(/\d+\.\d+\.\d+/); |
There was a problem hiding this comment.
The comment says the last line should be the version string, but the assertion only checks that somewhere in stdout there is an x.y.z pattern (which could match pip’s own output). Consider asserting against the last non-empty line (or anchoring the regex to the end) so the check matches the stated intent.
| expect(result.stdout).toMatch(/\d+\.\d+\.\d+/); | |
| const lines = result.stdout.split('\n').map(line => line.trim()).filter(line => line.length > 0); | |
| const lastLine = lines[lines.length - 1] || ''; | |
| expect(lastLine).toMatch(/^\d+\.\d+\.\d+$/); |
- Use mktemp dirs instead of fixed /tmp paths (avoid cross-run contamination) - Add --no-cache-dir to pip install (ensure real download) - Assert pip version on last line specifically (not just anywhere in stdout) - Use NODE_PATH for npm require instead of absolute path - Force CARGO_REGISTRIES_CRATES_IO_PROTOCOL=sparse for deterministic behavior - Ensure temp dir cleanup via trap pattern (EC=$?; rm -rf; exit $EC) - Revert unrelated api-proxy Dockerfile change Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Addressed all review feedback in 1c0791d:
|
Smoke Test Results
Overall: PASS
|
Build Test: C++ Results
Overall: PASS ✅
|
🦀 Rust Build Test Results
Overall: PASS ✅
|
Deno Build Test Results
Overall: ✅ PASS Deno version:
|
|
Smoke test results for ✅ GitHub MCP — Last 2 merged PRs: #1036 "docs: add integration test coverage guide with gap analysis", #1035 "feat: group --help flags by category, hide dev-only options" Overall: PASS
|
Node.js Build Test Results
Overall: ✅ PASS
|
Go Build Test Results
Overall: ✅ PASS
|
|
Merged PRs: docs: add integration test coverage guide with gap analysis; feat: group --help flags by category, hide dev-only options
|
🧪 Build Test: Bun Results
Overall: ✅ PASS Bun version:
|
Java Build Test Results
Overall: ✅ PASS Both projects compiled and all tests passed successfully against PR #1052.
|
Summary
chroot-package-managers.test.tsthat verifies actual package installation through the AWF proxyCloses #1044
Test plan
requestsand verifiesimport requestsworkschalk@4and verifiesrequire('chalk')workscfg-ifdependency🤖 Generated with Claude Code