Skip to content

Add dag-scope tests#49

Closed
guanzo wants to merge 2 commits intoipfs:mainfrom
guanzo:main
Closed

Add dag-scope tests#49
guanzo wants to merge 2 commits intoipfs:mainfrom
guanzo:main

Conversation

@guanzo
Copy link
Copy Markdown
Contributor

@guanzo guanzo commented May 15, 2023 •
edited
Loading

TODO:

  • Add a fixture that supports comprehensive car-scope tests.
  • Rename car-scope to dag-scope once lassie supports it.

How I generated the fixture

#!/bin/bash

# set kubo config "Internal.UnixFSShardingSizeThreshold": "10B"

mkdir -p dir/sub1
echo "abc123" > dir/sub1/hello.txt
mkdir -p dir/sub2
echo "xyz456" > dir/sub2/hello.txt

ROOT_CID=$(ipfs add -Qr --chunker=size-5 dir)

ipfs dag export ${ROOT_CID} > ./t0118-ipip-402-scope.car

The point of 2 subdirectories is that if the request is for one of them (/sub1/hello.txt), than the other (/sub2) should not appear in the CAR.

@aschmahmann aschmahmann mentioned this pull request May 16, 2023
Merged
4 tasks
laurentsenta
laurentsenta reviewed May 18, 2023
View reviewed changes
@aschmahmann aschmahmann mentioned this pull request May 19, 2023
Merged
5 tasks
@guanzo guanzo marked this pull request as ready for review May 26, 2023 22:28
@guanzo guanzo changed the title [WIP] add car-scope tests Add dag-scope tests May 27, 2023
@laurentsenta
Copy link
Copy Markdown
Contributor

laurentsenta commented May 29, 2023

Adding related contributors for review, the code looks fine to me;
Could you rebase over the latest main? It looks like it shows the car tooling changes that are already in main.

@guanzo
Copy link
Copy Markdown
Contributor Author

guanzo commented May 29, 2023

Had trouble doing a clean rebase so force pushed instead 😅

@galargh galargh mentioned this pull request May 30, 2023
Merged
@galargh
Copy link
Copy Markdown
Contributor

galargh commented May 30, 2023

The tests haven't been running on this PR because we had a wrong trigger configured for the job. This is being fixed in #60

In the meantime, I pushed https://github.com/ipfs/gateway-conformance/tree/guanzo-main so that we get CI runs for the HEAD of this PR. Unfortunately, it looks like the newly added tests aren't passing when running against kubo-master. See https://github.com/ipfs/gateway-conformance/actions/runs/5119205028/attempts/1#summary-13852714693

@guanzo
Copy link
Copy Markdown
Contributor Author

guanzo commented May 30, 2023 •
edited
Loading

The failures are due to kubo not "returning intermediate blocks from the CID root to the path terminus as part of the returned CAR file" as mentioned in this PR. ipfs/specs#402. Not sure how we want to proceed as idk when kubo will implement that.

@galargh
Copy link
Copy Markdown
Contributor

galargh commented May 31, 2023

We can skip the some expects like this:

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

Line 65 in 2648070

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

guanzo added 2 commits May 31, 2023 20:17
@guanzo
add car-scope tests
a3437dd
@guanzo
* rename car-scope to dag-scope
29372c1 
* add test for missing dag-scope parameter
@guanzo
Copy link
Copy Markdown
Contributor Author

guanzo commented May 31, 2023 •
edited
Loading

I can skip the CAR cid checks but that basically skips all tests added by this PR 😆 . We can do that or skip the intermediate cid checks for now.

@hacdias
Copy link
Copy Markdown
Member

hacdias commented Jun 1, 2023

I'm closing this and we'll continue on #56. Thanks @guanzo for your contribution on this. Changes are included in #56.

@hacdias hacdias closed this Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lidel lidel Awaiting requested review from lidel

@aschmahmann aschmahmann Awaiting requested review from aschmahmann

@galargh galargh Awaiting requested review from galargh

@hacdias hacdias Awaiting requested review from hacdias

1 more reviewer

@laurentsenta laurentsenta laurentsenta approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@guanzo @laurentsenta @galargh @hacdias