| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-03-31 | |||
| 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 | |
| 21:44:07 | imacdonn | zoiks ... nova could use some pep8 love ;) | |
| 21:59:34 | fried_bunny | imacdonn: Yeah, well, I was taking over someone else's patch, and kind of in a hurry, didn't want to get too involved. | |
| 21:59:38 | fried_bunny | What about pep8? | |
| 22:00:06 | imacdonn | I thought I'd have to pass pep8 with my change, but when I run it, I barfs on a ton of other stuff | |
| 22:00:38 | fried_bunny | How are you running it? | |
| 22:00:47 | imacdonn | just pep8 on the files I touched | |
| 22:00:55 | imacdonn | Trying to do it under tox instead now | |
| 22:02:36 | openstackgerrit | Eric Berglund proposed openstack/nova master: PowerVM Driver: Localdisk https://review.openstack.org/549300 | |
| 22:04:16 | fried_bunny | imacdonn: FYI, I use this: | |
| 22:04:16 | fried_bunny | efried@efried-ThinkPad-W520:~/Neo/nova$ type pep | |
| 22:04:16 | fried_bunny | pep is a function | |
| 22:04:16 | fried_bunny | pep () | |
| 22:04:16 | fried_bunny | { | |
| 22:04:17 | fried_bunny | ( . .tox/pep8/bin/activate || return; | |
| 22:04:17 | fried_bunny | num=${1:-1}; | |
| 22:04:18 | fried_bunny | echo "Running flake8 on files changed in the last $num commit(s)"; | |
| 22:04:18 | fried_bunny | git diff --name-only HEAD~$num | grep '\.py$' | xargs flake8 ) | |
| 22:04:19 | fried_bunny | } | |
| 22:04:46 | imacdonn | ah, neat .. I'll steal that - thanks ;) | |
| 22:06:51 | imacdonn | ./nova/tests/unit/compute/test_compute_mgr.py:21:1: H306 imports not in alphabetical order (keystoneauth1.exceptions, cinderclient.exceptions) | |
| 22:06:54 | imacdonn | wow .. pickyyyyy | |
| 22:18:11 | openstackgerrit | Eric Berglund proposed openstack/nova master: WIP: Resize https://review.openstack.org/553583 | |
| 22:34:01 | openstackgerrit | iain MacDonnell proposed openstack/nova master: Update check to ensure compute is using placement https://review.openstack.org/558089 | |