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: CAR order and dups parameters from IPIP-412 #87

Merged
merged 16 commits into from
Jul 26, 2023
Merged

feat: CAR order and dups parameters from IPIP-412 #87

merged 16 commits into from
Jul 26, 2023

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Jun 23, 2023

This PR creates tests for IPIP-412. Note the following:

  • ⚠️ This PR is running against ipip-412 branch in Kubo. Disable this before merging.
  • Tests for duplicates use a fixture where a directory contains two files that are the same. If dups=n, then there are no duplicates. If dups=y, then the blocks of the file are sent twice, by the order they show up in the DAG. This is tested with order=dfs.
  • The test above also tests order=dfs.
  • I added a test for order=unk where I simply check if all the CIDs are present, but not exactly since the order can be unknown. This test also doesn't check for the order in the response content type because the gateway can answer with any order they want.

@hacdias hacdias marked this pull request as ready for review June 23, 2023 09:58
@laurentsenta laurentsenta mentioned this pull request Jun 26, 2023
@lidel lidel mentioned this pull request Jul 5, 2023
@lidel lidel changed the title feat: order and dups parameters CAR tests feat: CAR order and dups parameters from IPIP-412 Jul 14, 2023
tests/trustless_gateway_car_test.go Outdated Show resolved Hide resolved
tests/trustless_gateway_car_test.go Show resolved Hide resolved
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.

lgtm, thank you @hacdias for adding remaining tests

@laurentsenta mind taking a look if it is ok to merge & release?
We want Rhea/Saturn to be able to run these in their CI.

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.

lgtm, let's not hardcode CIDs in the code without leaving notes to future maintainers!

@hacdias hacdias merged commit 150a3df into main Jul 26, 2023
19 of 20 checks passed
@hacdias hacdias deleted the ipip-412 branch July 26, 2023 08:33
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.

3 participants