Unify duration configuration values #1738
Conversation
| case time.Duration: | ||
| return d, nil | ||
| case string: | ||
| return ParseExtendedDuration(d) | ||
| case float32: | ||
| return time.Duration(float64(d) * float64(time.Millisecond)), nil | ||
| case float64: | ||
| return time.Duration(d * float64(time.Millisecond)), nil | ||
| default: | ||
| n, err := getInt64(v) |
There was a problem hiding this comment.
Are any beside time.Duration, string, float64 and int64 ... even possible? Because the previous code didn't handle the rest, and I have the feeling that was because we will never actually get one ?
There was a problem hiding this comment.
I added them out of an abundance of caution... I have no idea if some weird webpack minification magic or something like that won't try to encode plain integer values as something that goja will export as uint8 or whatever... Stranger things have happened... 😅 And given that this helper function is intentionally outside of js/, I also think it makes some sense to handle the other Go int native types. This way the function is more complete and can basically convert every possible value we can recognize as a time.Duration to that, no need to second-guess.
| case int: | ||
| return int64(n), nil | ||
| case int8: | ||
| return int64(n), nil | ||
| case int16: | ||
| return int64(n), nil | ||
| case int32: | ||
| return int64(n), nil |
There was a problem hiding this comment.
I'm still baffled that multiple cases don't work for type switches 😕:
This could be much more succinct:
case int, int8, int16, int32:
return int64(n), nilBut I guess there's a good reason for it.
There was a problem hiding this comment.
You can do that, but then n will be of an interface{} type, and you can't directly cast that to int64... You can do it via the reflect package, but that'd be much slower and somewhat type unsafe.
Hmm good point, I'll see about adding a test. Not sure it's required, since the options in the archive bundles are basically the equivalent of the JSON+JS ones, but I'll dig around and test it manually, if nothing else. In any case, most old tests with duration values without units probably hadn't worked since k6 v0.27.0, since people would have gotten errors like |
na--
force-pushed
the
unify-duration-configs
branch
from
a5fcc9c
to
6dd5316
Compare
Codecov Report
@@ Coverage Diff @@
## master #1738 +/- ##
==========================================
+ Coverage 71.40% 71.45% +0.05%
==========================================
Files 178 178
Lines 13751 13777 +26
==========================================
+ Hits 9819 9845 +26
Misses 3320 3320
Partials 612 612
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
I tested archives manually, and they are not a problem. If someone created an archive with k6 between v0.27.0 and v0.29.0, anything smaller than |
Basically, yes. And also treat integers ad milliseconds instead of nanoseconds everywhere, which hopefully no-one has ever consciously used.... 😅 I'll add a |
This should close #1305