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

fix: test for subscribers to ipns pubsub topic #584

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

lidel
Copy link
Member

@lidel lidel commented Aug 21, 2024

Description

IPNS interop tests for helia→pubsub→kubo did not execute the same tests as kubo→pubsub→helia.

Namely, publishing to IPNS was executed with assumption it will fail, as a convoluted way of confirming the kubo is not subscribed to the topic, and also kubo connectivity state is somehow taken on belief, rather than being programmatically verified:

// first publish should fail because kubo isn't subscribed to key update channel
await expect(name.publish(peerId, cid)).to.eventually.be.rejected()
  .with.property('message', 'PublishError.NoPeersSubscribedToTopic')

// should fail to resolve the first time as kubo was not subscribed to the pubsub channel
await expect(last(kubo.api.name.resolve(peerId, {
  timeout: 100
}))).to.eventually.be.undefined()

Good news is that there are native APIs for inspecting subsub topic subscriptions, and this PR refactors test to use them and have tests in both directions do more-or-less the same thing with the same asserts.

This should make interop less brittle.

Notes & open questions

This PR aims to fix problem identified in ipfs/kubo#10488 (comment).

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

@lidel lidel force-pushed the test/fix-helia-pubsub-kubo branch from 555a187 to ab6b385 Compare August 21, 2024 21:05
@lidel lidel marked this pull request as ready for review August 21, 2024 21:21
@lidel lidel requested a review from a team as a code owner August 21, 2024 21:21
lidel added a commit to ipfs/kubo that referenced this pull request Aug 21, 2024
@lidel lidel mentioned this pull request Aug 21, 2024
32 tasks
@lidel
Copy link
Member Author

lidel commented Aug 22, 2024

I did not mean to close this, bad Github.

@lidel lidel reopened this Aug 22, 2024
Copy link
Member

@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.

The code looks fine to me. Seems to just be validating that Kubo is actually subscribed like you mention in the PR description.

However, I'm confused why trying to resolve the IPNS name causes Kubo to subscribe to it. Is this a Kubo specific behavior or something in a spec somewhere?

@lidel
Copy link
Member Author

lidel commented Aug 24, 2024

"IPNS over pubsub" is an example of alternative way of resolving IPNS (than Amino DHT).
There is relevant spec at https://specs.ipfs.tech/ipns/ipns-pubsub-router/

Modified test's Kubo runs with "IPNS over PubSub" experiment enabled:

args: ['--enable-pubsub-experiment', '--enable-namesys-pubsub']

This means once we resolve IPNS name, we keep subscription to a topic specific to the name, for getting updates faster than over DHT.

@lidel lidel mentioned this pull request Aug 28, 2024
34 tasks
@SgtPooki
Copy link
Member

@achingbrain are you good with this change?

also, since this is a published "testing" package, I think it should be "fix: " in PR title

@lidel
Copy link
Member Author

lidel commented Sep 11, 2024

@achingbrain achingbrain changed the title test(interop): ipns via helia→pubsub→kubo fix: test for subscribers to ipns pubsub topic Sep 13, 2024
@achingbrain achingbrain merged commit c9c644c into main Sep 13, 2024
37 checks passed
@achingbrain achingbrain deleted the test/fix-helia-pubsub-kubo branch September 13, 2024 16:09
@achingbrain achingbrain mentioned this pull request Sep 11, 2024
lidel added a commit to ipfs/kubo that referenced this pull request Sep 17, 2024
lidel added a commit to ipfs/kubo that referenced this pull request Sep 17, 2024
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