PR: Use requests instead of urllib for updates

[mrclary]

Description of Changes

• Replaced the use of urllib.urlopen and urllib.urlrequest with requests.get when checking and downloading updates.
• SSL certificates are now verified by default when checking for updates. This was already the default for downloading updates.
• Some error messaging was updated to reflect possible SSL certificate verification errors.

Issue(s) Resolved

Fixes #21398

[pep8speaks]

Hello @mrclary! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file spyder/workers/updates.py:

Line 289:80: E501 line too long (80 > 79 characters)

Comment last updated at 2023-10-17 19:39:18 UTC

[dalthviz]

Thanks @mrclary ! Left some suggestions regarding the error text when doing the initial check for updates (keep mentioning that the error is related to Spyder checking for updates). I'm missing to give this a manual check but seems like it should work 👍

[ccordoba12]

Some small suggestions for you @mrclary, the rest looks good to me.

Also, could you add a test for WorkerDownloadInstaller? That can be similar to the ones we have for WorkerUpdates and could also test the progress reporting through the sig_download_progress signal.

[mrclary]

Some small suggestions for you @mrclary, the rest looks good to me.

Also, could you add a test for WorkerDownloadInstaller? That can be similar to the ones we have for WorkerUpdates and could also test the progress reporting through the sig_download_progress signal.

Actually, I think the only things to test in the WorkerDownloadInstaller are downloading and progress reporting. This class doesn't really do anything with respect to various circumstances (except Win/mac and Lite/Full, but I don't think we need to worry about that).

Do you have a recommendation on how to go about this? I was thinking of three options:

1. Run UpdateInstallerDialog.start_installation but somehow mock the download url to point to some small test file somewhere.
   • This provides the most coverage.
2. Instantiate WorkerDownloadInstaller and UpdateInstallation, connect sig_download_progress signal, and mock the download url to point to some small test file somewhere.
   • This tests the download worker and progress bar directly, so isolates these classes and is more narrowly tailored, but has less code coverage as it doesn't include the UpdateInstallerDialog class behavior.
3. Either of the above, but somehow mock the requests.Response.iter_content results.
   • If this can be done properly, it will not require actually downloading a file, but actually downloading a file might be something we want tested.

[dalthviz]

Checking this locally I noticed a couple of things:

Status bar text is not being updated when dismissing update:

• Checking the update behavior from 5.4.4 to 5.5.0 (when an update is detected the status bar text changes):

• Simulating the update availability using this PR installer (changing the string version in the installed files):

Status bar text is not being updated in case an error happens while checking updates (no internet connection):

Seems like this was happening also before so just sharing (could be worthy to fix here or maybe in a follow-up PR after this one gets merged).

• Simulating the update availability using the PR installer:

Status bar text is being wrongly updated in case case an error happens while downloading the installer (no internet connection):

Seems like this was happening also before so again just sharing (as the issue above, it could be worthy to fix here or maybe in a follow-up PR after this one gets merged).

[mrclary]

@dalthviz, thanks for reporting. Yes, I believe these may have been occurring before this PR. I recommend that we fix those in a separate PR. I may be able to address them next week before 5.5.0 release.

[mrclary]

Per discussion with @ccordoba12, we will not include unit testing for the WorkerDownloadInstaller. Testing the download of a release artifact will artificially affect our download count on GitHub metrics. To avoid this, a more involved solution is required that would otherwise delay releasing 5.5.0. Additionally, because the end-of-life for the 5.x branch is imminent, efforts at more comprehensive testing are better deferred to the 6.x branch.

[ccordoba12]

Thanks @mrclary!

[CAM-Gerlach]

If the expected page is not returned, we should raise a HTTPError as soon as possible, instead of a much longer and more cryptic JSONDecodeError on the next line or later on. This is much clearer and friendlier for users and ensures their reports will be much clearer and more useful for us.

So, in a followup PR we should add here between the .get() and the .json()

page.raise_for_status()

Which will raise this error appropriately.

[CAM-Gerlach]

We could add one more block here to catch HTTPError:

except HTTPError as err:
    error_msg = HTTP_ERROR_MSG.format(page.status_code)
    ...

with a friendly message, e.g. :

HTTP_ERROR_MSG = _(
    'HTTP error {status_code} when checking for updates.'
    '<br><br>Make sure your connection is working properly.'
)

[CAM-Gerlach]

This should say something more like "Unable to connect to the Spyder update service." since this could be something specific to that site or due to a firewall, proxy, etc. issue rather than just internet connectivity

[mrclary]

@CAM-Gerlach, I'm sorry, I should have requested a review from you as well. I'll submit a follow-up PR with your suggestions.

[CAM-Gerlach]

No worries, I was away on my trip to the Python core dev sprint until yesterday, so I wasn't around anyway.