sindresorhus / speed-test

Test your internet connection speed and ping using speedtest.net from the CLI
MIT License
3.91k stars 164 forks source link

add support for MBps #31

Closed SrinivasanTarget closed 8 years ago

SrinivasanTarget commented 8 years ago

@sindresorhus Please look into this. Also tested from my side.It works as expected but not sure how to test through test.js.

Fixes #28.

sindresorhus commented 8 years ago

I think it could be done a bit simpler and more DRY.

SrinivasanTarget commented 8 years ago

@sindresorhus Thank for your comments. Learning a lot :) Will try my best to make it as simple as possible.

SrinivasanTarget commented 8 years ago

@sindresorhus @SamVerschueren Hope this is fine. Please review.

Thanks, srini

SamVerschueren commented 8 years ago

You can use a multiplier that could be used everywhere instead of checking every time

var multiplier = cli.flags.bytes ? 1/8 : 1;
SrinivasanTarget commented 8 years ago

Thanks.Updated the PR now @SamVerschueren

sindresorhus commented 8 years ago

Much better :)

You missed https://github.com/sindresorhus/speed-test/pull/31#discussion_r48693104

SrinivasanTarget commented 8 years ago

Updated the PR now @sindresorhus

SrinivasanTarget commented 8 years ago

Hope this is fine @sindresorhus @SamVerschueren

SrinivasanTarget commented 8 years ago

@sindresorhus

SamVerschueren commented 8 years ago

I guess only the last 2 comments I provided and then it's good to go! Thanks for implementing this.

SrinivasanTarget commented 8 years ago

@SamVerschueren Not sure about the reason for failure. Can you help me to resolve this?

SamVerschueren commented 8 years ago

It took to long (more then 17 minutes) to determine the speed of the connection between travis and the test server. Nothing we can do about it. I restarted the test, might be resolved now.

SamVerschueren commented 8 years ago

Tests are fixed now.

SrinivasanTarget commented 8 years ago

Yup its passed now

SamVerschueren commented 8 years ago

I guess this is good to merge. Good job :+1:

sindresorhus commented 8 years ago

Landed. Thank you @SrinivasanTarget :)