Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

Add a failing test that verifies we can subscribe to an RPC subscription method #3506

Merged
merged 1 commit into from
Oct 31, 2024

Conversation

mcintyre94
Copy link
Contributor

In rc2 there's a bug where await rpcSubscriptions.anyNotifications().subscribe() hangs. Unlike rpc-api all tests in rpc-subscriptions-api are currently commented out.

This PR adds a (currently failing) test to subscribe to notifications against the localhost test validator.

Notes:

  • I've put this test in rpc-subscriptions rather than rpc-subscriptions-api because it relies on transport functions from rpc-subscriptions. If we wanted to build a full suite of rpc-subscriptions-api tests later then we should probably move these transport functions to a different package where they could be a dependency of rpc-subscriptions-api. We currently have rpc-transport-http for this on the rpc-api side.

  • I used it.skip instead of it.failing because the test times out rather than failing, and Jest doesn't treat that as a pass for it.failing. Anyway I don't plan to leave it failing long!

Copy link

changeset-bot bot commented Oct 31, 2024

⚠️ No Changeset found

Latest commit: ba0a455

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor Author

mcintyre94 commented Oct 31, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @mcintyre94 and the rest of your teammates on Graphite Graphite

Copy link
Contributor

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

I guess slotNotifications would actually be a good one to start with, because it doesn't require writes, but this is fine too!

Added via Giphy

Comment on lines +26 to +27
// eslint-disable-next-line jest/no-disabled-tests
it.skip('can subscribe to account notifications', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can actually write this as it.failing instead of skipping it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that, but because it fails by timing out it doesn't seem to work.

Copy link
Contributor

steveluscher commented Oct 31, 2024

Merge activity

  • Oct 31, 10:05 AM PDT: A user started a stack merge that includes this pull request via Graphite.
  • Oct 31, 10:05 AM PDT: A user merged this pull request with Graphite.

@steveluscher steveluscher merged commit bd1cd50 into master Oct 31, 2024
9 checks passed
@steveluscher steveluscher deleted the subscriptions-failing-test branch October 31, 2024 17:05
Copy link
Contributor

Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants