| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-05-29 | |||
| 10:34:56 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: DNM:test https://review.opendev.org/661931 | |
| 10:48:35 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: DNM:test https://review.opendev.org/661931 | |
| 10:51:22 | mdbooth | kashyap: Are you going to respin https://review.opendev.org/#/c/639091/ quickly? | |
| 10:51:50 | mdbooth | kashyap: Looks like you could fix up the nits in 2 minutes, and not much point wasting CI resources. | |
| 10:51:52 | kashyap | mdbooth: Yeah, trying the reg-ex thingie | |
| 10:52:05 | mdbooth | kashyap: I proposed a simpler alternative | |
| 10:52:10 | kashyap | Ah, let me look | |
| 10:52:19 | kashyap | No need to muck with regex'es then | |
| 10:52:31 | kashyap | Thanks for the review! | |
| 10:53:38 | kashyap | mdbooth: Relatd aside: would you be amenable if I simply do (instead of hard-coding a PID): | |
| 10:53:41 | kashyap | str=("Failed to terminate process \d+ with SIGKILL: Device " | |
| 10:53:43 | kashyap | "or resource busy") | |
| 10:54:31 | mdbooth | kashyap: Does it matter? | |
| 10:54:36 | aspiers | kashyap: then you'd need an exc_template | |
| 10:54:45 | kashyap | mdbooth: It doesn't | |
| 10:54:55 | mdbooth | kashyap: Surely you could write "Error: abducted by aliens" | |
| 10:55:00 | kashyap | Heh | |
| 10:55:03 | aspiers | exc_template % random_pid for raising, and exc_template % '\d+' for matching | |
| 10:55:20 | openstackgerrit | Guo Jingyu proposed openstack/nova master: Define and catch QemuGuestAgentNotRunning for set-password failure https://review.opendev.org/661466 | |
| 10:55:42 | aspiers | The important thing there is not so much the contents of the string, as ensuring that the test case isn't accidentally catching a totally different exception | |
| 10:56:16 | mdbooth | Right. We especially don't want to go asserting that libvirt is generating particular error text for us. | |
| 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 = self.assertRaises(fakelibvirt.libvirtError, drvr._destroy, | |
| 11:08:01 | mdbooth | instance) | |
| 11:08:01 | mdbooth | + self.assertEqual(fakelibvirt.VIR_ERR_SYSTEM_ERROR, | |
| 11:08:01 | mdbooth | + raised.get_error_code()) | |
| 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 | |