| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-05-29 | |||
| 07:06:50 | openstackgerrit | Yongli He proposed openstack/nova-specs master: add spec "show-server-numa-topology" https://review.opendev.org/658716 | |
| 08:00:21 | kashyap | efried: aspiers: Thanks for the review on that os-traits patch. Will address it soon. | |
| 08:15:06 | openstackgerrit | guangchao.pan proposed openstack/nova master: Add a name for the automatically created volume https://review.opendev.org/660096 | |
| 09:12:32 | aspiers | kashyap: cool | |
| 09:48:12 | kashyap | aspiers: Still about? | |
| 09:48:19 | aspiers | yep | |
| 09:48:56 | kashyap | aspiers: When you get a moment, I'm stuck in "unit test rut", wonder if you can have a gander at this WIP (line 102): http://paste.openstack.org/show/752226/ | |
| 09:49:29 | kashyap | aspiers: The code it is testing is also pasted in the pastebin, for convenience. (It is in relation to this patch you reviewed in the past: https://review.opendev.org/#/c/639091/) | |
| 09:49:59 | kashyap | Maybe I should just post the WIP, broken unit test, so you could comment in Gerrit itself :D | |
| 09:51:09 | kashyap | Let me post it | |
| 09:52:02 | stephenfin | johnthetubaguy++ Thanks for running through those remove-cellsv1 patches | |
| 09:52:38 | openstackgerrit | Kashyap Chamarthy proposed openstack/nova master: libvirt: Rework 'EBUSY' (SIGKILL) error handling code path https://review.opendev.org/639091 | |
| 09:52:51 | kashyap | aspiers: ^ The "broken" unit test | |
| 09:53:24 | zigo | Is there a way to remove an already runningn instance from a server group? | |
| 09:55:34 | aspiers | kashyap: oh nice, didn't know it was possible to refer to abbreviated change-ids | |
| 09:57:04 | kashyap | aspiers: Yeah, I just tried it, based on commit ID trimming I do | |
| 09:57:36 | aspiers | kashyap: what's the FIXME for? | |
| 09:58:07 | aspiers | won't that fail flake8 or something? | |
| 09:58:16 | kashyap | aspiers: As I felt the test was not 100% accurate, although it "passes" | |
| 09:58:34 | aspiers | I think any FIXME needs an explanation | |
| 09:58:35 | kashyap | Wanted someone to double-check it; will remove it on next PS (as I need one more test) | |
| 09:58:57 | kashyap | Yes, yes. I was not going to put in a "naked FIXME" like that. It won't exist. | |
| 09:59:35 | aspiers | Oh, sorry I missed your previous messages | |
| 10:00:06 | aspiers | Nevertheless, I think I'm missing (i.e. probably forgot) some context here | |
| 10:00:42 | aspiers | How are you stuck? | |
| 10:01:05 | kashyap | aspiers: Err, not exactly 'stuck' (bad wording); but just needs a once-over of the test. Is the "mocking" correct, etc | |
| 10:01:13 | aspiers | Oh right | |
| 10:01:42 | aspiers | OK so this one is for testing the >= 4.7.0 behaviour? | |
| 10:02:39 | kashyap | aspiers: Right | |
| 10:03:05 | kashyap | aspiers: Both, test with and without MIN_LIBVIRT_BETTER_SIGKILL_HANDLING | |
| 10:03:18 | kashyap | aspiers: I.e. hit the warining of _SIGKILL_HANDLING isn't satisfied | |
| 10:03:29 | aspiers | I mean the test with the FIXME | |
| 10:05:15 | kashyap | Yeah, it is "supposed" to :D | |
| 10:13:06 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: DNM:test https://review.opendev.org/661931 | |
| 10:16:07 | openstackgerrit | Guo Jingyu proposed openstack/nova master: Define and catch QemuGuestAgentNotRunning for set-password failure https://review.opendev.org/661466 | |
| 10:26:32 | openstackgerrit | Chris Dent proposed openstack/nova master: Make all functional tests reusable by other projects https://review.opendev.org/657659 | |
| 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... | |