Revanth14 commented on PR #1505: URL: https://github.com/apache/iceberg-go/pull/1505#issuecomment-5289483733
> Really nice suite. The `//go:build integration` gating keeps this out of the default build, the capture transport that lets you assert the decoded response and the raw Java wire in a single pass is a clean design, and keeping the wire types independent from the production structs — so both sides can't quietly agree on a wrong encoding — is well-justified in the comment. The typed-partition and hex-bounds assertions are exactly the cross-client contracts worth pinning. > > I'd hold on one thing before merge, and it isn't the test logic — it's blast radius. Moving the shared `rest` service to an unreleased digest-pinned image and adding `RUN_INTEGRATION_TESTS=1` to `integration-rest` means every existing REST integration test now runs against a pre-release image on the main CI target. If that image regresses on anything unrelated to scan planning, it shows up as a confusing failure somewhere else entirely, and a `:latest@sha256` pin with no release tag keeping it alive can get GC'd out from under CI. I'd isolate the scan-planning tests onto their own service/target and leave the shared `rest` path on the released tag. > > The rest is test-robustness polish: a couple of non-fatal assertions that report the wrong cause on failure, one unguarded type assertion that could panic the whole binary, and a helper that breaks the method convention of the file. Details inline. > > A few things I'd want before merge: > > * isolate the scan-planning image/target from the shared REST integration suite (or a tracking issue + TODO to move to the release tag once 1.11.0 ships) > * `Require().ElementsMatch` before the bound-value lookups so a field-ID mismatch doesn't masquerade as a hex-encoding failure > * guard or replace the `http.DefaultTransport` type assertion so a swapped transport can't panic the binary > > Once those are addressed, happy to take another pass and approve. Thanks for the thorough review. I’ve addressed all the requested changes, including isolating the scan-planning fixture/target and tightening the test assertions and transport setup. Please take a look when you got some time @laskoviymishka -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
