| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-03-31 | |||
| 19:02:24 | imacdonn | I assume we need to fix both | |
| 19:02:38 | fried_bunny | It's only getting that far because you're not mocking out the service. | |
| 19:03:24 | imacdonn | I guess I'm looking at it the other way around | |
| 19:04:06 | imacdonn | from my perspective, it's not getting far enough to actually try to use the service, because ksa is kicking back that MissingAuthPlugin, because it's not configured | |
| 19:05:56 | fried_bunny | okay, I think I see what you're getting at. But actually, I suspect MissingAuthPlugin is only happening *after* (and *because*) ksa already retrieved the version document. | |
| 19:07:22 | fried_bunny | Humor me and try that useFixture I mentioned above. You don't have to run the whole world with it - just try one suite | |
| 19:07:28 | imacdonn | huh .. interesting .. I guess I don't know enough about how ksa works | |
| 19:07:37 | fried_bunny | nobody does. | |
| 19:07:38 | fried_bunny | nobody. | |
| 19:07:46 | imacdonn | I already humo[u]red .. and it seems to work | |
| 19:07:52 | imacdonn | heh | |
| 19:08:39 | fried_bunny | excellent. Now, I don't think that's what we want to do here anyway; it's a bit of a big hammer. But it's good to know we're looking under the right rock. | |
| 19:09:14 | imacdonn | it does seem like the right thing to do (to have the tests need the placement service to be there) | |
| 19:09:17 | fried_bunny | or, actually... maybe it's not so bad. Let me take a closer look at that fixture. | |
| 19:09:52 | imacdonn | trying a full set of tests now .. just did that one simple one before | |
| 19:10:10 | imacdonn | so now failing on 6 .. better than 27 | |
| 19:12:27 | fried_bunny | show | |
| 19:15:23 | imacdonn | http://paste.openstack.org/show/718087/ | |
| 19:17:00 | imacdonn | in start_service(), the conditional is ... if name == 'compute' and self.USES_DB: | |
| 19:17:08 | fried_bunny | ah | |
| 19:17:20 | imacdonn | not sure what self.USES_DB is .... may need to move my fixture to aseparate conditional | |
| 19:18:26 | imacdonn | that fixed one of the 6, at least | |
| 19:18:54 | fried_bunny | The rest are in test_compute_mgr - you can just use that same line to instantiate the fixture in the setUp of that guy | |
| 19:18:55 | imacdonn | but not the second :) | |
| 19:19:11 | imacdonn | ok | |
| 19:19:50 | fried_bunny | ...which is bypassing start_service cause it actually wants to test the stuff the service fixture mocks out | |
| 19:21:32 | imacdonn | yeah, that makes sense | |
| 19:22:05 | imacdonn | ok, now down to 1 failing under test_compute_mgr .. I suspect that there's a specific test for the old-style config check | |
| 19:22:29 | imacdonn | actually no, it's failing differently | |
| 19:22:57 | imacdonn | http://paste.openstack.org/show/718088/ - haven't studied yet | |
| 19:24:16 | fried_bunny | well, that's the exact test case you're gonna want to tweak for your change. | |
| 19:24:48 | imacdonn | oh yeah, duh... I was kinda right the first time | |
| 19:24:55 | fried_bunny | and that exception is happening waaaay later than you want to let that sucker go. | |
| 19:25:08 | imacdonn | I was focused on the exception, and not the test it came from | |
| 19:25:24 | fried_bunny | That's the point at which you want to mock that .get to do horrible things and then assert that you raise PlacementNotConfigured. | |
| 19:25:25 | fried_bunny | Now | |
| 19:25:34 | imacdonn | right | |
| 19:25:47 | fried_bunny | you may wish to put the fixture into an instance var in your setUp | |
| 19:25:58 | fried_bunny | so that you can shove your mock side effect right into that guy. | |
| 19:26:16 | fried_bunny | Or you could just @mock.patch('....SchedulerReportClient.get') on your test method. | |
| 19:26:17 | fried_bunny | but | |
| 19:26:26 | fried_bunny | I'm not sure the latter will work because the PlacementFixture is monkey patching that guy. | |
| 19:26:32 | fried_bunny | Still might work | |
| 19:27:05 | fried_bunny | actually probably will, because otherwise those other tests doing same would be failing. Unless they're all doing mock.patch.object. Which actually - do whatever they're doing, and it should work. | |
| 19:28:43 | imacdonn | k, looking into that ... mock stuff is far from intuitive for me, so usually need to plagiarise ;) | |
| 19:30:25 | fried_bunny | using the right bit of the fixture is the proper way to do it, but it can be a pain to figure out what that is. | |
| 19:31:53 | fried_bunny | In this case you should just be able to decorate your test method with | |
| 19:31:53 | fried_bunny | @mock.patch('nova.scheduler.client.report.SchedulerReportClient.get') | |
| 19:31:53 | fried_bunny | def test_whatever(self, mock_get): | |
| 19:31:53 | fried_bunny | mock_get.side_effect = ...MissingAuthPlugin | |
| 19:31:53 | fried_bunny | self.assertRaises(PlacementNotConfigured, self.init_host) | |
| 19:31:58 | fried_bunny | kind of thing. | |
| 19:32:23 | fried_bunny | a mock.patch.object would be a lil more elegant I guess. For that you might have to use a context manager. | |
| 19:49:47 | imacdonn | k, I guess I have it working with mock.patch | |
| 19:56:57 | openstackgerrit | Eric Fried proposed openstack/nova master: Return anchor providers in a_r and p_s https://review.openstack.org/558014 | |
| 19:57:07 | fried_bunny | good deal. | |
| 19:57:24 | fried_bunny | You 'bout ready to post that puppy? | |
| 19:57:57 | imacdonn | have one more test to fix (not looked at it yet) .. and also pondering what to do with test_init_host_placement_ensures_default_config_is_unset | |
| 19:58:14 | fried_bunny | kill it. | |
| 19:58:16 | imacdonn | it's related, but ... sort of different | |
| 19:58:48 | imacdonn | you think I should just whack it in this change, or separately? | |
| 19:58:58 | fried_bunny | looking... | |
| 19:59:12 | fried_bunny | oh, kill it. | |
| 19:59:16 | fried_bunny | in fact... | |
| 19:59:22 | imacdonn | wondering if it has/had any other purpose | |
| 20:00:02 | fried_bunny | https://review.openstack.org/#/c/557086/ -- I thought that was touching that test case, but no. | |
| 20:00:33 | imacdonn | yeah, I was also surprised that https://review.openstack.org/#/c/554759/ didn't at least touch it | |
| 20:01:44 | imacdonn | so Im going to nuke it, on the assumption that its only purpose was to ensure the validity of the following test | |
| 20:04:54 | fried_bunny | probably just overlooked tbh | |
| 20:05:05 | imacdonn | yeah | |
| 20:05:36 | fried_bunny | on that note, you can look around and remove all the places we set placement.[os_]region_name now. | |
| 20:05:43 | fried_bunny | ...in the test suites. | |
| 20:07:55 | imacdonn | not actually seeing any (others) | |
| 20:08:21 | imacdonn | well, there's this... nova/tests/unit/conf_fixture.py: self.conf.set_default('region_name', 'RegionOne', group='placement') | |
| 20:13:09 | fried_bunny | yup, don't need that anymore. | |
| 20:14:41 | imacdonn | agreed ... removed it already, and running all tests again | |
| 20:15:15 | imacdonn | d'you think we need comments at all the places where we made the placement fixtures, explaining why ? | |
| 20:15:52 | fried_bunny | It sure wouldn't hurt. | |
| 20:19:19 | imacdonn | right ... all tests are passing now ... I think I'll grab lunch, then work up some comments and a commit message | |
| 20:19:39 | fried_bunny | nice | |
| 20:20:00 | imacdonn | thanks very much for your help and patience! | |
| 20:22:23 | fried_bunny | imacdonn: Thank you very much for taking this on. | |
| 20:22:30 | imacdonn | np! | |
| 20:23:03 | fried_bunny | ooo, your first nova patch? | |
| 20:27:48 | imacdonn | probably, yeah | |
| 20:28:12 | imacdonn | have done a few cinder/os-brick ones | |
| 20:38:27 | openstackgerrit | Eric Berglund proposed openstack/nova master: PowerVM Driver: Snapshot https://review.openstack.org/543023 | |
| 21:02:14 | openstackgerrit | Eric Berglund proposed openstack/nova master: PowerVM Driver: DiskAdapter parent class https://review.openstack.org/549053 | |
| 21:03:19 | openstackgerrit | Eric Fried proposed openstack/nova master: Fix unit tests to work with new oslo.config https://review.openstack.org/558084 | |
| 21:03:38 | fried_bunny | dhellmann ^ | |
| 21:08:57 | openstackgerrit | Eric Berglund proposed openstack/nova master: PowerVM Driver: DiskAdapter parent class https://review.openstack.org/549053 | |
| 21:09:25 | openstackgerrit | Eric Berglund proposed openstack/nova master: PowerVM Driver: Localdisk https://review.openstack.org/549300 | |
| 21:20:22 | openstackgerrit | Eric Fried proposed openstack/nova master: Add unrequested resources to provider_summaries https://review.openstack.org/558045 | |
| 21:20:22 | openstackgerrit | Eric Fried proposed openstack/nova master: Test alloc_cands with indirectly sharing RPs https://review.openstack.org/519601 | |
| 21:20:23 | openstackgerrit | Eric Fried proposed openstack/nova master: Support relay RP for allocation candidates https://review.openstack.org/533437 | |
| 21:20:23 | openstackgerrit | Eric Fried proposed openstack/nova master: Return anchor providers in a_r and p_s https://review.openstack.org/558014 | |
| 21:28:04 | fried_bunny | imacdonn: Aha, I knew that sounded familiar: https://review.openstack.org/#/c/554759/1/nova/tests/unit/compute/test_compute_mgr.py@3708 | |
| 21:33:36 | openstackgerrit | Eric Fried proposed openstack/nova master: Remove deprecated [placement] opts https://review.openstack.org/557086 | |
| 21:42:06 | imacdonn | frickler: ahh, so I guess you decided to leave it alone that time ;) | |
| 21:42:21 | imacdonn | oops that was meant for fried_bunny ... grr, bad tab-completion | |