DM-10401 Make utils.getPackageDir raise correct exception #32
Conversation
| @@ -22,6 +22,8 @@ | |||
| # | |||
| from __future__ import absolute_import | |||
|
|
|||
| import lsst.pex.exceptions | |||
|
|
|||
There was a problem hiding this comment.
Is this the correct place for this import to affect all the parts of the code base? I am not sure, I just want you to verify, and I trust if you say it is so.
There was a problem hiding this comment.
It's the correct place to make it affect all parts of utils anyway. I'm not sure there's a place we can put it that will affect "everything".
There was a problem hiding this comment.
Would it work to put it in a python/lsst/__init__.py in pex_exceptions? Otherwise somewhere low in the hierarchy.
There was a problem hiding this comment.
I don't think that's enough, since one would still have to import lsst.pex.exceptions to see it. See DM-10415 for the broader problem:
tests/testGetPackageDir.py
Outdated
| # Hence the odd structure of this assert block. | ||
| with self.assertRaises(Exception) as cm: | ||
| getPackageDir("nameOfNonexistendPackage2234q?#!") | ||
| import lsst.pex.exceptions |
There was a problem hiding this comment.
why is this import here in the middle of a test function? Are you trying to trigger one kind of behavior above and below this import? As I see it the only thing you are doing below is checking existing variables.
There was a problem hiding this comment.
I jumped right here without reading all the comments on the ticket, so I am sorry. I see this was talked about on the ticket, but I must confess I still do not understand why it is necessary to be placed here. If I remove the import completely the test still seems to pass. You seem to be explicitly importing exceptions in the __init__ file now, which to me seems to ensure it will be imported...
There was a problem hiding this comment.
Woah... I don't understand how the test can pass if lsst.pex.exceptions is not imported (but I just tested it, and it does pass). I put the import here because I thought it was necessary to put lsst.pex.exceptions in the unittest's namespace. The fact that it isn't confuses me.
There was a problem hiding this comment.
It's the magic we put in the lsst/__init__.py files: that makes it so all symbols in lsst/* are pulled into that package namespace whenever they're imported, even if that import happens in C++ with a py::module::import("lsst.pex.exceptions").
There was a problem hiding this comment.
@natelust, I think the point is to test whether the import in __init__ (or something with an equivalent effect) happens. So the code can't assume it.
@parejkoj if you remove the test import, does the test still work in the failure mode it's testing (i.e., no special handling of pex.exceptions in __init__ or the pybind11 files)?
There was a problem hiding this comment.
@kfindeisen Interesting question: removing the import from __init__.py and the unittest results in a different failure,
AttributeError: module 'lsst' has no attribute 'pex'
instead of the expected:
AssertionError: RuntimeError('Package nameOfNonexistendPackage2234q?#! not found',) is not an instance of <class 'lsst.pex.exceptions.wrappers.NotFoundError'>
So, it's a failure, but not exactly the failure I'd hope for. Although I think the different error makes sense given what @TallJimbo says above about our lsst/__init__.py magic.
parejkoj
force-pushed
the
tickets/DM-10401
branch
2 times, most recently
from
729f023
to
9f54f34
Compare
assertRaises with isInstance test of correct exception, and note about why it looks strange.
No description provided.