Refactor int64 timeout to time.Duration #1035
Conversation
There was a problem hiding this comment.
Nice catch, thanks for this! WDYT about the following?
Should we update this part of the code (WaitForEvent), too? And make it millisecond-based for consistency throughout our API?
xk6-browser/common/browser_context.go
Line 351 in 22ca963
Maybe not related to this PR, but I guess we have a bug in WaitForEvent:
xk6-browser/common/browser_context.go
Line 351 in 22ca963
It multiplies 30 * time.Second with time.Second (might be problematic if that option is the default):
Line 11 in 22ca963
I like that idea, and yeah, maybe best for another PR. Happy to take that on after i've got these few timeout PRs merged in 👍 |
ankur22
changed the title
The timeout is assumed to be in milliseconds when the user sets it using the provided APIs on browserContext and page. The issue is that internally it's assumed to be seconds. This commit fixes that for the default navigation so it's always assumed to be milliseconds.
The timeout is assumed to be in milliseconds when the user sets it using the provided APIs on browserContext and page. The issue is that internally it's assumed to be seconds. This commit fixes that for the default timeout so it's always assumed to be milliseconds.
We should try to work with time duration across most of the codebase. The only place where it could be an int64 is when the module receives a millisecond value in int64.
time.Duration should be used throughout the codebase instead of int64. This change ensures that in64 is quickly converted to a time.Duration which means we have to care less about whether the in64 is in seconds or milliseconds etc.
We should be working with time.Duration throughout the code base instead of trying to work out what int64 means. This change aligns the default timeout to this aim.
We can reduce the sleep time on the timeout integration tests since the timeout is now not being confused between milliseconds and seconds.
ankur22
force-pushed
the
fix/use-time-duration-timeout
branch
from
22ca963 to
1826aaf
Compare
What?
This PR fixes a problem where the timeout was an int64 making it difficult to workout whether that was in seconds or milliseconds. This change enforces us to work with
time.Durationwhen it comes to the default and navigation timeouts.Why?
This prevents confusion as to whether timeout (which was a int64) was in milliseconds or seconds.
Checklist
Related PR(s)/Issue(s)