| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-05-20 | |||
| 15:39:26 | efried | but | |
| 15:39:33 | efried | test_servers is way too effin big already. | |
| 15:39:34 | aspiers | efried: Ideally it should test not just that a) it provides the SEV trait on detection, but also b) it replaces it if the admin removes it, and that it removes it if c) the host isn't SEV-capable but an admin provides it or d) if the host *becomes* SEV-incapable | |
| 15:39:56 | aspiers | i.e. the same scenarios I tested when doing the cap-to-trait thing before | |
| 15:40:19 | aspiers | but this time with the trait coming from a different bit of code | |
| 15:40:23 | efried | ...And these are testing that libvirt reports traits, so LibvirtReportTraitsTests seems appropriate just based on the name. | |
| 15:40:37 | aspiers | Yeah, name-wise that wins for sure | |
| 15:40:52 | efried | aspiers: I understand. I wouldn't be averse to a patch that moves those tests :) | |
| 15:41:09 | aspiers | but yeah, you're right this needs to test the real libvirt driver | |
| 15:41:23 | efried | any patch that refactors test_servers.whatever into more-appropriately-named-smaller-module would be a win imo. | |
| 15:41:26 | aspiers | Moves which tests? | |
| 15:41:35 | efried | aspiers: The ones you did about capability traits | |
| 15:41:46 | aspiers | To where? They're not libvirt-specific | |
| 15:41:50 | efried | ahem, I guess those should... right | |
| 15:41:57 | efried | ...be in a module about drivers setting traits. | |
| 15:42:22 | efried | bah | |
| 15:42:34 | aspiers | How about this: | |
| 15:42:52 | efried | ++ (/me predicts future) | |
| 15:43:46 | aspiers | I find some co-conspirators from Red Hat, and stage a coup where the PTL gets overthrown. Oh good, +1 from you already | |
| 15:44:10 | efried | yeah, I walked into that | |
| 15:44:22 | aspiers | What I was actually gonna suggest is, I'll stick them in LibvirtReportTraitsTests, but if that means duplicating code already in test_servers.py, then I'll extract it out into a shared utils library | |
| 15:44:32 | aspiers | which might already exist | |
| 15:44:37 | aspiers | or might not | |
| 15:44:37 | efried | ++ | |
| 15:45:02 | efried | Doesn't have to be shared utils per se, just a separate module that tests trait-y things but not libvirt-specific. | |
| 15:45:08 | efried | Then the libvirty one can inherit from that. | |
| 15:45:08 | aspiers | right | |
| 15:45:29 | efried | Do the refactor in a new patch underneath, if you please. | |
| 15:45:49 | efried | If it's straightforward enough, I might be able to ninja approve it. | |
| 15:46:37 | aspiers | OK | |
| 15:47:30 | efried | yeah, test_servers is nearly 7kloc. That's too much | |
| 15:47:35 | aspiers | agreed | |
| 15:48:32 | aspiers | efried: should I first just extract TraitsTrackingTests to a separate file? | |
| 15:49:00 | efried | yeah, and TraitsBasedSchedulingTest with it | |
| 15:49:01 | aspiers | or that and ProviderTreeTests? | |
| 15:49:20 | aspiers | something like test_server_traits.py? | |
| 15:49:33 | bnemec | efried: Worth noting that the save_and_reraise context was to work around a bug in eventlet that I believe has since been fixed. | |
| 15:49:37 | aspiers | or test_server_provider_tree.py | |
| 15:49:47 | bnemec | I'm not sure you still have to worry about log calls and such messing with the exception context. | |
| 15:50:06 | efried | bnemec: oh, I didn't know that. It makes sense to me that intervening code could still muck with the exception context. | |
| 15:50:40 | efried | bnemec: or is well-behaved code supposed to not do that | |
| 15:51:16 | bnemec | It was _probably_ eventlet doing something naughty. | |
| 15:51:55 | bnemec | I'm sure it could still happen, but it's probably not common anymore. | |
| 15:53:14 | bnemec | (my overuse of probably is pretty representative of my confidence in this ;-) | |
| 15:53:36 | efried | adrianc: in any case, 'raise' rather than 'raise e' is appropriate | |
| 15:53:36 | efried | bnemec: So you're not yet recommending that we go through and rip out all our uses of that context manager? | |
| 15:53:38 | bnemec | I guess I'm not aware of anyone running into a bug that required save_and_reraise since the eventlet fix though. | |
| 15:54:15 | bnemec | efried: No, I don't think it hurts anything to have it there. | |
| 15:54:17 | efried | aspiers: test_servers_provider_tree seems like a winner. | |
| 15:54:24 | aspiers | ok | |
| 15:54:31 | bnemec | And +1 to the raise vs. raise e point. | |
| 15:58:13 | efried | Recent guidance is just to include the license. But legally, since you're porting code from A to B, you should preserve A's copyright and license header. Sorry. | |
| 15:59:43 | efried | It is stupid. I've actually spent hours with lawyers on this. | |
| 16:00:21 | aspiers | Haha my sympathies | |
| 16:03:41 | cdent | FIGHT THE POWER | |
| 16:10:12 | aspiers | :) | |
| 16:10:20 | openstackgerrit | Adam Spiers proposed openstack/nova master: Extract provider tree functional tests into new file. https://review.opendev.org/660157 | |
| 16:11:00 | aspiers | efried: ^^^ | |
| 16:11:04 | efried | on it | |
| 16:13:37 | aspiers | Why the hell did I finish the commit title with a period | |
| 16:13:56 | aspiers | I never do that | |
| 16:16:08 | efried | arght, /me retracts +2! | |
| 16:16:18 | efried | whatever | |
| 16:16:39 | aspiers | ;) | |
| 16:16:43 | efried | fix it or not. It's only your reputation at stake, permanently and indelibly etched into git | |
| 16:16:50 | aspiers | ROFL | |
| 16:17:22 | aspiers | I see plenty of other trailing '.' in the commit history, thankfully | |
| 16:17:38 | aspiers | I'll just be joining a distinguished group of disgraced engineers | |
| 16:17:56 | aspiers | Hmm, "Disgraced Engineer" sounds like a job title | |
| 16:18:05 | aspiers | Maybe that's in my career track | |
| 16:18:58 | aspiers | Crap, now I have no way to re-submit the SEV trait change without rebasing all the other SEV stuff on top of this | |
| 16:19:10 | aspiers | or without rebasing this on top of the other SEV stuff | |
| 16:19:18 | aspiers | This appears to be a flaw in the Gerrit model | |
| 16:20:03 | efried | aspiers: Rebasing the whole series should be as easy as rebasing one patch. | |
| 16:20:32 | efried | aspiers: except it looks like you may not have been done rebasing the series on itself. | |
| 16:20:54 | aspiers | From the CLI perspective maybe, but it creates a load of CI churn and also resets existing +1 / +2 | |
| 16:21:05 | aspiers | And it's hard enough to get those precious votes already | |
| 16:21:18 | efried | rebasing doesn't reset votes. | |
| 16:21:31 | efried | unless it involves actual code changes | |
| 16:21:41 | aspiers | Doesn't that depend on the patch-id remaining constant? ... right | |
| 16:21:58 | efried | Change-Id has to stay constant. | |
| 16:22:08 | aspiers | I mean the git patch-id | |
| 16:22:22 | efried | I don't know what that is. You mean the commit hash? | |
| 16:22:26 | aspiers | nope | |
| 16:22:28 | aspiers | https://git-scm.com/docs/git-patch-id | |
| 16:22:42 | efried | ah | |
| 16:22:50 | aspiers | It's how git does clever stuff during rebase, for instance | |
| 16:22:52 | sean-k-mooney | aspiers if its a commit message only change i thik it keeps previous votes | |
| 16:22:52 | efried | yes, in spirit. | |
| 16:23:06 | efried | sean-k-mooney: Pretty sure not ^ | |
| 16:23:08 | aspiers | sean-k-mooney: it's not commit message only | |
| 16:23:12 | aspiers | in this case | |
| 16:23:28 | aspiers | (We moved on from the trailing "." discussion to something more serious) | |
| 16:23:39 | efried | In this case you don't have any votes yet on the patch that's consuming the refactor, right? | |
| 16:23:50 | efried | because you're having to address some annoying core's -1 | |
| 16:24:30 | sean-k-mooney | aspiers: we dont actully enforce the no trail . in the hacking checks if i rememebr | |
| 16:24:40 | aspiers | https://review.opendev.org/#/c/638680/ depends on https://review.opendev.org/#/c/633855/ which depends on https://review.opendev.org/#/c/655268/ | |
| 16:24:56 | sean-k-mooney | so sinc its not enforced by the ci or atleast was not always enforced it is likely not consitent | |
| 16:25:04 | aspiers | Now I need https://review.opendev.org/#/c/638680/ to *also* depend on this test_servers.py refactoring | |
| 16:25:30 | efried | oh, I see, you want to make a downward-facing tree | |