Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-10-02
13:58:20 mriedem not documented very well
13:58:58 efried What spy does is allow you to keep track of things per invocation of the wrapped method, which wraps= doesn't
13:59:29 efried also selectively change the behavior
13:59:42 efried like "run normally the first time, raise the second time"
13:59:54 dansmith I don't see how it's different from side_effect
14:04:21 gibi dansmith: side_effect is not good for calling the original if the original is a bound method http://paste.openstack.org/show/780754/
14:05:02 dansmith gibi: that's because you're mocking the class not the object
14:05:15 dansmith I think you're just using your spy in a different order
14:05:27 dansmith if you mock.patch.object(a) I think you'll be fine
14:06:10 gibi dansmith: but in the functional test when we mock we tend not to have easy access to the instantiated objects just the classes
14:06:35 dansmith in that one scenario you mean, but not necessarily as a general rule
14:07:24 dansmith efried's use of spy in the patch above that does not seem to be very clean to me, especially for a supposed functional test
14:07:34 gibi dansmith: my goal is to want to get rid of (or contain) the scope magic and the original function handling in cases like https://github.com/openstack/nova/blob/bf37bec80baa527ac013dfaa7480ef2761ed2cb9/nova/tests/functional/regressions/test_bug_1843090.py#L100
14:07:59 dansmith but anyway, not saying it's not useful, it just seems like in a lot of cases, you could get away with base mock stuff instead of having to learn a new convention
14:08:11 efried oh, yeah, in the vast majority of cases
14:08:33 gibi spy is not a replacement for mock. There are cases where I think mock is hard to use to achive the same thing
14:08:43 gibi like the funct test I linket above
14:08:52 dansmith gibi: understand. using side_effect to provide a sequence of run,fail is more conventional than what either the old or new code in that patch is doing
14:09:02 dansmith assuming you can hook at the right spot to mock the object itself
14:09:40 efried dansmith: I don't think you can use side_effect to run,fail
14:09:46 dansmith efried: of course you can
14:09:53 dansmith efried: provide it a list and it will iterate on each call
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

Earlier   Later