Catch Timeout on test connection

[Glutexo]

All Pull Requests:

Check all that apply:

• Have you followed the guidelines in our Contributing document, including the instructions about commit messages?
• 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 RedHatInsights/insights-client#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.

[m-horky]

Overall, this PR looks good, I like the tests. I left a few notes there to consider.

[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.

[m-horky]

LGTM for the code itself.

CI for Python 2.6 is failing on

E       fixture 'caplog' not found
>       available fixtures: cache, capfd, capsys, cov, doctest_namespace, insights_connection, monkeypatch, pytestconfig, record_xml_property, recwarn, run_rule, tmpdir, tmpdir_factory

you'll need to find a replacement (or e.g. backport it and use in 2.6 only) for that.

[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.

[m-horky]

LGTM.

[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 ✅

Project coverage is 77.05%. Comparing base (416ed6b) to head (6588b75).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4211      +/-   ##
==========================================
+ Coverage   77.01%   77.05%   +0.04%     
==========================================
  Files         761      761              
  Lines       41471    41471              
  Branches     8763     8763              
==========================================
+ Hits        31937    31955      +18     
+ Misses       8481     8462      -19     
- Partials     1053     1054       +1     

Flag	Coverage Δ
unittests	77.03% <100.00%> (+0.04%)	⬆️

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]

If this gets merged before #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 #4246 which itself is blocked by #4248. Merging those first will make merging easier and would significantly reduce the size of the test patch.

[Glutexo]

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