Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-06-13
15:15:01 huanxie Hi jaypipes, sorry for my mistack, I mean I will fix your comments my next morning :)
15:15:21 jaypipes huanxie: no problem!
15:16:01 cfriesen andreykurilin: the tests in question use mock.MagickMock for novaclient.client.Client, and then later on some of the tests do @mock.patch('novaclient.client.Client'), and I can't figure out how to get them to report api_version properly
15:16:09 huanxie Thanks a lot jaypipes
15:16:36 andreykurilin cfriesen: are you talking about unit or functional tests?
15:17:06 cfriesen andreykurilin: unit. novaclient/tests/unit/test_shell.py
15:17:52 mriedem cfriesen: wouldn't you just put an api_version attribute on the mock?
15:19:42 cfriesen mriedem: the code in question does this in setUp(): self.useFixture(fixtures.MonkeyPatch('novaclient.client.Client', mock.MagicMock()))
15:20:12 cfriesen mriedem: I can't figure out how to put an api_version attribute on that
15:20:20 andreykurilin cfriesen: it looks like just that module requires some cleanup
15:20:38 mriedem cfriesen: mock.MagicMock(api_version=api_versions.APIVersion('2.1'))
15:20:40 mriedem or something
15:20:41 andreykurilin if setUp method uses fixtures, no @mock.patch is required
15:22:36 andreykurilin cfriesen: I suppose, we need to same mock.MagicMock() object as self.mock_client before transmitting to MonkeyPatch, and than you will able to do `self.mock_client.api_version = api_versions.APIVersion("2.22")` in your test
15:24:58 cfriesen andreykurilin: if you don't @mock.patch on the test itself, how do you do stuff like "client_kwargs = mock_client.call_args_list[0][1]" ?
15:25:31 andreykurilin cfriesen: as I mentioned previously, we need to store object transmitted to MockPatch in setUp method
15:25:59 cfriesen andreykurilin: I think I see. will try that
15:29:10 cfriesen andreykurilin: you are a genius. Looks like that'll work. let me respin a new version for review.
15:29:47 andreykurilin cfriesen: If you have time, please submit a patch with removing all redundant @mock.patch there
15:30:03 cfriesen andreykurilin: whoops, spoke too soon. TypeError: '<MagicMock name='mock().api_version' id='69241360'>' should be an instance of '<class 'novaclient.api_versions.APIVersion'>'
15:30:53 andreykurilin cfriesen: in your test, you should do something like `self.mock_client.api_version = api_versions.APIVersion("2.22")`` where api_versions is a module novaclient.api_versions
15:31:42 cfriesen andreykurilin: this is what I have now: http://paste.openstack.org/show/612435/
15:31:42 mriedem _CellProxy
15:31:43 mriedem oh _CellProxy
15:31:46 mriedem why oh why
15:33:18 cfriesen andreykurilin: and this is what I get when running it: http://paste.openstack.org/show/612437/
15:33:47 mriedem cfriesen: needs to be an object,
15:33:49 mriedem not a string
15:34:38 mriedem cfriesen: https://github.com/openstack/python-novaclient/blob/master/novaclient/client.py#L59
15:34:43 mriedem ^ is what you're mocking out ultimately
15:34:57 cfriesen mriedem: what needs to be an object?
15:34:58 mriedem so it needs to be of type novaclient.api_versions.APIVersion
15:35:09 mriedem cfriesen: self.mock_client.api_version = novaclient.API_MAX_VERSION
15:35:13 mriedem that's a string
15:35:15 mriedem needs to be an object
15:35:22 mriedem an APIVersion object
15:35:26 mriedem which is constructed with a string value
15:36:02 mriedem i would probably also use API_MIN_VERSION as the default
15:36:06 mriedem for existing tests
15:36:44 cfriesen yeah, that was my intent. copy/paste error just now
15:37:25 mriedem bauzas: i've got a fun cells v1 question for you
15:37:34 mriedem bauzas: i need to implement __contains__ for the _CellProxy
15:37:36 bauzas mriedem: kaboom
15:37:37 mriedem like https://github.com/openstack/nova/blob/master/nova/cells/utils.py#L64
15:37:41 mriedem i see that id and host are special cased
15:37:50 mriedem i was just doing this:
15:37:51 mriedem def __contains__(self, key):
15:37:51 mriedem return key in self._obj
15:37:56 mriedem but do i need to special case id and host?
15:38:11 mriedem i don't think so, but wanted to ask
15:38:37 bauzas mriedem: I need to reload the context in mind because it's a 2-yr blame :p
15:39:11 mriedem i think we special case id and host because they are namespaced
15:39:12 mriedem to the cell
15:39:28 mriedem whether they are IN the proxied object is the same either way i think
15:41:00 bauzas mriedem: oh right I remember
15:41:01 bauzas mriedem: now
15:41:30 bauzas AFAIR, we're amending the host value by the cell path
15:41:43 bauzas the same goes with the ID
15:41:55 bauzas something like 'foo' becomes cell1!foo AFAIR
15:42:13 bauzas which was a problem for our object layers
15:42:20 bauzas at least the id
15:42:48 bauzas so, we wrote those ugly patterns to make sure we can just modify both the ID and the host fields without touching the object
15:43:48 mriedem yeah, so i think the answer is my simple __contains__ impl is fine
15:43:50 mriedem for what i need
15:43:57 mriedem which is just checking if a field is set in the wrapped object
15:44:08 mriedem it's a proxy to the ovo base __contains__
15:44:26 bauzas I'm thinking of your question
15:44:33 bauzas whether we should special case id and host
15:44:44 bauzas I think no
15:44:50 bauzas because we have the properties
15:46:59 bauzas mriedem: ^
15:49:31 bauzas mriedem: the thing is, I'm thiking of the precedence between __getitem__ and __contains__
15:50:33 bauzas mriedem: but if you ask : 'id' in mycellproxy
15:50:39 mriedem bauzas: __contains__ is used before __getitem__
15:50:43 bauzas it should provide True anyway
15:50:45 mriedem which is why i needed to add it
15:50:59 mriedem see http://logs.openstack.org/99/471899/5/check/gate-nova-python27-ubuntu-xenial/7be94a3/testr_results.html.gz
15:51:12 mriedem that was calling __getitem__ with key=0
15:51:17 bauzas FWIW, the doc is https://docs.python.org/3/reference/datamodel.html
15:51:21 mriedem treating it as a list rather than a map (dict)
15:51:37 mriedem and getattr blows up with TypeError if you're passing a non-string
15:51:41 mriedem __contains__ fixes it
15:53:05 bauzas mriedem: not sure I get why you land with key=0 but okay
15:53:37 mriedem me neither
15:53:47 mriedem anyway, i think it's resolved,
15:53:51 mriedem forget i ever asked about this
15:54:16 bauzas well, okay :)
15:55:22 sfinucan sdague: Could you take a look at this and see if it makes sense? https://review.openstack.org/#/c/473224/
15:55:54 sdague sfinucan: with the version bump it should be fine
15:56:31 sfinucan sdague: so they should go in together?
15:57:00 sfinucan *in the same patch
15:57:06 sdague yeh
15:57:24 sdague I think there is going to need to be a unit test change at the same time
15:57:31 sdague I can pull them together if you like
15:57:45 cfriesen mriedem: andreykurilin: Still hitting the problem. http://paste.openstack.org/show/612439/ has my setUp() code, the test itself, and the pdb output while running the test.
15:58:45 cfriesen mriedem: andreykurilin: for some reason I can't do pdb in novaclient/v2/shell.py, it just hangs. wonder if that's related to the issue
15:59:31 andreykurilin it is strange, because I fetched your patch, applied changes in setUp method and everything works for me(at least test_default_endpoint_type passes)
15:59:55 cfriesen andreykurilin:did you change it from "flavor-list" to "list"?
16:00:12 andreykurilin ok, let me try
16:00:18 cfriesen in test_default_endpoint_type9)

Earlier   Later