-
Notifications
You must be signed in to change notification settings - Fork 15
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: issue-75 - test entity-bytes with missing blocks #85
Conversation
6502815
to
e39c5d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @laurentsenta, this is exactly what we needed!
(pushed some minor tweaks to Name and Hint + some comments inline fyi, but ok to merge as-is – we need this and to unblock filecoin-saturn/L1-node#447)
tests/trustless_gateway_car_test.go
Outdated
{ | ||
Name: "GET CAR with entity-bytes when missing a block will timeout", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@laurentsenta (just FYI) I've removed this one, as its body was the same as the first one we had – assuming it was just left-over of some copy-paste.
I did not fix it to actually test timeout, because:
- we don't want to introduce unnecessary latency to the test suite
- the duration after which returning HTTP 504 is not currently specified, so we can't test conformance anyway
- two existing tests are more than enough - especially
entity-bytes=2200:*
one – this alone will fail if implementation is broken the way I described in Add backend timeout test for entity-bytes from IPIP-402 #75
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update and removing that useless test 🙏
I created an issue to add timeouts #89
assert.Len(t, cids, 3) | ||
assert.Equal(t, "QmZgfvZtoFdbJy4JmpPHc1NCXyA7Snim2L8e6zKspiUzhu", cids[0]) | ||
assert.Equal(t, "QmaATBg1yioWhYHhoA8XSUqD1Ya91KKCibWVD4USQXwaVZ", cids[1]) | ||
assert.Equal(t, "QmdQEnYhrhgFKPCq5eKc7xb1k7rKyb3fGMitUPKvFAscVK", cids[2]) | ||
} | ||
|
||
func TestMustGetChildrenDespiteMissingBlocks(t *testing.T) { | ||
f := MustOpenUnixfsCar("./_fixtures/file-3k-and-3-blocks-missing-block.car") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 excellent call to have a separate fixture for testing tooling, independent of fixture used in actual gateway test 👌
Path("/ipfs/{{cid}}", missingBlockFixture.MustGetCidWithCodec(0x70)). | ||
Query("format", "car"). | ||
Query("dag-scope", "entity"). | ||
Query("entity-bytes", "2200:*"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ I had gut feeling to check what we send on the wire – we may need to add QueryRaw
here, but not in scope of this PR, not a blocker – filled separate #88
Closes #75
MustGetChildren
->MustGetDescendants
MustListChildren
README.md
with a recipe for the fixtures.