Move logic out of storage.py #23
Conversation
docker_explorer/de.py
Outdated
| """Returns a Container object given a container_id. | ||
|
|
||
| Args: | ||
| container_id (str): the ID of the container. | ||
| Returns: |
There was a problem hiding this comment.
Add a newline between Args and Returns
docker_explorer/de.py
Outdated
| Returns: | ||
| Namespace: the populated namespace. | ||
| Container: the container's info. |
There was a problem hiding this comment.
What is returned is a container.Container object right?
docker_explorer/de.py
Outdated
| return [self.GetContainer(cid) for cid in container_ids_list] | ||
|
|
||
| def GetContainersList(self, only_running=False): | ||
| """Returns a list of container ids which were running, sorted by start date. |
There was a problem hiding this comment.
Doesn't this return Container objects rather than container ids?
docker_explorer/de.py
Outdated
|
|
||
| Args: | ||
| only_running (bool): Whether we display only running Containers. | ||
| Returns: |
There was a problem hiding this comment.
Add a newline (starting to wonder if that's in fact what the style guide specifies)
There was a problem hiding this comment.
https://google.github.io/styleguide/pyguide.html?showone=Comments#Comments
There are indeed newlines there
docker_explorer/lib/container.py
Outdated
| docker_version (int): (Optional) the version of the Docker storage module. | ||
|
|
||
| Raises: | ||
| errors.BadContainerException: if there was an error with parsing |
There was a problem hiding this comment.
s/with parsing/when parsing
tests.py
Outdated
| self.assertEqual(expected_docker_root, options.docker_directory) | ||
|
|
||
| def testDetectStorageFail(self): | ||
| """Tests that the DockerExplorer.DetectStorage function fails.""" |
There was a problem hiding this comment.
As a general rule I try to make dosctrings in tests more specific. Here I would specify under which conditions the functinon should fail: Tests that DockerExplorer.DetectStrorage fails when a nonexistent directory is specified
|
|
||
| @classmethod | ||
| def setUpClass(cls): | ||
| cls._setup('aufs', aufs.AufsStorage) | ||
|
|
||
| def testGetAllContainers(self): | ||
| """Tests the GetAllContainers function on a AuFS storage.""" |
There was a problem hiding this comment.
I'm not super familiar with testing but it seems this test case bundles a bunch of different tests: testing name, timestamp, image name, and length of the returned items
There was a problem hiding this comment.
Yes.
I "test" one function per test here. Probably not the best.
filed #24 to deal with this later
| 'sha256:{0:s}'.format(self.image_id)) | ||
| layer_info = self.de_object.storage_object.GetLayerInfo( | ||
| 'sha256:' | ||
| '7968321274dc6b6171697c33df7815310468e694ac5be0ec03ff053bb135e768') | ||
| self.assertEqual('2017-01-13T22:13:54.401355854Z', layer_info['created']) | ||
| self.assertEqual(['/bin/sh', '-c', '#(nop) ', 'CMD ["sh"]'], | ||
| layer_info['container_config']['Cmd']) | ||
|
|
||
| def testShowRepositories(self): | ||
| """Tests the Storage.ShowRepositories function on a AUFS storage.""" |
There was a problem hiding this comment.
"Test that ShowRepositories returns a correctly formatted info string"
There was a problem hiding this comment.
See #24
I'll make test docstrings more explicit at the same time I split unit tests.
tests.py
Outdated
| @@ -75,7 +75,8 @@ def testParseArguments(self): | |||
| self.assertEqual(expected_docker_root, options.docker_directory) | |||
|
|
|||
| def testDetectStorageFail(self): | |||
| """Tests that the DockerExplorer.DetectStorage function fails.""" | |||
| """Tests that the DockerExplorer.DetectStorage function fails on | |||
There was a problem hiding this comment.
Is there a word missing ? ("nonexistent")
Couple things happening here