DM-11725: Improve code coverage and numpydoc #51
Conversation
This is not used anywhere and it is far better to specify test file locations using __file__
python/lsst/utils/get_caller_name.py
Outdated
| function has no class) is silently omitted, along with an associated separator. | ||
| An empty string is returned if `skip` exceeds the stack height. | ||
| Any item that cannot be determined (or is not relevant, e.g. a free | ||
| function function has no class) is silently omitted, along with an |
There was a problem hiding this comment.
Duplicated "function" (a pre-existing issue)
python/lsst/utils/get_caller_name.py
Outdated
| ------- | ||
| name : `str` | ||
| Name of the caller as a string in the form ``module.class.method``. | ||
| An empty string is returned is ``skip`` exceeds the stack height. |
python/lsst/utils/tests.py
Outdated
| @@ -63,14 +63,20 @@ | |||
|
|
|||
|
|
|||
| def _get_open_files(): | |||
| """Return a set containing the list of open files.""" | |||
| """Return a set containing the list of open files from this process. | |||
There was a problem hiding this comment.
What does "from this process" mean? Files that were opened by this process? Please reword or clarify in a longer description following the brief description.
python/lsst/utils/tests.py
Outdated
| if psutil is None: | ||
| return set() | ||
| return set(p.path for p in psutil.Process().open_files()) | ||
|
|
||
|
|
||
| def init(): | ||
| """Initialize the memory tester""" | ||
| """Initialize the memory tester and leak tester.""" |
There was a problem hiding this comment.
What is the difference between the memory tester and the leak tester? I thought the memory tester tested for memory leaks. Please reword and/or clarify in a longer description.
There was a problem hiding this comment.
clarification: file descriptor leak tester
python/lsst/utils/tests.py
Outdated
| @@ -80,7 +86,28 @@ def init(): | |||
|
|
|||
|
|
|||
| def run(suite, exit=True): | |||
| """Exit with the status code resulting from running the provided test suite""" | |||
| """Exit with the status code resulting from running the provided test | |||
| suite | |||
There was a problem hiding this comment.
This is a pre-existing issue, but the returned value is apparently the test exit status only if exit is True. Please consider changing this to something like Run a test suite
python/lsst/utils/tests.py
Outdated
| @@ -197,7 +238,7 @@ class ExecutablesTestCase(unittest.TestCase): | |||
| @classmethod | |||
| def setUpClass(cls): | |||
| """Abort testing if automated test creation was enabled and | |||
| yet not tests were found.""" | |||
| yet no tests were found.""" | |||
There was a problem hiding this comment.
Good fix. Consider deleting "yet" as well
python/lsst/utils/tests.py
Outdated
| Path to an executable. ``root_dir`` is not used if this is an | ||
| absolute path. | ||
| root_dir : `str`, optional | ||
| Directory containing exe. Ignored if `None`. |
There was a problem hiding this comment.
"exe" -> "executable"; I realize this is pre-existing, but I find it very confusing, since file names for our executables do not end with .exe
python/lsst/utils/tests.py
Outdated
| @@ -262,16 +306,12 @@ def _build_test_method(cls, executable, root_dir): | |||
| The method is built for the supplied excutable located | |||
| in the supplied root directory. | |||
There was a problem hiding this comment.
I realize this is pre-existing, but... do you have any idea what means? I don't.
There was a problem hiding this comment.
For each executable I create a new method that runs that executable. I attach that method to the test class so that pytest will find it and run the test method later.
tests/test_executables.py
Outdated
| msg="testexe.sh failed") | ||
|
|
||
| # Try a non-existent test | ||
| with self.assertRaises(Exception): |
There was a problem hiding this comment.
Can you safely be more specific than Exception? Perhaps AssertionError? Similarly in several places below
tests/test_get_caller_name.py
Outdated
| @@ -31,7 +31,8 @@ class GetCallerNameTestCase(unittest.TestCase): | |||
| """Test get_caller_name | |||
|
|
|||
| Warning: due to the different ways this can be run | |||
| (e.g. directly or py.test, the module name can be one of two different things) | |||
| (e.g. directly or py.test, the module name can be one of two different | |||
| things) | |||
There was a problem hiding this comment.
Pre-existing, but please move the ) to just after py.test
and wrap at 79 characters.
I'm testing code coverage testing with pytest and as part of that I ended up improving test coverage in utils. I also tweaked the numpydoc a bit whilst I was there and ensured it is 79 column compliant.