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

Updates subscription tests for faster execution #211

Merged
merged 1 commit into from
Jan 28, 2021

Conversation

crystalin
Copy link
Collaborator

What does it do?

It improves the subscription tests to avoid waiting 10s per test.

What important points reviewers should know?

web3 SDK is broken and types are not well implemented :/

Is there something left for follow-up PRs?

Some of the subscription tests are not independent (which is also true for many other tests to increase speed). This should be fixed at some point. (Probably when we use --dev which will make boot a lot faster)

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

Checklist

  • Does it require a purge of the network?
  • You bumped the runtime version if there are breaking changes in the runtime ?
  • Does it require changes in documentation/tutorials ?

@tgmichel
Copy link
Contributor

Awesome, looks much nicer now! I understand we don't need the timeouts anymore because the CI is executed in our machine?

Copy link
Contributor

@joelamouche joelamouche left a comment

Choose a reason for hiding this comment

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

What do you mean when you say web3 sdk is broken? Are you referring to the typing?

function web3Subscribe(type: "logs", params: {}): Subscription<Log>;
function web3Subscribe(type: "newBlockHeaders" | "pendingTransactions" | "logs", params?: any) {
return (context.web3.eth as any).subscribe(...arguments);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this code above? Is it to make sure the result of this function is typed for every type of event?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right, I mostly meant the Typings is broken.
I defined this web3Subscribe to be able to return the modified Subscription class which allow the use of "once" (which is not declared in the default web3 subscription)

tests/tests/test-subscription.ts Show resolved Hide resolved
Copy link
Contributor

@JoshOrndorff JoshOrndorff left a comment

Choose a reason for hiding this comment

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

Looks clean 👍

Probably when we use --dev which will make boot a lot faster

I'm not really expecting the node start time to get faster with #204

@crystalin
Copy link
Collaborator Author

Awesome, looks much nicer now! I understand we don't need the timeouts anymore because the CI is executed in our machine?

No, it is more related how the callback and promises were used. I tried my branch on the github servers (see https://github.com/PureStake/moonbeam/actions/runs/518123822) and it worked fine too.

What I suspect is the fact that you provided a callback to the test, which in return prevents the use of async promises. But I didn't test the previous code on github servers so I'm not sure.

@crystalin
Copy link
Collaborator Author

Looks clean 👍

Probably when we use --dev which will make boot a lot faster

I'm not really expecting the node start time to get faster with #204

The idea is that using the --dev instead of the plain specs, the load time will be a lot faster (already observed when doing that manually)

@crystalin crystalin merged commit 636ae70 into master Jan 28, 2021
@crystalin crystalin deleted the crystalin-sub-tests branch January 28, 2021 19:00
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.

4 participants