search

m-lab / mlab-speedtest

Repository of the Interface and Project for speed.measurementlab.net
https://speed.measurementlab.net
Apache License 2.0
36 stars 14 forks source link

Results interface may incorrectly reports throughput units #6

Open nkinkade opened 5 years ago

nkinkade commented 5 years ago

A user reported that their internet connection is supposed to be 12Mbps down and 1Mbps up, but, when testing, the interface reports the upload speed as "0.92 kb/s". They flagged this as a probable bug in our code, and that it likely should be reading "0.92 Mb/s".

Looking at the code that renders this display value, it does appear that there is an issue. When determining the unit for the speed result the code assumes that the value it is working with is Kb/s. However, the value that is getting passed into that function, has already been converted (roughly) from Kb/s to Mb/s.

The easy fix for this, I believe, is to simply stop using the getSpeedUnit() function and to fix the unit Mb/s. Speeds are advertised, usually, as Mbps, and so, even if this worked, reported Kbps or Gbps (Tbps?) may not be useful to a user.

pboothe commented 5 years ago

The conversion you see from Kb to Mb ( https://github.com/m-lab/mlab-speedtest/blob/master/app/assets/js/ndt-wrapper.js#L181 ) is, I thought, actually a conversion from b to Kb?

nkinkade commented 5 years ago

It looks to me as if c2sRate is kilobytes:

https://github.com/m-lab/mlab-speedtest/blob/master/app/assets/js/ndt-browser-client.js#L240

pboothe commented 5 years ago

You win! I had forgotten about that /1000 in there.

I think your solution is correct, but it did not remove enough code - we also need to delete the use of the "justified" function. I'll comment further there.