fix: flooring of progress when rate is low

[vrajashkr]

Fixes flooring of the rate when precision is 0 and the rate is low (between 0 and 1).

Closes #1753

Any feedback and suggestions are welcome!

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[codecov-io]

Codecov Report

Merging #1766 (221e522) into master (1443e5e) will decrease coverage by 0.02%.
The diff coverage is 25.00%.

@@            Coverage Diff             @@
##           master    #1766      +/-   ##
==========================================
- Coverage   71.45%   71.43%   -0.03%     
==========================================
  Files         178      178              
  Lines       13777    13784       +7     
==========================================
+ Hits         9845     9846       +1     
- Misses       3319     3324       +5     
- Partials      613      614       +1     

Flag	Coverage Δ
ubuntu	71.39% <25.00%> (+0.01%)	⬆️
windows	69.97% <25.00%> (-0.06%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
lib/executor/constant_arrival_rate.go	93.14% <25.00%> (-3.29%)	⬇️
core/engine.go	90.95% <0.00%> (-2.02%)	⬇️
stats/cloud/collector.go	80.44% <0.00%> (+0.63%)	⬆️
js/runner.go	81.10% <0.00%> (+0.65%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1443e5e...221e522. Read the comment docs.

[asettouf]

Would be nice to have at least a test case associated with it. Maybe in constant_arrival_rate_test.go for unit testing?

[vrajashkr]

Would be nice to have at least a test case associated with it. Maybe in constant_arrival_rate_test.go for unit testing?

Thank you for the suggestion! I'll start working on it.

[vrajashkr]

Hey again! I looked at the code in constant_arrival_rate_test.go. However, there don't appear to be any existing tests in there testing the printed rate (unless I missed something).

Would be great if I could get some help with adding a test case for this.

Thanks!

[imiric]

@darkaether Thanks for the PR, and yeah, testing this as is would be difficult since this value is only exposed when rendered in the UI.

I'm wondering if this logic is generic enough for all use cases where pb.GetFixedLengthFloatFormat() is used. What do you think @mstoykov?

If so, we could make it part of that function, otherwise you could extract this into a standalone function in constant_arrival_rate.go and just unit test that, avoiding the need to test ConstantArrivalRate.Run().

[mstoykov]

This also seems to be broken in the ramping arrival rate as well ...

I wonder if it won't be easier and more user friendly for the format to be something like "1/5s" (read as "1 per 5 seconds") instead of getting floats involved ... as "0.2/s" seems ... bad IMO ... and it might be okay here, but the ramping arrival rate will change this so it will be something like "0.1112/s" then "0.1113/s" and so on ... instead it should probably go from something like "0/20s" to "1/20s" or similar ... Where always the "/s" is just what is configured and the number in front is the only thing that changes

[vrajashkr]

This also seems to be broken in the ramping arrival rate as well ...

I wonder if it won't be easier and more user friendly for the format to be something like "1/5s" (read as "1 per 5 seconds") instead of getting floats involved ... as "0.2/s" seems ... bad IMO ... and it might be okay here, but the ramping arrival rate will change this so it will be something like "0.1112/s" then "0.1113/s" and so on ... instead it should probably go from something like "0/20s" to "1/20s" or similar ... Where always the "/s" is just what is configured and the number in front is the only thing that changes

Hey! Sorry for the late response. This sounds like an interesting idea. This definitely seems like it would be more readable especially for smaller fractions. If it sounds reasonable and in scope, I could try out this change.

[mstoykov]

Closing in favor of #2282. As mentioned there - there has been some internal debate and we fill some fix is better than no fix, and nobody has had time to try my idea, which also will make the UI a bit less intuitive - given that it will have different timeUnits on the screen. It also doesn't fix going from 0 to 1 over long periods for example.

Thanks for the PR @darkaether 🙇 , even though it won't be merged you have helped push this issue forward 🎉