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: finish porting and cleaning all simple tests #92

Merged
merged 24 commits into from
Jun 30, 2023

Conversation

laurentsenta
Copy link
Contributor

@laurentsenta laurentsenta commented Jun 26, 2023

Closes #26

Notes

  • I am reviewing all the tests and removing shell comments, which will mark the end of "porting sharness tests".
  • Sister PR in Kubo: I reviewed every tests and removed the code that was duplicated. We don't migrate kubo-specific code, like testing /api/, behavior with configs, etc.

@laurentsenta laurentsenta marked this pull request as ready for review June 28, 2023 09:26
Copy link
Contributor

@galargh galargh left a comment

Choose a reason for hiding this comment

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

Nice job! I think we should also prepare an accompanying PR to kubo that removes ported tests from sharness there. (ipfs/kubo#9999)

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.

Thank you @laurentsenta for driving this over the finish line ❤️

I did a pass over this and dropped some questions/comments/nits to address, but other than that lgtm.

As for Kubo PR, it is bit more tricky, left a review there, butI don't think that PR should block this one.

fixtures/README.md Outdated Show resolved Hide resolved
fixtures/dir_listing/README.md Outdated Show resolved Hide resolved
fixtures/ipns_records/README.md Outdated Show resolved Hide resolved
fixtures/path_gateway_dag/README.md Outdated Show resolved Hide resolved
fixtures/path_gateway_dag/plain-that-can-be-dag.cbor.car Outdated Show resolved Hide resolved
tests/path_gateway_unixfs_test.go Outdated Show resolved Hide resolved
tests/redirects_file_test.go Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

"dag" name is confusing because unixfs is a DAG too.

These are CBOR and JSON tests, and dag- variants are part of the story.
Maybe rename fixtures/path_gateway_dagfixtures/path_gateway_cbor_json for clarity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Related to my other comments about naming and specs (#92 (comment)) I'd keep this in a different PR if that's OK with you

Copy link
Member

Choose a reason for hiding this comment

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

nit _dag_ in filename is confusing – CARs and UnixFS also have DAGs.
These are CBOR and JSON tests, and dag- variants are only part of the story.

Maybe rename tests/path_gateway_dag_test.gotests/path_gateway_cbor_json_test.go for clarity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We aligned these file names with the spec changes in #79

Spec defs:

PathGatewayDAG = Leaf{"path-dag-gateway", stable}

I'm worried if we change that filename here, we'll create inconsistencies with the rest of the code, I created #100 to capture your comments.

tests/path_gateway_dag_test.go Show resolved Hide resolved
Copy link
Member

@hacdias hacdias left a comment

Choose a reason for hiding this comment

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

🥳

I think this one is mostly fine, as long as @lidel's comments are addressed. Same in Kubo land: ipfs/kubo#9999 - I think they can merged independently and I would like them to be merged as soon as possible to avoid future conflicts.

@laurentsenta laurentsenta merged commit 84488da into main Jun 30, 2023
@github-actions github-actions bot mentioned this pull request Jul 31, 2023
@BigLep BigLep mentioned this pull request Aug 3, 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.

Port all "simple" sharness tests
4 participants