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

chore: remove sharness tests ported to conformance testing #9999

Merged
merged 19 commits into from
Aug 2, 2023

Conversation

laurentsenta
Copy link
Contributor

@laurentsenta laurentsenta commented Jun 27, 2023

Sister PR: ipfs/gateway-conformance#92

To complete the first iteration of conformance testing, I am going through all the tests, double-checking what was ported, and removing duplicate tests.

Tasks

  • update conformance with the changes in 3da4e5b

@laurentsenta laurentsenta requested review from lidel and a team as code owners June 27, 2023 15:57
@laurentsenta laurentsenta marked this pull request as draft June 27, 2023 15:57
@laurentsenta
Copy link
Contributor Author

laurentsenta commented Jun 27, 2023

@lidel @hacdias I created this PR to review what was ported to conformance testing (tests related to Kubo's API or configurations won't be ported for example) and eventually remove duplicate code.

There are around 300 test cases over 13 files involved.

Are we willing to go through with this and eventually merge? We'll need to implement changes like 3da4e5b in conformance instead.

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.

🥳 🎈 🎉

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 this herculean effort ❤️

I agree we want to remove duplicates, so we only maintain conformance testing, but there seems to be a set of "duplicates" which I feel we should still keep, because even tho it is a duplicate of gateway conformance test, it is also testing Kubo-specific things like configuration, or some Kubo-specific HTML responses, which we want to remove from conformance at some point.

See comments inline – some examples there, lmk if my thinking is correct, or if we have tests for these elsewhere.

Copy link
Member

Choose a reason for hiding this comment

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

⚠️ tests of Kubo-specific HTML behavior we have here should not be removed, as we have no other tests for these (conformance will not check for "Index of" or inspect links in breadcrumbs).

I know we have tests for this in boxo too, but that is not always enough.
Things like dnslink gw: hash column should be a CID link to cid.ipfs.tech could pass in boxo but be broken in Kubo due to invalid Gateway.PublicGateways config wiring.

MAybe rename this (and other tests like this) to t0115-gateway-kubo-specific-html-dir-listing.sh to make it very clear the tests we kept are specific to Kubo.

Copy link
Contributor Author

@laurentsenta laurentsenta Jun 29, 2023

Choose a reason for hiding this comment

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

We do have them in conformance, for example:
https://github.com/ipfs/gateway-conformance/blob/d921d6259689aca6a75774e31978ea8f88d3a667/tests/path_gateway_unixfs_test.go#L50-L65
(living in ipfs/gateway-conformance#92)

Later we'll implement HTML expectations to remove the Kubo-strings checks (ipfs/gateway-conformance#17).

Copy link
Contributor Author

@laurentsenta laurentsenta Jun 30, 2023

Choose a reason for hiding this comment

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

@lidel you thumb's up'ped, but I would like to be 100% sure, is that enough? Would you prefer I restore the file in this PR?

test/sharness/t0114-gateway-subdomains.sh Outdated Show resolved Hide resolved
test/sharness/t0114-gateway-subdomains.sh Outdated Show resolved Hide resolved
test/sharness/t0112-gateway-cors.sh Outdated Show resolved Hide resolved
test/sharness/t0114-gateway-subdomains.sh Outdated Show resolved Hide resolved
test/sharness/t0109-gateway-web-_redirects.sh Outdated Show resolved Hide resolved
test/sharness/t0109-gateway-web-_redirects.sh Outdated Show resolved Hide resolved
test/sharness/t0109-gateway-web-_redirects.sh Outdated Show resolved Hide resolved
test/sharness/t0109-gateway-web-_redirects.sh Outdated Show resolved Hide resolved
test/sharness/t0116-gateway-cache.sh Outdated Show resolved Hide resolved
@laurentsenta
Copy link
Contributor Author

@lidel last 6 commits are restores for the file you flagged, thanks again for the deep review, let me know if that's all good!

@laurentsenta laurentsenta requested a review from lidel June 30, 2023 08:07
@laurentsenta laurentsenta force-pushed the feat/trim-sharness branch 2 times, most recently from 344090f to b6c987c Compare June 30, 2023 10:40
@laurentsenta laurentsenta marked this pull request as ready for review June 30, 2023 10:49
@laurentsenta
Copy link
Contributor Author

Marking this PR ready for review, the failing sharness tests are the same in master.

@lidel
Copy link
Member

lidel commented Jun 30, 2023

Ack, sharness was failing due to unrelated DNS issue – fixed it in https://github.com/protocol/infra/pull/1211.
and is now green.

Run out of time today, but will make final look next week (just want to make another pass).

@hacdias
Copy link
Member

hacdias commented Jul 25, 2023

ping @lidel as you said you wanted to take one last pass

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 for addressing the feedbackn and once again, for driving this cleanup @laurentsenta ❤️

Merging this sgtm, we keep some bare minimum for most important surface (subdomains, used in Brave and ipfs-companion) + kubo-specific behavior,
but the coverage efforts should happen in https://github.com/ipfs/gateway-conformance.

(I am still a bit anxious we've missed something due to the size of changes, and lose coverage around niche edge case, but merging this is a lesser evil, and a forcing function for all of us to invest time into specs and gateway-conformance 👍 )

@lidel lidel changed the title feat: remove sharness tests ported to conformance testing chore: remove sharness tests ported to conformance testing Aug 2, 2023
@lidel lidel merged commit 7977f26 into master Aug 2, 2023
13 checks passed
@lidel lidel deleted the feat/trim-sharness branch August 2, 2023 17:15
@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.

4 participants