Catch Timeout on test connection

[Glutexo]

All Pull Requests:

Check all that apply:

• [x] Have you followed the guidelines in our Contributing document, including the instructions about commit messages?
• [x] Is this PR to correct an issue?
• [ ] Is this PR an enhancement?

Complete Description of Additions/Changes:

The --test-connection command only used to catch ConnectionError, including ConnectionTimeout, but not other timeouts such as ReadTimeoutError. Re-used the existing REQUEST_FAILED_EXCEPTIONS list to process all timeouts in the same way.

There is an integration test pull request https://github.com/RedHatInsights/insights-client/pull/244 for this fix.

Card IDs:

• CCT-506

[Glutexo]

This is a copy of #4191, which got closed when I deleted the remote branch because of the changes in the history of master.

To unblock CCT-506, I only kept the original fix of read timeout not being properly caught. I‘ll create a separate pull request for Matyáš’s discovery of duplicate exception print.

Not all cases were covered by the original set of unit tests, so I backported those I wrote dealing with the additional fix. Now, the happy path and an uncaught exception path is tested in addition to the request failure one.

This pull only deals with the reported bug of not dealing with timeouted requests. As such, it only tests that the exception is properly handled, printing concise details. Integration tests verifying the standard output as a whole is going to be part of the follow-up pull request.

The executive code is same as in the original pull request, the tests changed slightly and there is more of them. Requesting thus a fresh review from @m-horky. Any feedback to them here will help me with the new PR.

[Glutexo]

Also, there was a failing test because of non-ASCII characters in the comments. I replaced the nice quotes (’) with crude ones (').

[xiangce]

FYI, we can ignore the following failed test:

>               assert hasattr(DefaultSpecs, spec_name)
E               AssertionError: assert False
E                +  where False = hasattr(DefaultSpecs, 'cloud_init_cfg_run')

The cloud_init_cfg_run was just removed from core collection and as well from the uploader.json, however, the uploader.json on CDN will be updated after next release. Let's ignore it this time. And the uploader.json will be deprecated soon, as well as this test.

[Glutexo]

Addressed the review comments except one. See my inline comment.

[Glutexo]

Resolved the last review comment and rebased on the current master. Ready for a review.

[Glutexo]

pytest-catchlog providing the caplog fixture became part of Pytest since version 3.3. On previous ones it needs to be installed separately. Added the dependency to setup.py for Python < 2.7. Let’s see what the pipeline says.

[Glutexo]

Adding the dependency to setup.py is not enough. The dependencies are grabbed from another repository. I created a pull request there, but I am afraid it’s never going to be reviewed and merged as it’s a personal repo not owned by RedHatInsights.

I created a card suggesting migration of the requirements. Since jenkins-s2i-example is only used to provide the requirements file and its wheels, I think we can ditch the repo and move the files to this repository. We can even only adopt ci_requirements.txt and curl the wheels directly from PyPI, which is what pip does for other Python versions anyways.

For now, I can either mark the tests as skipped for Python 2.6, or update them to check for function calls to the logger. The latter is suboptimal, but can do for the time being until CCT-768 is resolved. What do you think?

[m-horky]

Aha, interesting. At this point I think those tests could be skipped, let's not spend more time on this than needed.

[Glutexo]

The test_connection method only use a single very simple error logging call. It was trivial to replace it with a logger mock. It doesn’t even use logging string interpolation, so the asserts don’t need to incorrectly test the exact arguments rather than the resulting message.

The other tests of the _test_urls_ and _legacy_test_urls methods use more complex logger calls, but they are no longer part of this pull request.

[Glutexo]

Fixed the remaining Flake8 complaint. Now, everything should be ok. Please take a look at the alternative asserts without caplog.

[Glutexo]

Updated on the current master.

[xiangce]

@m-horky, @Glutexo - So, we are okay to merge this PR, right? I received your comment tagging me from github's notfi, but didn't find it here in the PR (you deleted it?), so having this query.

[m-horky]

@xiangce Hi, after I tagged you I realized it didn't go through QE preverification yet. Sorry for the noise.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 77.18%. Comparing base (11209f9) to head (3e55650).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4211   +/-   ##
=======================================
  Coverage   77.17%   77.18%           
=======================================
  Files         761      761           
  Lines       41472    41472           
  Branches     8763     8763           
=======================================
+ Hits        32008    32010    +2     
+ Misses       8407     8406    -1     
+ Partials     1057     1056    -1     

Flag	Coverage Δ
unittests	77.17% <100.00%> (+<0.01%)	:arrow_up:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[Glutexo]

If this gets merged before https://github.com/RedHatInsights/insights-core/pull/4246, that pull request would bring a regression. See my comment there for more details.

[m-horky]

QE PASSED, this patch has been verified by @zpetrace. Blocked by #4246 now.

[Glutexo]

Cool! So to make things simpler, let‘s say this is blocked by https://github.com/RedHatInsights/insights-core/pull/4246 which itself is blocked by https://github.com/RedHatInsights/insights-core/pull/4248. Merging those first will make merging easier and would significantly reduce the size of the test patch.

[Glutexo]

There is also https://github.com/RedHatInsights/insights-core/pull/4254 in the family. It neither blocks anything nor is blocked by anything. It only shares some tests.

[Glutexo]

https://github.com/RedHatInsights/insights-core/pull/4248 is now merged. This is currently only blocked by https://github.com/RedHatInsights/insights-core/pull/4246.

[Glutexo]

To reduce the number of conflicts, rebased on the new master with https://github.com/RedHatInsights/insights-core/pull/4248 merged in. https://github.com/RedHatInsights/insights-core/pull/4246 got merged too, so going to do the second round.

[Glutexo]

Rebased on current master with https://github.com/RedHatInsights/insights-core/pull/4246 merged in. Requesting a fresh review.

---

tl;dr

Resolving the conficts in tests, I fixed a few inconsistencies:

• A single logger.error call was sometimes asserted with assert_called_once_with(...) and sometimes with mock_calls == [mock.call(...)]. Unified to the latter.
• Logger patches were sometimes listed first and sometimes last. Unified them to be first.

I made verifications of multiple exception prints in _legacy_test_urls more straightforward. Added a separate string list for messages instead of only having a list of exception. As a result:

• There are no more cryptic patch.side_effect = exception_list assignments. They do magic behind the curtain.
• Tests don’t rely on implicit conversions of exception. Plain strings are compared.

The updates to the tests all result from the fact that both test_connection and _(legacy_)test_urls now rescue from the same exception types. ReadTimeout is always expected.

• Parametrization for expected exceptions is always the same.
• Parametrization for unexpected exceptions is no longer necessary. UnexpectedException is enough.
• The fail tests for _(legacy_)_test_urls don’t need to be split for a connection timeout and read timeout.