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

Do not test ETAG values #73

Open
Tracked by #121
laurentsenta opened this issue Jun 5, 2023 · 3 comments
Open
Tracked by #121

Do not test ETAG values #73

laurentsenta opened this issue Jun 5, 2023 · 3 comments
Labels
feat:universality Minimize implementation-specific checks

Comments

@laurentsenta
Copy link
Contributor

Brought up by @lidel and @hacdias on Slack:

https://filecoinproject.slack.com/archives/C04M8232QRW/p1685573605787349?thread_ts=1685531506.240809&cid=C04M8232QRW

For the Etags specifically , the best IMO would be to move those tests to Boxo and remove them from both Kubo and the conformance. I think that’s something we should agree on.

yeah, conformance is expecting boxo/kubo-specific Etag values (example) because MVP was to port sharness.
next step is to make these tests vendor-agnostic:
Conformance should not care what is inside of Etag, but must test Etag is present, and unique to specific resource+response type.
Boxo/Kubo can test specific values.

@hacdias
Copy link
Member

hacdias commented Jun 5, 2023

Yeah, I already changed the CAR ones in #56 to only check for its presence, but not the value itself.

@hacdias
Copy link
Member

hacdias commented Jun 8, 2023

I was thinking about this and it's not just about Etag in the HTTP Response. It's also about If-None-Match in the HTTP Request. The current tests assume that ETags are built by Boxo, and many will fail with different implementations.

To make them good tests, we have to first make a request and use the Etag value for a new request with If-None-Match. We can make this be two tests, dependent on each other. The first one checks that there is an Etag, the second one uses that value as If-None-Match and should get a Not Modified status.

Summarising, I think there's a few things here:

  • Change all tests that check for specific Etag to Not().IsEmpty()
  • Change all tests that send If-None-Match to: (1) make request, (2) read Etag, (3) send request with Etag of previous request as If-None-Match
  • Add tests to ensure that different ?format for same /ipfs/path return different Etag
  • Add tests to ensure that different /ipfs/path return different Etag
  • Add specific tests to Boxo with our known Etag values.
  • Add X-Ipfs-Roots check for current tests.

I'm not sure if this is something we can do right now: use the values of one request for the next. cc @laurentsenta @galargh

@galargh
Copy link
Contributor

galargh commented Jun 12, 2023

I'm not sure if this is something we can do right now: use the values of one request for the next. cc @laurentsenta @galargh

Not yet but it's something we've been thinking about for a while and waiting for when the actual need arises - it seems it just has. We've been playing around with this idea in #30. The PR itself is not longer relevant in its' current form but there are some parts of it that we'll salvage.

@laurentsenta laurentsenta added the feat:universality Minimize implementation-specific checks label Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat:universality Minimize implementation-specific checks
Projects
None yet
Development

No branches or pull requests

3 participants