feat: tests for IPIP-402 CAR parameters by aschmahmann · Pull Request #56 · ipfs/gateway-conformance

aschmahmann · 2023-05-19T15:30:01Z

Conformance tests for IPIP-402.

Superseded and includes Add dag-scope tests #49
IPIP: IPIP-402: Partial CAR Support on Trustless Gateways specs#402
Boxo PR: feat(gateway): implement IPIP-402 extensions for gateway CAR requests boxo#303
Kubo PR: feat(gateway): support for IPIP-402 CAR params kubo#9914
CI changes must be reverted before merging: @hacdias updated this PR to run against the Kubo PR

Note: the last commits for cloning the requestbuilder and splitting out the repeatable tasks (e.g. format parameter + accept header testing) could use separate review on if they're a good idea/pattern.

tests/t0118_gateway_car_test.go

laurentsenta

really cool, thanks for the PR,

A note on moving file, and fixes, the Master test should pass, it's broken because of templating right now.

laurentsenta · 2023-05-24T07:32:18Z

Thanks for the update, I can see it's failing because of a few errors: https://github.com/ipfs/gateway-conformance/actions/runs/5060325462?pr=56#summary-13700442847

Error: Body length mismatch: 2 != 3

Error: Header 'Content-Disposition' expect to find substring 'attachment; filename="bafybeiefu3d7oytdumk5v7gn6s7whpornueaw7m7u46v2o6omsqcrhhkzi...

Error: Header 'Content-Length' expected empty string, got '221' (CAR is streamed, gateway may not have the entire thing, unable to calculate total size)

The issue with Content-Length is a known bug with Kubo ipfs/kubo#9651 we have a workaround, add the tests in

gateway-conformance/.github/workflows/test.yml

Line 65 in c32f74d

    
           args: -skip 'TestGatewayCar/GET_response_for_application/vnd.ipld.car/Header_Content-Length'

Not sure about the two others, is this a new bug?

hacdias · 2023-06-01T12:54:58Z

@aschmahmann @lidel @laurentsenta:

I closed Add dag-scope tests #49 in favour of this one. Commits from Add dag-scope tests #49 are included here.
I updated this PR to run against Kubo's PR: feat(gateway): support for IPIP-402 CAR params kubo#9914. We should revert those ci tests before merging.
I commented the Content-Length check for now. Since @aschmahmann reworked it to be applied to all CAR tests, I don't think it is possible to ignore that little check via the command line.
~~There are failing tests with likely problems on both sides (Boxo and Tests).~~

hacdias · 2023-06-02T14:42:30Z

@lidel @aschmahmann tests are passing against ipip-402 branch in Kubo, which uses the latest stuff in Boxo. I think we have a very good test coverage here.

Problem identified in ipfs/gateway-conformance#56 (comment)

tests/t0118_gateway_car_test.go

lidel · 2023-06-07T12:43:23Z

tests/t0118_gateway_car_test.go

+				Status(200).
+				Body(
+					IsCar().
+						HasRoot(subdirTwoSingleBlockFilesFixture.MustGetCid()).


@hacdias @aschmahmann based on feedback from Rod, I've relaxed requirement to allow no-buffering pass-through: ipfs/specs#402 (comment)
Means we need to update conformance tests and expect either terminus CID, or empty roots.

Unsure which one will be easier to do in ipfs/boxo#303 – I would prefer terminus, as that keeps behavior from Kubo 0.20, but fine with either as long we document in changelog.

lidel

@aschmahmann @hacdias fyi I've filled #75 to unblock landing this sooner than later.

iiuc the remaining asks are:

update boxo code and assets for value in CarV1Header.roots, as noted in ipfs/specs#402 (comment)
add regression test for MustGetCIDsInHAMT* helpers (or fill issue for future PR)

I'm out Thu-Fri, but if above two are handled, lgtm to merge and ship.

tooling/car/unixfs.go

tests/t0118_gateway_car_test.go

hacdias · 2023-06-08T09:03:54Z

This is blocked. Tests work well if the gateway we're testing against sends roots. If not, car.NewReader will scream at you saying there's no roots: https://github.com/ipfs/boxo/blob/aa12ba3e27965674d3e7cef7fefdccfa8d978d9c/ipld/car/car.go#L137-L139 (same in ipld/go-car, which I would prefer to use here).

The CAR specification also mentions the CAR requires 1 or more roots: https://ipld.io/specs/transport/car/carv1/#constraints

We have to reach a consensus on what to do about the CAR roots, then update the libraries if we decide 0 roots is possible. Release those libraries and then update here. See also Slack thread.

Co-authored-by: Eric Guan <guanzo91@gmail.com> Co-authored-by: Adin Schmahmann <adin.schmahmann@gmail.com>

hacdias · 2023-06-08T15:31:01Z

From standup: we decided to merge implementation in Boxo and Kubo, then this.

The decision on the CAR roots is deferred and both Boxo and conformance tests can be later updated.

⚠️ This tests currently check for 1 root (last segment) or no roots. The test logic is correct according tot he current IPIP-402 state. However, go-car does not support zero roots and therefore the tests will fail. Let's continue ipfs/specs#402 (comment).

done

Problem identified in ipfs/gateway-conformance#56 (comment)

aschmahmann force-pushed the feat/gateway-ipip-402 branch from d544f28 to 5232b33 Compare May 22, 2023 15:36