Catch Timeout on test urls

[Glutexo]

All Pull Requests:

Check all that apply:

• Have you followed the guidelines in our Contributing document, including the instructions about commit messages?
• No Sensitive Data in this change?
• Is this PR to correct an issue?
• Is this PR an enhancement?

Complete Description of Additions/Changes:

The internal methods used by the --test-connection command only used to catch ConnectionError, including ConnectionTimeout, but not other timeouts such as ReadTimeoutError. As a result, in case of a read timeout,  the failure is not fully handled. That means:

• an error message containing the failed URL is not logged;
• and in case of legacy upload, the URL fallback mechanism is interrupted.

Re-used the existing REQUEST_FAILED_EXCEPTIONS list to process all timeouts in the same way.

Card IDs:

• CCT-859

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.10%. Comparing base (416ed6b) to head (bafcb3d).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4248      +/-   ##
==========================================
+ Coverage   77.01%   77.10%   +0.09%     
==========================================
  Files         761      761              
  Lines       41471    41471              
  Branches     8763     8763              
==========================================
+ Hits        31937    31976      +39     
+ Misses       8481     8438      -43     
- Partials     1053     1057       +4     

Flag	Coverage Δ
unittests	77.09% <100.00%> (+0.09%)	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Glutexo]

I extracted this fix from #4237 and originally from #4211. #4211 only changes exception handling in the outer test_connection method, because that is the minimal patch to fix the reported issue of a stack trace being dumped on the standard output. This new PR, on the other hand, fixes error logging and fallback mechanism in case of a read timeout.

Moreover, this would unblock #4246 in case of #4211 being merged first of the two. See my comment in #4246 for more details.

[Glutexo]

This patch only updates the inner _test_urls and _legacy_test_urls methods, not the outer test_connection method. It doesn’t even change any printing or logging logic. Therefore, unlike #4211 and #4246, there are no tests of test_connection or integration tests.

The error log and standard out tests are the only way how to find out whether the except block fired as intended. That’s why there are log/stdout tests in place. They, however, don’t test the contents of the messages as that’s not the subject of this pull request. That will change after merging with the tests from other related PRs.

[Glutexo]

Open for review. @m-horky

[m-horky]

I don't have objections, I'll just need to see in context of the other PRs.

[m-horky]

Are all these tests for the patch of connection.py in this PR?

[Glutexo]

Not entirely. As I mentioned in my comment, there are only tests for the _test_urls and _legacy_test_urls. There are no tests for test_connection.

Some notes for tests that may seem over the scope:

• The log and print tests are there to check that the except block executed. They only assert the calls, not the contents.
• The get/post methods are the ones that raise the caught exceptions. Hence the call asserts for those.

The other pull requests contain more tests or make some of the existing ones more precise. The latter applies for the log/stdout messages. I will resolve the merge conflict, by which the new patch in the following PR will be nice and clear. The test names are the same, so this will be easy.

[Glutexo]

To make things simpler, let‘s say this is a blocker of #4246, which is a blocker of #4211. Merging this first will make further reviewing and merging easier.

[Glutexo]

There is also #4254 in the family. It neither blocks anything nor is blocked by anything. It only shares some tests.

[m-horky]

ACK

[m-horky]

Hi, @xiangce, good to merge!