| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-07-27 | |||
| 11:52:32 | gibi | lets wait for stephenfin to chime in. Also I can try to look into it tomorrow to see how big is the issue | |
| 11:52:36 | gibi | then we can reconsider | |
| 11:52:48 | sean-k-mooney | we might need to add a helper funciton that check if its mocked and only create a new mock if not | |
| 11:52:59 | gibi | I would go and remove the duplicates if possible | |
| 11:53:08 | gibi | but I don't know how hard that will be | |
| 11:53:17 | sean-k-mooney | yes if we can do that once | |
| 11:53:25 | sean-k-mooney | then we will never hit this again because it will fail | |
| 11:53:35 | gibi | the helper feels like a coverup | |
| 11:54:00 | sean-k-mooney | im just worried about loading the context of is this mocked | |
| 11:54:27 | sean-k-mooney | again with the new behaivor it shoudl be obvious i guess because it will fail | |
| 11:54:34 | gibi | yeah | |
| 11:56:48 | sean-k-mooney | i just hope this does not cause shared global state | |
| 11:57:11 | sean-k-mooney | the fixture should be set up for every test | |
| 11:57:37 | sean-k-mooney | but we likely will have to replace part of them in the test where its double mocking | |
| 11:58:02 | sean-k-mooney | as i feel like we double mock somethimes when the fixture does not provide exactly what we want | |
| 11:58:09 | sean-k-mooney | i know i have done that at least once | |
| 11:58:32 | gibi | yes, I think that is the "normal" case when we double mock. I did that before | |
| 11:58:42 | gibi | so in this case the fixture should expose the mock | |
| 11:58:54 | gibi | so the test case can use it instead of remocking it | |
| 11:59:18 | sean-k-mooney | well we can jsut create a new mock object and assign it | |
| 11:59:27 | sean-k-mooney | instaed of having it patch it again | |
| 11:59:40 | gibi | we cannot create the new Mock as autospec will fail | |
| 11:59:54 | gibi | as it will try to spec what is already mocked | |
| 12:00:08 | sean-k-mooney | we can with mock.Mock() | |
| 12:00:13 | gibi | hm | |
| 12:00:14 | sean-k-mooney | we dont tend to use autospec much | |
| 12:00:29 | sean-k-mooney | we do in somecases but i rarely do | |
| 12:01:14 | gibi | A().foo = mock.Mock() is a monkeypatch that is not reverted automatically so that is a bad pattern | |
| 12:01:43 | gibi | also I think there is automatic autospecing in the mock lib (obviously not for mock.Mock() but for mock.patch decorator) | |
| 12:02:09 | sean-k-mooney | i dont think there is | |
| 12:02:14 | sean-k-mooney | or at least | |
| 12:02:21 | sean-k-mooney | its behavior shoudl be the same as the standard lib | |
| 12:02:28 | sean-k-mooney | so if there is it shoudl be in both | |
| 12:02:48 | sean-k-mooney | gibi: in the A().foo = mock.Mock() | |
| 12:03:08 | sean-k-mooney | we shoudl not need to revert that automaticlly if A is a mock already | |
| 12:03:52 | gibi | ahh, yeah, I ment there is automatic autospec in both lib and stdlib | |
| 12:04:56 | gibi | sean-k-mooney: yeah, if A().foo is already a mocked then that mock's patcher.stop() will revert the change. But the caller doing the A().foo = mock.Mock() does not know if foo is a Mock. So we establish a dangerous pattern | |
| 12:05:37 | gibi | if that pattern is used to other than a mocked field the we will leak global state between tests | |
| 12:06:22 | sean-k-mooney | gibi: right but we would only do that for the double mock case | |
| 12:06:40 | sean-k-mooney | but i agreee in general its bad | |
| 12:06:49 | sean-k-mooney | we have leaked state that way in the past | |
| 12:06:51 | gibi | yes, now, but then the next new dev came look at the code and think that A().foo = mock.Mock() is a generic pattern | |
| 12:07:05 | sean-k-mooney | where a mock was assinged to a stdlib function | |
| 12:07:25 | sean-k-mooney | so that is why i was wondering if we need a nova helper fucniton | |
| 12:07:34 | sean-k-mooney | to basiclaly always do the right thing | |
| 12:07:50 | gibi | but that mean all our mocking need to go through that helper | |
| 12:07:54 | gibi | that will be a massive change | |
| 12:08:19 | sean-k-mooney | perhaps i need to think about what pattern we can actully use that will work | |
| 12:08:36 | gibi | anyhow I'd like to do a non theorethicaly investigation with this | |
| 12:08:40 | sean-k-mooney | perhaps using the mock as a context manager | |
| 12:08:49 | sean-k-mooney | but not sure if that would still fail for the double mocking case | |
| 12:09:04 | gibi | btw this is why I remembered automatic autospeccing https://review.opendev.org/q/message:autospec+project:openstack/nova+status:merged | |
| 12:09:53 | sean-k-mooney | sure but that not automatic | |
| 12:10:06 | sean-k-mooney | that was exiplcitly enabling it and new right | |
| 12:10:11 | gibi | yeah, I missremembered | |
| 12:10:16 | sean-k-mooney | oh ok | |
| 12:10:54 | gibi | I will look at this mock thing tomorrow | |
| 12:11:10 | gibi | now I'm in a middle fighting with reshape + provider generation conflict | |
| 12:12:12 | gibi | I'm in the denial phase where I think my code is correct and reshape is broken :D | |
| 12:12:46 | sean-k-mooney | :) | |
| 12:13:08 | sean-k-mooney | maybe but if thats the case you will probly have to fix it in placment yes | |
| 12:13:17 | sean-k-mooney | so hopefully the issue is in your current code | |
| 12:13:21 | gibi | I know :D | |
| 12:13:24 | sean-k-mooney | that feels like less work | |
| 12:13:25 | gibi | hence "denial" | |
| 12:13:54 | gibi | anyhow I think this is the first case when we both create new RPs and reshape allocations at the same time | |
| 12:14:08 | gibi | from nova | |
| 12:14:55 | gibi | and RP creation is outside of the reshape code | |
| 12:15:02 | sean-k-mooney | if nova has not done it its likely no one else has | |
| 12:15:17 | sean-k-mooney | i dont think neutron or cyborg have used reshapes before | |
| 12:16:28 | gibi | me neither | |
| 12:50:54 | bauzas | I just capture the discussion | |
| 12:51:10 | bauzas | is it because unittest.mock is not on par with our mock lib ? | |
| 12:52:16 | gibi | it is that in py310 unittest.mock introduced a breaking change. The mock lib is a rolling backport of the upstream unittest.mock so at some point this change will appera in the standalone mock lib too | |
| 12:52:19 | kashyap | bauzas: Please put the stable/wallaby backport through: https://review.opendev.org/c/openstack/nova/+/851206 | |
| 12:52:34 | kashyap | gibi ACKed it too; and you've ACKed the upstream main | |
| 12:53:39 | gibi | kashyap: we need to land the backports in order | |
| 12:53:44 | gibi | so we land the yoga first | |
| 12:53:48 | gibi | then xena | |
| 12:53:50 | gibi | then wallaby | |
| 12:54:03 | kashyap | gibi: Oh, yes. I mean to also implicitly look at 'em. :) | |
| 12:54:11 | gibi | ack | |
| 12:54:21 | bauzas | gibi: which breaking change ? | |
| 12:54:53 | gibi | if you mock a function twice, the second mocking attempt will fail | |
| 12:55:05 | gibi | as autospecing a mock is disallowed now | |
| 12:55:48 | bauzas | then we should wait for our mock lib | |
| 12:55:57 | gibi | what? why? | |
| 12:56:19 | gibi | why it is better to get the same breaking change later? | |
| 12:56:20 | bauzas | we should change the related tests first due to this | |
| 12:57:04 | bauzas | gibi: well, once our mock lib would be modified, then the CI wouldn't work and we would see it so we could not accept the new release | |
| 12:57:16 | bauzas | then we could modify the tests | |
| 12:57:23 | bauzas | for this new release | |
| 12:57:32 | bauzas | and then accepting the new release | |
| 12:58:07 | gibi | this is the same thing. If we want to accept py310 it means we need to accept all the breaking changes there. | |
| 12:59:13 | gibi | so we are only debating when to fix those duplicated mocks | |
| 12:59:27 | bauzas | at least we need to modify the only needed tests | |
| 12:59:29 | gibi | and as the mock switch is ready now I think we should fix it now | |
| 12:59:40 | bauzas | before changing to use unittest.mock | |
| 13:00:17 | gibi | why before? we can switch to unittest.mock and then fix the duplicated mock to get the py310 non voting job gren | |
| 13:00:20 | gibi | green again | |