test(e2e): verify remote-repo cached artifacts show in the browser#461
test(e2e): verify remote-repo cached artifacts show in the browser#461brandonrc wants to merge 1 commit into
Conversation
) Adds a Playwright spec covering the v1.2.0 regression where packages pulled through a remote (proxy) repository filled up storage but never appeared in the repo browser, so they couldn't be browsed or scanned. The spec pulls a small package through the seeded `e2e-npm-remote` proxy, then asserts the cached entry appears both in the listing API (GET /api/v1/repositories/{key}/artifacts) and in the Artifacts tab of the repo detail page. It skips gracefully if the upstream registry is unavailable, since upstream availability is not what's under test. The fix is backend-only (artifact-keeper#1567 / #1548): the listing for remote repos is now reconstructed from the proxy cache. The web client already consumed the corrected data through the same endpoint, so no web code change is required. Closes #424
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
brandonrc
left a comment
There was a problem hiding this comment.
Test/E2E quality review (v1.2.0 batch). Verdict: the assertions ARE good when they run, but both tests sit behind an upstream-availability skip that will frequently swallow the whole fix in CI. This is the one I am most skeptical of.
The skip problem. Both tests call pullThroughProxy() and, if !resp.ok(), do test.skip(true, 'Upstream NPM registry unavailable...'). The bug under test (#424: remote-cached items vanished from the listing) can ONLY manifest after a successful pull-through. So whenever registry.npmjs.org is unreachable, slow, or rate-limited from the CI runner, both tests go green having asserted nothing about the fix. For a release-gate e2e that depends on a third-party registry, this is a real "perpetually green without testing" risk, not a theoretical one. The comment frames upstream availability as out of scope, but the practical effect is that the fix's only e2e coverage is conditional on a flaky external dependency.
When it does run, the assertions are honest. The listing-API test asserts items.length > 0 (the regression was an empty listing), finds the package by path/name, and asserts size_bytes > 0 and checksum_sha256 truthy, proving the reconstructed-from-sidecar metadata is real, not stubbed. The UI test asserts a row containing the package is visible in the Artifacts tab. These genuinely distinguish fixed-vs-broken.
Recommendation. Reduce the external dependency: either (a) seed the proxy cache deterministically (PUT a sidecar+content pair into proxy-cache/<key>/... via storage, or a fixture upstream served by the test harness) so the listing assertion always runs, or (b) point the remote at a local stub registry. Pair this with the backend gap I flagged on #1567 (no unit test exercises list_cached_artifacts against a storage backend) — right now, between the two PRs, there is a plausible CI run where the entire #424 fix is asserted by nothing. Strongly suggest fixing the determinism before relying on this as release-gate coverage. Not formally requesting changes, but this is the weakest spec in the batch.
|
Shipped in v1.2.0. The changes from this PR were folded into the release integration branch #466, which was squash-merged to main and tagged as v1.2.0. The code is already on main, so closing this as superseded by #466 rather than merging it (re-applying would just duplicate what already shipped). No work lost. See #466 for the consolidated record. |
Summary
Fixes the v1.2.0 regression reported in #424: when using remote (proxy) repositories, artifacts pulled through stopped appearing in the repo browser. Storage filled up (bytes were written) but the UI listed 0 artifacts, so users could not browse or scan them.
Root cause is in the backend. #1278 / #1280 stopped recording proxy-cached items in the
artifactstable to fix a filesystem doubled-prefix storage path bug. The acknowledged tradeoff was that remote-cached items no longer surfaced inGET /api/v1/repositories/{key}/artifacts, which is exactly what the repo browser lists from. The backend fix (artifact-keeper#1567, tracking artifact-keeper#1548) reconstructs the listing for remote repos from the proxy cache, preserving the #1278 fix.The web client already consumes the corrected data through the same endpoint, so no web code change is required. This PR adds the Playwright regression test for the user-visible flow:
is-odd) through the seedede2e-npm-remoteproxy.The spec skips gracefully if the upstream registry is unavailable (upstream availability is not what's under test). It runs in CI against the 1.2.0 backend image.
Test Checklist
UI Changes
This is a test-only change; the behavior fix lands in the backend (artifact-keeper#1567).
Closes #424