| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-03-31 | |||
| 18:50:09 | fried_bunny | self.useFixture(nova_fixtures.PlacementFixture()) | |
| 18:50:45 | imacdonn | I thought about something like that ... but I also don't know where the fixture gets its config from | |
| 18:50:58 | fried_bunny | It uses the conf patcher :) | |
| 18:51:12 | fried_bunny | ...which is instantiated further down the line. | |
| 18:51:25 | imacdonn | ok, but where does that get its config from :) | |
| 18:52:13 | fried_bunny | It's *possible* it gets layered. | |
| 18:52:19 | fried_bunny | Which is kinda cool. | |
| 18:52:39 | fried_bunny | I think you make conf changes in tests via self.flags | |
| 18:54:53 | imacdonn | but that wouldn't be the config for the fixture (?) | |
| 18:56:01 | imacdonn | seems like it'd have to be passed through start_service() somehow | |
| 18:56:01 | fried_bunny | It is, because the fixture is basically mocking conf entirely. I think. | |
| 18:57:08 | fried_bunny | If you look at nova.tests.unit.conf_fixture.ConfFixture, it's setting a veritable plethora of things. | |
| 18:58:01 | imacdonn | yes, but in that case, the fixture itself is the code being tested | |
| 18:58:23 | fried_bunny | eh? | |
| 18:59:15 | fried_bunny | You mean in the case you're trying to write? | |
| 18:59:35 | imacdonn | in that case "self" is a fixture ... in the other cases, they create fixtures, but "self" is not the fixture. so "self.conf" wouldn't help .. I think | |
| 18:59:36 | fried_bunny | (no, cause you took out the conf checking) | |
| 19:00:05 | fried_bunny | Okay, let's back up. Why do you think you care about conf for this test? | |
| 19:00:54 | imacdonn | because the compute service fixture needs to have the auth_method (etc.) set, otherwise it'll throw that MissingPlugin | |
| 19:01:19 | imacdonn | MissingAuthPlugin* | |
| 19:01:42 | fried_bunny | Will it? Even with the PlacementFixture in place? | |
| 19:02:12 | imacdonn | I think so .. it's not failing because the service is not there .. it's failing because of that MissingAuthPlugin | |
| 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. | |