richiebachala commented on PR #2376:
URL: https://github.com/apache/iceberg-go/pull/2376#issuecomment-6561945227
Thanks for the review. Both inline points are addressed in c19f2bc:
- `wire.go`: applied your comment as written.
- `TestSpecErrorResponses`: a raw-HTTP table that asserts status, error
`type`, and that `error.code` matches the status. It covers your three rows
plus missing namespace, existing namespace or table, rename to a missing
namespace (the status-gated `NoSuchNamespaceException` case), commit to a
missing table without `assert-create`, and a malformed body. I checked that it
catches what the client-level tests can't: changing the non-empty drop's type
to `AlreadyExistsException` fails this test, while `TestConformance` still
passes.
On the bigger picture, here's what I'd propose:
- **What uses it.** I wouldn't move the existing `catalog/rest` unit tests
onto `resttest`. `rest_test.go` and similar files pin exact wire payloads,
headers and error-type fallbacks, such as
`TestRenameTable404UntypedFallsBackToNoSuchTable`, and canned responses are the
right tool for that. `resttest` is for things that need real state across calls:
- the REST `catalogtest` run in plain `go test` (this PR);
- cross-catalog contract cases;
- client flows such as commit conflict and retry, staged create then
commit, and rename then load;
- downstream projects that want a REST catalog in tests.
- **What stays on Docker.** Everything in `go-integration.yml`:
- the Spark 3.5 and 4.0 interop runs against the fixture and MinIO;
- the Polaris external-REST run.
Those check behaviour against real servers and object storage, which an
in-memory server can't stand in for.
- **Order after this PR:**
1. `catalogtest` cases for the #2100 contract: a terminal requirement
doesn't match `ErrCommitFailed`, and a stale append lands with retries on.
These would run here and on every other catalog.
2. Views lifecycle, with matching `catalogtest` view cases.
3. `transactions/commit`.
4. Fault-injection options, such as failing the next commit with a 409 or
503, to cover retry and unknown-state paths.
5. Pagination.
- **`planfake`.** I'd keep it separate. It replays scripted responses for
exact scan-planning sequences, polling and cancellation, while `resttest` keeps
catalog state. If remote planning ever needs real table state, `resttest` could
grow a planning hook, but I wouldn't merge them up front.
Happy to put this on #2044 instead if that's a better home.
@slachiewicz, thanks for the review. I'll address your points in the next
push and reply in that thread.
--
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]
