-
Notifications
You must be signed in to change notification settings - Fork 110
chore: increase servewallet timeout. #3574
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
Conversation
await page.goto(`http://${host}:${frontendPort}`); | ||
const elapsed = Date.now() - start; | ||
console.log(`Connected to servewallet on ${host}:${servewalletPort} after ${elapsed} ms`); | ||
; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually added it on purpose :) I think keeping track of it can help us make sure that the value we use is "reasonable". WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(unless you meant the extra ;
that I mistakenly added and removed now :D)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha yes I meant only the extra ;
|
||
} | ||
} | ||
throw new Error(`Timeout exceeded waiting for servewallet on ${host}:${servewalletPort}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The timeout in the CI errored on this line, I wonder if we shouldn't put the timeout elapsed message before this line instead of in the while loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean, in the while we look a successful connection, here we return an error. What do you suggest we change here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oups I didn't read it properly sorry, I looked at the new message that contains elapsed time, but expected it to be a fail error message.
all good from my side 😇
To avoid spurious failures when the servewallet takes more times to be ready, we increase the timeout but also we log how long a successful connection took, in order to help keep track of it.
6915335
to
dc1a215
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
untested LGTM
To avoid spurious failures when the servewallet takes more times to be ready, we increase the timeout but also we log how long a successful connection took, in order to help keep track of it.
Looking at one execution, we were very close to the timeout limit so it does make sense that we had intermittent failures.
Before asking for reviews, here is a check list of the most common things you might need to consider: