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

test: re-enable hamt-test in interop #450

Closed
wants to merge 8 commits into from

Conversation

SgtPooki
Copy link
Member

@SgtPooki SgtPooki commented Feb 23, 2024

Based on fix from #448, only including e0f8d5e & 3c40080, and then copying over some code from ipfs-unixfs-exporter

Title

Description

Re-enables the hamt-sharded test since it's reliably passing before 60 second timeout now (roughly 40seconds on my machine)

Notes & open questions

Until the fix in #448 is sufficient, this should not be merged. We should decide on the proper fix, and utilize this test to ensure things are working as expected.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

Copy link
Member Author

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

self review

@@ -0,0 +1,147 @@
import { decode, type PBLink, type PBNode } from '@ipld/dag-pb'
Copy link
Member Author

Choose a reason for hiding this comment

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

all of this was copied over from ipfs-unixfs-exporter

@@ -4,6 +4,8 @@ import { DoesNotExistError, InvalidParametersError } from '../../errors.js'
import { addLink } from './add-link.js'
Copy link
Member Author

Choose a reason for hiding this comment

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

All of the changes in here are originally from @aschmahmann in #448, except that I copied the contents of ./find-shard-cid.js from ipfs-unixfs-exporter

Comment on lines +87 to +95
const onProgress = sinon.stub()
const resp = await verifiedFetch('ipfs://bafybeidbclfqleg2uojchspzd4bob56dqetqjsj27gy2cq3klkkgxtpn4i/685.txt', { onProgress })
expect(resp).to.be.ok()
const text = await resp.text()
const onProgressEvents = onProgress.getCalls().map(call => call.args[0])
const walkEvents = onProgressEvents.filter((e) => e.type.includes('unixfs:exporter:walk'))
const blockGetEvents = onProgressEvents.filter((e) => e.type === 'blocks:get:providers:get')
expect(blockGetEvents).to.have.length(7)
expect(walkEvents).to.have.length.lessThanOrEqual(8)
Copy link
Member Author

Choose a reason for hiding this comment

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

note that I re-enabled this test on a branch https://github.com/ipfs/helia/tree/test/re-enable-hamt-on-main and it doesn't seem to fail there (it was only failing locally previously), but the events don't show the issues that are seen in ipfs/service-worker-gateway#19 & ipfs/service-worker-gateway#18.

They both have onProgressEvents.length === 41 (which includes blockstore & verified fetch events), so some event is not being surfaced fully from the walk without the changes here.

Copy link
Member Author

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

reviewing the latest changes (addition of sinon dep to interop)

@@ -164,6 +164,7 @@
"@libp2p/interface": "^1.1.2",
"@libp2p/logger": "^4.0.5",
"@multiformats/murmur3": "^2.1.8",
"err-code": "^3.0.1",
Copy link
Member

@achingbrain achingbrain Feb 26, 2024

Choose a reason for hiding this comment

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

Please use CodeError from @libp2p/interface instead of adding err-code

@achingbrain
Copy link
Member

Please take a look at #455 - it also re-enables this test but uses the ipfs-unixfs-exporter to do the DAG traversal so we don't need to copy/paste any code from anywhere.

@SgtPooki
Copy link
Member Author

Closing in favor of #455

@SgtPooki SgtPooki closed this Feb 27, 2024
@SgtPooki SgtPooki deleted the fix/hamt-overqueried-copy-unixfs-exporter branch February 27, 2024 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants