-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Test against static servers #763
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the PR and ended up using it myself to test a server in another region. However there are a few changes I'd suggest.
The current PR puts most of get_servers()
under a conditional (if servers_json is None:
), which can possibly break other functionality and makes the diff harder to analyze since things are nested a step further.
I would suggest:
- Change the command line flag to something simpler like just
--json [file.json]
- Instead of --json overriding get_servers(), have it add the server to the servers list. Instead of the conditional, rearrange it like:
#start of get_servers()
"""If it exists, load in a server config from a JSON file."""
if servers_json:
printer('Loading Servers from:\n%s' % servers_json, debug=True)
try:
with open(servers_json) as json_file:
self.servers = json.load(json_file)
except FileNotFoundError:
#handle the exception
#continue with get_servers()
if servers is None:
#...
Then the user can still specify the server via --server [ID]
. The reason I suggest this over the current implementation is that there may be cases where the user wants to include the server or servers via JSON but not exclude other servers from being considered relative to "best server based on ping".
usage: speedtest.py [-h] [--no-download] [--no-upload] [--single] [--bytes] | ||
[--share] [--simple] [--csv] | ||
[--csv-delimiter CSV_DELIMITER] [--csv-header] [--json] | ||
[--load-servers-from-json SERVERS_JSON] [--list] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something shorter like --json would suffice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--json is already being used for specifying json output
"sponsor": "R-KOM GmbH & Co. KG", | ||
"id": "4404", | ||
"host": "speedtest.glasfaser-ostbayern.de:8080", | ||
"d": 313.8447559412203 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is distance, right? What is the source of that? Might be worth specifying in the readme. (I only know of https://c.speedtest.net/speedtest-servers-static.php which does not provide distance.)
|
||
if exclude is None: | ||
exclude = [] | ||
if servers_json is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idk if I would nest the whole get_servers() function under this conditional. The code looks right at first glance (and functioned well enough when testing the added json functionality) but that doesn't mean it won't throw something else off.
It may be preferable to still get a list of servers and insert the static ones via JSON, then be able to list them with --list and select them with --server. This would also prevent nesting all the below under this if statement.
Thanks for your feedback, atm I've not too much time for my personal projects, but I'll try to update the PR in the next week. |
PR #765 might help with your core issue of the server list not always having the server you want. |
just wondered why i couldn't specify a direct server number for a server in norway to test against netherlands 6554 and 17137 and found this actual discussion. the option was working for the last years, seems that the serverlist check is pretty nonsense, cause an server ID does not need to be in the server list, which is limited to 100 servers based on the ping radius, so why was that implemented and changed, it makes no sense and is a bug. |
+1 on the question from @001101 - I'm in California and the only servers that show up for me now are on the US East Coast. I get "ERROR: No matched servers: 14204" when I try to specify a server, e.g., UPDATE: and then, half an hour later, it was working again - somebody must've rebooted a server somewhere. I'll leave it here in case you already read it, but please ignore my post. |
I have also run into this issue.
…On Sun, Jan 10, 2021 at 1:29 AM jgarr16 ***@***.***> wrote:
+1 on the question from @001101 <https://github.com/001101> - I'm in
California and the only servers that show up for me now are on the US East
Coast. I get "ERROR: No matched servers: 14204" when I try to specify a
server, e.g., speedtest --server 14204 even though this is a known good
server that resolves on older versions of speedtest-cli.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#763 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEYUR2LF3FGPPQ2GHVBMTRTSZFJPNANCNFSM4TEXIFBQ>
.
|
No thanks. |
I've added a option to test against a list of servers loaded from a json file.
I needed this option because the server I wanted to test against sometimes didn't appeared in
speedtest.get_servers()
Now we can define one ore more servers like this:
example-server.json