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(e2e): cleanup timeout ms #1072

Merged
merged 2 commits into from
Oct 3, 2022
Merged

test(e2e): cleanup timeout ms #1072

merged 2 commits into from
Oct 3, 2022

Conversation

TarikGul
Copy link
Member

This sets the timeout to a set const, and raises it. Raising the timeout is just in response to westend's responses being a bit a longer than the other chains. This avoids any testing failing for timeouts.

Copy link
Contributor

@Imod7 Imod7 left a comment

Choose a reason for hiding this comment

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

LGTM! :)

Just a few questions of minor importance :

  1. I was wondering if it is worth having different timeouts for westend vs rest of chains.
  2. The choice of 60000 for TIMEOUT_MS was made based on which criteria ? I imagine the goal was to double the previous value of 30 seconds right ?
    Thank you!!!

@TarikGul
Copy link
Member Author

TarikGul commented Oct 3, 2022

I was wondering if it is worth having different timeouts for westend vs rest of chains.

Unnecessary complexity, they can all be the same MS. The Timeout is there so just in case the request + test doesnt hang. This is a rare case in jest but it can happen.

The choice of 60000 for TIMEOUT_MS was made based on which criteria ? I imagine the goal was to double the previous value of 30 seconds right ?

The choice of 60000 was chosen because I have never really seen it go over 40000 before, but wanted to give it an extra buffer.

@TarikGul TarikGul merged commit 935905d into master Oct 3, 2022
@TarikGul TarikGul deleted the tarik-e2e-ms branch October 3, 2022 18:24
This pull request was closed.
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