Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-10-02
14:09:55 efried because when you pass an iterable to side_effect, it's return values, not callables.
14:10:00 dansmith nope
14:10:07 dansmith return_value behaves like that, side_effect does not
14:10:28 efried side_effect=one_thing calls one_thing
14:10:28 efried side_effect=[one, two] *returns* one followed by two
14:10:33 gibi dansmith: btw, mocking f through the instance doesn't work either http://paste.openstack.org/show/780755/
14:10:54 dansmith efried: "If side_effect is an iterable then each call to the mock will return the next value from the iterable."
14:11:01 efried ^
14:11:56 dansmith efried: https://pastebin.com/bLpt6Skz
14:12:22 dansmith gibi: it's all about calling convention, it's tricky and magical, but you're not doing anything different other than the ordering
14:12:46 efried dansmith: yup, that didn't print "Foo ran 1"
14:13:12 efried try to get it to do that the first time, and raise the second
14:13:13 gibi dansmith: tell me how can I call the original with side_effect if the original is a bound method because I failed to find a way
14:13:26 efried gibi: you can't
14:13:27 dansmith efried: it printed 1, my local env has a different foo
14:14:13 dansmith efried: wait, what are you arguing? that it didn't run foo? I didn't ask it to
14:14:22 efried I know. And you can't.
14:14:36 efried that's what spy helps with
14:14:54 gibi exaclty what efried is asking is what I'm after. having the original function is called while spying on the call
14:15:07 dansmith ah, I see what you mean.. a lambda in the list will solve that
14:15:20 gibi dansmith: no, the lambda would be returned, not called
14:15:29 efried ^
14:15:38 dansmith gibi: having the original called is what side_effect=callable will do, but the list, fine fine, I see
14:16:20 gibi dansmith: but I cannot do that^^ if the original is a bound method as mock does not pass proper arglist to the callable
14:16:42 dansmith you guys seem to be describing a world where spy will make it easier to not write terrible tests like what efried is adding (and what he replaced), yet he's using spy to (if called once, then explode)
14:16:56 efried what did I write?
14:17:10 dansmith efried: this: https://review.opendev.org/#/c/685950/3/nova/tests/functional/regressions/test_bug_1843090.py
14:17:40 efried gibi gets credit for those
14:17:53 efried I think
14:17:55 dansmith oh sorry, I thought you were commenting on your own
14:18:37 dansmith I should have provided a url instead of a name :)
14:18:38 gibi dansmith: I want to make the test in test_bug_1843090.py better with spy. If there is other way to make it better then I'm open to suggestion
14:19:07 dansmith gibi: okay, but you're not in that change, right? you're still counting calls and failing once it's been called
14:19:19 dansmith and doing gross stuff like mangling the original object to tell if you've been called twice
14:19:59 dansmith so where is your call,fail bit you said was easy with spy?
14:20:00 gibi dansmith: we need the logic like first call fails to trigger a re-schedule but the second succeds so that the re-schedule succeeds.
14:20:01 artom Actually yeah
14:20:19 dansmith fine, where is your fail,call ? :)
14:20:20 gibi dansmith: is there a nicer way to code that?
14:20:26 artom What we're doing in https://review.opendev.org/#/c/685950/3/nova/tests/functional/regressions/test_bug_1843090.py is exactly what we said we shouldn't do in https://review.opendev.org/#/c/685950/2/nova/tests/functional/compute/test_live_migration.py@23
14:21:13 dansmith gibi: well, for one thing you should keep track of your mock calls outside of the runtime objects you're handling
14:21:37 efried dansmith: Checking the actual mock's call_count is hard because you have to define the method before you create the mock before you define the method, but you have to reference the mock from within the method.
14:21:44 dansmith gibi: point being I'm not sure what spy is making nicer about that test because it seems like the same ickiness, but with a different calling convention
14:22:03 gibi dansmith: it hides the scope magic efried is just described
14:22:15 gibi dansmith: it automates the call to the original function
14:22:16 dansmith efried: everywhere else we keep a list above the scope and chuck things into there inside the handler
14:22:36 dansmith gibi: so it saves the one line of saving a reference to the original method?
14:22:50 gibi dansmith: and it hides the outer scope list
14:23:14 dansmith but replaces it with the terrible hack of modifying the runtime objects instead of keeping that fully within the test!
14:23:17 efried and when there are multiple calls to the same method nested inside whatever's happening inside the context manager, it gives you a way to introspect them individually
14:23:31 gibi dansmith: and it remembers that we have to use new= instead of side_effect if the mocked function is bound method
14:23:55 efried with mock.patch:
14:23:56 efried do_thing()
14:23:56 efried do_thing calls foo() N times with different args or whatever
14:23:57 openstackgerrit Matt Riedemann proposed openstack/nova master: Add functional regression test for migrate part of bug 1781286 https://review.opendev.org/686017
14:23:57 openstack bug 1781286 in OpenStack Compute (nova) "CantStartEngineError in cell conductor during reschedule - get_host_availability_zone up-call" [Medium,In progress] https://launchpad.net/bugs/1781286 - Assigned to Matt Riedemann (mriedem)
14:23:57 openstackgerrit Matt Riedemann proposed openstack/nova master: Add Selection.availability_zone field https://review.opendev.org/685807
14:23:57 openstackgerrit Matt Riedemann proposed openstack/nova master: Set Instance AZ from Selection AZ during build reschedule https://review.opendev.org/686047
14:23:57 openstackgerrit Matt Riedemann proposed openstack/nova master: Set Instance AZ from Selection AZ during migrate reschedule https://review.opendev.org/686050
14:23:58 openstackgerrit Matt Riedemann proposed openstack/nova master: Update cells v2 up-call caveats doc https://review.opendev.org/686053
14:24:42 efried dansmith: I also suggested a way we don't have to do the
14:24:42 efried spy.claim_calls_on_node = getattr(spy, 'claim_calls_on_node', [])
14:24:42 efried thing, which puts creation of the tracker container back in the hands of the caller.
14:25:07 efried same #LOC, just a little more scrutable
14:25:13 dansmith efried: yes, initializing that "context" elsewhere would be a lot better
14:25:59 dansmith anyway, it seems to do a lot for you and gibi, that's all that matters. it's something else I have to learn to comprehend the test, but...
14:26:09 efried gibi: I can work that ^ up if you like, at least for demonstration purposes.
14:26:19 efried maybe it'll read more intuitively to dansmith
14:26:59 dansmith no, not more intuitively, it'll just make the handler less gross
14:28:37 mriedem i found out yesterday that we patch mock.patch globally using something in oslotest for fixing the autospec'ing stuff - what claudiub worked on a long time ago. i saw those changes merge but didn't realize we were patching the mock library.
14:28:47 mriedem i would not be surprised when we go to a new version and that blows up in our faces
14:29:06 openstackgerrit Merged openstack/nova stable/train: Reduce scope of 'path' query parameter to noVNC consoles https://review.opendev.org/686066
14:31:46 gibi efried, dansmith: fine. I'm out of steam. I think I get that we cannot remove the full uglyness of these tests. I think we can move and contain some the uglyness. But I also get that it makes the test less explicit. so meh
14:32:28 dansmith gibi: I appreciate your efforts to improve :)
14:32:32 mriedem gibi: if it makes you feel better, i was able to use mock.patch context manager in that migrate reschedule functional test :)
14:32:44 gibi mriedem: I haven't tried mock.patch(wraps=) recently. so it might work
14:32:58 mriedem using wraps= is awkward in my experience
14:33:09 mriedem side_effect is more straightforward, but maybe that's just b/c i have more experience with it
14:33:17 gibi mriedem: and patching the mock library feels baaaad
14:33:42 mriedem we'll find out about ^ but this is what we call https://github.com/openstack/oslotest/blob/master/oslotest/mock_fixture.py#L187
14:34:10 gibi dansmith: I guess I will go and fined another tech debt and work on that a bit :)
14:34:21 gibi s/fined/find/
14:36:24 gibi mriedem: that patching might even be relevant to my problem with side_effect and bound methods as it touches 'self'
14:36:44 mriedem too much touching of self can lead to problems
14:36:47 mriedem i've heard
14:36:48 mriedem ...
14:36:52 gibi ...
14:38:05 dansmith mriedem: comment here: https://review.opendev.org/#/c/686017/3
14:39:52 mriedem i can probably keep a handle to the mock and assert it's called, but if you remove the mock the test fails so i'm not sure it's worthwhile
14:41:07 mriedem actually as written i should confirm that statement,
14:41:23 mriedem because it probably fails anyway since the stub always raises right not, not just on the first host
14:41:29 mriedem *now
14:42:43 mriedem dansmith: replied inline with options
14:42:58 dansmith mriedem: that's why I'm asking.. just seems like since we can't see the actual failure in the fault, we should make sure it's hitting what we think it is
14:44:00 dansmith mriedem: I'd rather the explicit assertCalled personally, but it was just a suggestion anyway
14:50:43 mriedem ok i can tinker with it, gonna be a bit since i'm in a meeting in 10 minutes
14:52:26 dansmith ack, I dropped a few other comments on that series,
14:52:45 dansmith but overall..thanks for closing that loop.. it always seemed like a big deal so I never wanted to even start, but you made it look easy
14:59:32 mnaser sean-k-mooney: cpu/libvirt-y related question, is it possible that not all flags are passed down to the vm even with host-passthrough ?
15:00:56 sean-k-mooney i belive that yes there are 1 or 2 instruction that are not passsed but in general no

Earlier   Later