Skip to content
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

Added connection information to exceptions #1287

Merged
merged 5 commits into from
Aug 19, 2018

Conversation

andy-maier
Copy link
Contributor

@andy-maier andy-maier commented Jun 10, 2018

Ready for review and merge.
What is needed for this PR, is a test on a life server.

The first three commits implement having the connection information in the pywbem exceptions, plus some preparatory changes. Each of these three commits is separate logical step and is best reviewed separately. They were part of the first review.

Compared to the first review, I had to change the connection information that is added to the pywbem exceptions, from adding the entire WBEMConnection object, to adding just the connection ID string. Reason was that in cim_http.py, the connection ID was available but not the connection object, and I did not want to change that there. This change in the exception classes (to take the connection ID string instead of the WBEMConnection object) is done in the fourth commit in this PR.

The fifth commit changes the places that raise pywbem exceptions to pass the connection ID to the exceptions, including some changes that were needed in support of that such as changing a static method to become an instance method.

The following pywbem exceptions that are raised do not get the connection ID passed as part of this PR:

  • Parse errors in CIM-XML responses received from a WBEM server are raised as pywbem.ParseError exceptions and they do not have the connection ID. Reason is that adding the connection ID there is difficult to impossible, because in tupleparse we have flat functions whose interface is determined by our use of PLY.
  • Pywbem exceptions raised in the pywbem listener do not have the connection ID. Reason is that for the listener, we don't have a concept of a connection ID.

@andy-maier andy-maier self-assigned this Jun 10, 2018
@andy-maier andy-maier added this to the 0.13.0 milestone Jun 10, 2018
@andy-maier andy-maier force-pushed the andy/#1155-conn-exceptions branch 3 times, most recently from 51b0f2d to 47f3b45 Compare July 17, 2018 05:07
@coveralls
Copy link

coveralls commented Jul 17, 2018

Coverage Status

Coverage increased (+0.05%) to 84.353% when pulling 52d186d on andy/#1155-conn-exceptions into 31c2ca5 on master.

@andy-maier andy-maier force-pushed the andy/#1155-conn-exceptions branch 2 times, most recently from 92c3a83 to 4933fb3 Compare July 18, 2018 05:33
@andy-maier andy-maier changed the title [WIP] Added connection information to exceptions Added connection information to exceptions Jul 18, 2018
the error happened. `None` if the error did not happen in context
of any connection, or if the connection context was not known.

:ivar args: A tuple (status, reason, cimerror, cimdetails) set from the
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that the args doc moved moved up to HTTPError at the same time you inserted the super... However, I do not see where above this in the hierarchy the args actually gets set. I may be just missing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The args doc on HTTPError was missing, and adding it is a docs fix that is independent of all the other changes.

The args instance variable is set in the Python Exception class, and the previous implementation duplicated that into HTTPError (and into CIMError, fwiw). Now that our Error class has an init method, we can use the more logical approach of delegating that into the Python Exception class. Maybe that would have worked even without an init method in the Error class in between, but I never tried. In any case, the unit tests verify that the args instance variable is set.

Copy link
Collaborator

@KSchopmeyer KSchopmeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only made one comment when I sorted through it all and that was more a question than comment. I like the approach. It looks like most of this was really cleaning up messy code from before.

I approve this.

@KSchopmeyer
Copy link
Collaborator

KSchopmeyer commented Jul 24, 2018

Hit one issue in doing "make clobber install check test" in the tests.

============================= test session starts ==============================
platform linux2 -- Python 2.7.6, pytest-3.6.2, py-1.5.4, pluggy-0.6.0
rootdir: /home/kschopmeyer/pywbem/githubpywbem/pywbem, inifile:
plugins: cov-2.5.1
collected 0 items / 1 errors
Coverage.py warning: No data was collected.

==================================== ERRORS ====================================
______________________________ ERROR collecting  _______________________________
../../../.virtualenvs/pywbem27/local/lib/python2.7/site-packages/py/_path/common.py:377: in visit
    for x in Visitor(fil, rec, ignore, bf, sort).gen(self):
../../../.virtualenvs/pywbem27/local/lib/python2.7/site-packages/py/_path/common.py:429: in gen
    for p in self.gen(subdir):
../../../.virtualenvs/pywbem27/local/lib/python2.7/site-packages/py/_path/common.py:419: in gen
    if p.check(dir=1) and (rec is None or rec(p))])
../../../.virtualenvs/pywbem27/local/lib/python2.7/site-packages/_pytest/main.py:511: in _recurse
    ihook = self.gethookproxy(path)
../../../.virtualenvs/pywbem27/local/lib/python2.7/site-packages/_pytest/main.py:413: in gethookproxy
    my_conftestmodules = pm._getconftestmodules(fspath)
../../../.virtualenvs/pywbem27/local/lib/python2.7/site-packages/_pytest/config/__init__.py:383: in _getconftestmodules
    mod = self._importconftest(conftestpath)
../../../.virtualenvs/pywbem27/local/lib/python2.7/site-packages/_pytest/config/__init__.py:412: in _importconftest
    raise ConftestImportFailure(conftestpath, sys.exc_info())
E   ConftestImportFailure: AttributeError("'module' object has no attribute 'SSLContext'",)
E     File "/home/kschopmeyer/.virtualenvs/pywbem27/local/lib/python2.7/site-packages/_pytest/assertion/rewrite.py", line 216, in load_module
E       py.builtin.exec_(co, mod.__dict__)
E     File "/home/kschopmeyer/.virtualenvs/pywbem27/local/lib/python2.7/site-packages/py/_builtin.py", line 221, in exec_
E       exec2(obj, globals, locals)
E     File "<string>", line 7, in exec2
E     File "/home/kschopmeyer/pywbem/githubpywbem/pywbem/testsuite/testclient/conftest.py", line 137, in <module>
E       import httpretty
E     File "/home/kschopmeyer/.virtualenvs/pywbem27/local/lib/python2.7/site-packages/httpretty/__init__.py", line 28, in <module>
E       from . import core
E     File "/home/kschopmeyer/.virtualenvs/pywbem27/local/lib/python2.7/site-packages/httpretty/core.py", line 102, in <module>
E       old_sslcontext_wrap_socket = ssl.SSLContext.wrap_socket

NOTE: I repeated the test on the latest master and hit same issue.

================================
The good news is that it passed several runs with run_cim_operations. I am not picking up any extra errors.

However, for some reason it takes a very long time to go through the subscriptions and indications so that there is something I don't understand. When I say long time, I mean minutes instead of seconds. Further, the whole slowdown when we start the subscription processing.

Note that this may not be part of this change since I retested with master and one time it operated at normal speed but on a later try it also slowed way down when the subscription tests started.

I think I am going to have to dig issue probably tomorrow afternoon. I am going to continue testing.

@KSchopmeyer
Copy link
Collaborator

Simple tests with wbemcli worked fine.

Details:

* Fixed the incorrect use of cimerror in HTTPError as a CIMError
  exception. It is the CIMError HTTP header field, instead.

* Made the testing for exception text generic, so that changes
  in the exception string representation do not affect the test
  result. This is testing the recorder module, after all.

Signed-off-by: Andreas Maier <andreas.r.maier@gmx.de>
Details:

* Fixed an error in the httperror_args fixture where cimdetails was
  incorrectly specified as a string, when it needs to be a dict.

* Added a test case with empty string to the simple_args fixture.

* Simplified the access to the status_tuple fixture parameter.

* Improved the description of all fixtures.

Signed-off-by: Andreas Maier <andreas.r.maier@gmx.de>
TODO:
* Specify the connection in all pywbem exceptions that are raised.

Details:

* Added a keyword argument 'conn' to all pywbem exceptions, to pass the
  WBEMConnection object of the connection.
  Added two new properties to all pywbem exceptions:
  - A `conn` property of the connection object.
  - A `conn_str` property for the connection info string, for use in exception
    messages.

* Extended the unit test cases in test_exceptions.py with a fixture for the
  'conn' init argument, and with a new _assert_connection() method that
  verifies the connection information.

Signed-off-by: Andreas Maier <andreas.r.maier@gmx.de>
Signed-off-by: Andreas Maier <andreas.r.maier@gmx.de>
…istener

Signed-off-by: Andreas Maier <andreas.r.maier@gmx.de>
@andy-maier
Copy link
Contributor Author

Karl, on your issue: The "check" and "test" targets depend on the "develop" target. If "develop" is not run, you can get these kinds of issues. Please try with "develop" before "check" and "test" and let me know whether the problem persists.

@andy-maier
Copy link
Contributor Author

Discussion points:

  • Are we confident enough w.r.t. tests against real CIMOMs?
  • We can actually add connection information also to tupleparse, by adding attributes to the parser objects. We do that already for some things. Should we add that?

@KSchopmeyer
Copy link
Collaborator

We agreed to add connection info to tupleparse.

Karl will rerun tests again with clean virt env and be sure to make develop

Copy link
Collaborator

@KSchopmeyer KSchopmeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marked approved. For some reason the problem I have in issue #1334 was not showing up in earlier tests but it bit me the last couple of days. Ran with that patch OK.

@andy-maier
Copy link
Contributor Author

I am merging this and will add support for connection info in tupleparse in another PR.

@andy-maier andy-maier merged commit 5ecd731 into master Aug 19, 2018
@andy-maier andy-maier deleted the andy/#1155-conn-exceptions branch August 19, 2018 09:29
@andy-maier
Copy link
Contributor Author

This PR addresses most of issue #1155.

@andy-maier
Copy link
Contributor Author

andy-maier commented Aug 20, 2018

Just for the record: There were several misconceptions in what I stated further up in this PR:

  1. The difficulty of adding cinnection info to tupleparse has been described as being caused by the use of PLY. That is not true; that argument applies to the mof_compiler module but not to tupleparse.
  2. The solution for adding connection info to tupleparse has been suggested to be done by adding attributes to the parser objects. There are no parser objects in tupleparse (they are again in the mof_compiler module).

The solution for adding connection info to the tupleparse (and tupletree) modules is to put the module-global functions into a new class that stores the connection ID as an instance attribute. The solution for tupletree is to add the connection ID as an additional optional argument to the three relevant functions of that module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants