Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: tests for IPIP-402 CAR parameters #56

Merged
merged 4 commits into from
Jun 8, 2023
Merged

Conversation

aschmahmann
Copy link
Contributor

@aschmahmann aschmahmann commented May 19, 2023

Conformance tests for IPIP-402.


cc @guanzo @laurentsenta

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.

Copy link
Contributor

@laurentsenta laurentsenta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

laurentsenta commented May 24, 2023

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

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
Copy link
Member

hacdias commented Jun 1, 2023

@aschmahmann @lidel @laurentsenta:

@hacdias hacdias force-pushed the feat/gateway-ipip-402 branch 4 times, most recently from 4a8b57a to 619ba13 Compare June 2, 2023 14:22
@hacdias hacdias changed the title feat: gateway ipip 402 feat: add tests for IPIP-402 CAR parameters Jun 2, 2023
@hacdias hacdias changed the title feat: add tests for IPIP-402 CAR parameters feat: tests for IPIP-402 CAR parameters Jun 2, 2023
@hacdias hacdias requested a review from laurentsenta June 2, 2023 14:39
@hacdias
Copy link
Member

hacdias commented Jun 2, 2023

@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.

@hacdias hacdias mentioned this pull request Jun 5, 2023
tests/t0118_gateway_car_test.go Show resolved Hide resolved
tests/t0118_gateway_car_test.go Outdated Show resolved Hide resolved
tests/t0118_gateway_car_test.go Outdated Show resolved Hide resolved
tests/t0118_gateway_car_test.go Outdated Show resolved Hide resolved
tests/t0118_gateway_car_test.go Outdated Show resolved Hide resolved
Status(200).
Body(
IsCar().
HasRoot(subdirTwoSingleBlockFilesFixture.MustGetCid()).
Copy link
Member

@lidel lidel Jun 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
lidel previously requested changes Jun 7, 2023
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

iiuc the remaining asks are:

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

tooling/car/unixfs.go Outdated Show resolved Hide resolved
tooling/car/unixfs.go Show resolved Hide resolved
tests/t0118_gateway_car_test.go Show resolved Hide resolved
@hacdias
Copy link
Member

hacdias commented Jun 8, 2023

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.

@hacdias hacdias force-pushed the feat/gateway-ipip-402 branch 7 times, most recently from 79ea524 to 8d7b65f Compare June 8, 2023 15:27
hacdias and others added 3 commits June 8, 2023 17:28
Co-authored-by: Eric Guan <guanzo91@gmail.com>
Co-authored-by: Adin Schmahmann <adin.schmahmann@gmail.com>
@hacdias
Copy link
Member

hacdias commented Jun 8, 2023

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).

@hacdias hacdias merged commit 37838fe into main Jun 8, 2023
@hacdias hacdias deleted the feat/gateway-ipip-402 branch June 8, 2023 15:52
lidel added a commit to ipfs/specs that referenced this pull request Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants