| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-05-29 | |||
| 10:56:18 | aspiers | assertRaises(fakelibvirt.libvirtError, ...) doesn't guarantee that it's a VIR_ERR_SYSTEM_ERROR | |
| 10:56:28 | aspiers | (IIUC) | |
| 10:56:41 | aspiers | and also that int1=errno.EBUSY | |
| 10:57:13 | aspiers | so six.assertRaisesRegex can function as a substitute for those two checks | |
| 10:57:19 | mdbooth | aspiers: My suggestion was to assert that the error raised is the identity of the fake error | |
| 10:57:32 | mdbooth | i.e. we raised literally the expected error object | |
| 10:57:50 | aspiers | mdbooth: yeah, that probably sounds like the best option to me | |
| 10:58:00 | aspiers | how do you code that though? | |
| 10:58:08 | mdbooth | assertRaises(ex, ...) | |
| 10:58:33 | aspiers | oh, that easy :) | |
| 10:58:36 | mdbooth | Actually, lemme try something. Would that work if I copied it? | |
| 10:58:45 | aspiers | I thought the first parameter had to be the exception _class_ | |
| 10:58:58 | aspiers | the docs are kinda unclear on this | |
| 10:59:01 | kashyap | mdbooth: Hmm, I tried the assertRaises(ex, ...) it fails with: | |
| 10:59:05 | kashyap | TypeError: issubclass() arg 2 must be a class or tuple of classes | |
| 10:59:10 | aspiers | right | |
| 10:59:30 | aspiers | the prose suggests you can use the exception instance, but the examples suggest otherwise | |
| 10:59:35 | mdbooth | kashyap: I only ran it under py37, so maybe. | |
| 10:59:56 | mdbooth | I hate python | |
| 11:00:00 | aspiers | ;) | |
| 11:00:01 | kashyap | Sigh | |
| 11:00:12 | aspiers | yeah, Python APIs aren't the best | |
| 11:00:19 | aspiers | biab | |
| 11:00:41 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: DNM:test https://review.opendev.org/661931 | |
| 11:01:09 | kashyap | mdbooth: Do you have a preference given that the above goes bonkers on non-PY37? | |
| 11:01:20 | kashyap | s/preference/preference,/ | |
| 11:01:51 | mdbooth | kashyap: My test was bogus... | |
| 11:02:15 | kashyap | Ah | |
| 11:02:28 | aspiers | I figured it out | |
| 11:02:35 | kashyap | Please educate | |
| 11:02:40 | aspiers | with self.assertRaises(ex) as cm: | |
| 11:02:49 | aspiers | ... do assertions on cm.exception ... | |
| 11:02:56 | mdbooth | raised = self.assertRaises(fakelibvirt.libvirtError, ...) | |
| 11:03:12 | mdbooth | self.assertEqual("Foo", raised.foo) | |
| 11:03:36 | aspiers | Oh, context manager only works with >= 3.1 :-/ | |
| 11:03:52 | mdbooth | py27 | |
| 11:04:08 | aspiers | Although IIRC maybe we don't use native py27 unittest? | |
| 11:04:40 | mdbooth | I think it inherits at some point | |
| 11:04:51 | aspiers | testtools>=2.2.0 | |
| 11:04:56 | aspiers | in test-requirements.txt | |
| 11:05:17 | aspiers | https://testtools.readthedocs.io/en/latest/for-test-authors.html#improved-assertraises | |
| 11:05:29 | aspiers | "Note that this is incompatible with the assertRaises in unittest2 and Python2.7." | |
| 11:06:33 | mdbooth | kashyap: Sec... | |
| 11:06:42 | aspiers | I would try the approach in that link | |
| 11:06:47 | aspiers | which is basically what mdbooth suggested | |
| 11:06:53 | kashyap | mdbooth: Nod | |
| 11:07:21 | aspiers | I suspect his suggestion only works *because* we are using testtools not native unittest | |
| 11:07:30 | aspiers | but yeah, we'll see in a few seconds :) | |
| 11:07:47 | mdbooth | ...works | |
| 11:07:49 | mdbooth | code is: | |
| 11:07:51 | aspiers | \o/ | |
| 11:08:01 | mdbooth | + raised.get_error_code()) | |
| 11:08:01 | mdbooth | + self.assertEqual(fakelibvirt.VIR_ERR_SYSTEM_ERROR, | |
| 11:08:01 | mdbooth | instance) | |
| 11:08:01 | mdbooth | ~ raised = self.assertRaises(fakelibvirt.libvirtError, drvr._destroy, | |
| 11:08:22 | mdbooth | you can add more | |
| 11:08:37 | mdbooth | ^^^ tested on py(2|3)7 | |
| 11:09:37 | kashyap | mdbooth: Thanks! Let me try | |
| 11:12:00 | kashyap | mdbooth: That get_error_code() needs a mock, no? | |
| 11:12:23 | kashyap | @mock.patch.object(fakelibvirt.libvirtError, 'get_error_code') | |
| 11:12:26 | mdbooth | kashyap: No | |
| 11:12:56 | mdbooth | kashyap: I didn't look, but it must already be implemented by fake libvirtError | |
| 11:13:16 | mdbooth | It certainly works and returns the expected value | |
| 11:19:51 | kashyap | mdbooth: You're of course correct | |
| 11:27:28 | kashyap | mdbooth: Before I take CI resources, does it look OK to you: http://paste.openstack.org/show/752232/ | |
| 11:30:31 | kashyap | aspiers: BTW, on self.assertTrue(mock_warning.called) vs. mock_warning.assert_called_once() -- I went with the former as that seemed to be the "pattern" | |
| 11:30:53 | aspiers | OK | |
| 11:31:01 | aspiers | I don't really mind much either way | |
| 11:31:26 | aspiers | Latter feels a bit more idiomatic to me (after all, presumably they implemented it for a reason) but whatever :) | |
| 11:31:57 | kashyap | Yeah, I see what you mean, though. I like the latter less obtuse | |
| 11:46:24 | openstackgerrit | Kashyap Chamarthy proposed openstack/nova master: libvirt: Rework 'EBUSY' (SIGKILL) error handling code path https://review.opendev.org/639091 | |
| 11:50:01 | kashyap | aspiers: BTW, I fully share your horror of super-long methods; I just made an elaborate TODO to address it into 3 other refactor commits, based on your remarks. | |
| 11:52:46 | cdent | (super-long {methods,packages,tests,files,documents})-- | |
| 11:53:04 | kashyap | cdent: :-) | |
| 11:53:26 | cdent | but | |
| 11:53:31 | cdent | (super-log emails)++ ;) | |
| 11:53:39 | cdent | damn: long! | |
| 11:54:10 | kashyap | mdbooth: Hopefully I addressed "all things", including Adam's nits. Hope that one test suffices? Or do we need _another_ test? (I don't want to add more without a very good reason) | |
| 12:34:05 | openstackgerrit | François Palin proposed openstack/nova stable/rocky: Include all network devices in nova diagnostics https://review.opendev.org/661962 | |
| 12:40:09 | openstackgerrit | Stephen Finucane proposed openstack/nova master: docs: Don't version links to reno docs https://review.opendev.org/661967 | |
| 12:52:48 | openstackgerrit | Kashyap Chamarthy proposed openstack/nova master: libvirt: Rework 'EBUSY' (SIGKILL) error handling code path https://review.opendev.org/639091 | |
| 12:55:49 | kashyap | mdbooth: Addressed the two nits ^^; can I have my +1 back? :D | |
| 12:55:51 | kashyap | (Thanks!) | |
| 12:57:44 | artom | sean-k-mooney, tempest-full-py3/nova-multinode all passed on top of https://review.opendev.org/#/c/644881/18 | |
| 12:58:17 | artom | Need to fix unit tests now | |
| 12:58:42 | artom | Unless mriedem or dansmith want to sanity check the approach to make sure it's acceptable first | |
| 12:58:50 | artom | (The revert resize thing) | |
| 13:11:32 | aspiers | kashyap: great :) | |
| 13:12:30 | openstackgerrit | François Palin proposed openstack/nova stable/rocky: Include all network devices in nova diagnostics https://review.opendev.org/661962 | |
| 13:15:12 | sean-k-mooney | artom well its a race or was so you might want to do a few rechecks to confirm but cool ill take a look | |
| 13:16:24 | sean-k-mooney | artom: although im also plannign to change how hybrid plug works to fix a different issue... | |
| 13:17:58 | sean-k-mooney | i need to get back to working on this https://review.opendev.org/#/c/602432/ but if i end up altering the beahvior i geuss we could revert or update your change if hybrid plug nolonger matters | |
| 13:18:53 | sean-k-mooney | this is actully next on my todo list once i get the spec im working on finished | |
| 13:19:13 | kashyap | mdbooth: Haha, you caught me sneaking in the \d+ | |
| 13:19:45 | mdbooth | kashyap: It makes no difference :) | |
| 13:20:31 | kashyap | Yeah, indeed | |
| 13:20:49 | mdbooth | i.e. I don't care if you don't change it | |
| 13:24:44 | artom | sean-k-mooney, I don't think it was a race in this case | |
| 13:24:58 | artom | We were blocking on the wait before Neutron could ever send us the event | |
| 13:25:11 | artom | When waiting in the compute manager | |