)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"c1034ab1b1cd1dd39dbc3ba0d899fe16121cf803","unresolved":false,"context_lines":[{"line_number":11,"context_line":"resulting domain."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"We add empty extra_specs to our fake flavor to avoid attempting to hit"},{"line_number":14,"context_line":"the DB."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"Related-bug: #1597669"},{"line_number":17,"context_line":"Change-Id: Ibe739b1811bdacad08a7fc499d26e4bed834eec9"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":15,"id":"3ac371cc_0363a1dd","line":14,"updated":"2016-08-15 15:19:48.000000000","message":"This is kind of a bit too much sausage-making for my taste. Would be perfectly fine as a code comment in the unit test.","commit_id":"7599e67406699d6886afb1ed15213d38aeadb3aa"}],"nova/tests/unit/virt/libvirt/test_driver.py":[{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"c1034ab1b1cd1dd39dbc3ba0d899fe16121cf803","unresolved":false,"context_lines":[{"line_number":16429,"context_line":"            \u0027ramdisk.rescue\u0027: \u0027raw\u0027,"},{"line_number":16430,"context_line":"            \u0027disk.rescue\u0027: \u0027default\u0027"},{"line_number":16431,"context_line":"        }"},{"line_number":16432,"context_line":"        # cache() should have been called on the 3 disk backends"},{"line_number":16433,"context_line":"        for name in (\u0027kernel\u0027, \u0027ramdisk\u0027, \u0027disk\u0027):"},{"line_number":16434,"context_line":"            name \u003d name + \u0027.rescue\u0027"},{"line_number":16435,"context_line":"            disk \u003d disks[name]"}],"source_content_type":"text/x-python","patch_set":15,"id":"3ac371cc_c36869bd","side":"PARENT","line":16432,"updated":"2016-08-15 15:19:48.000000000","message":"I don\u0027t see where you are testing any more that the disks involved were pulled from the image cache?","commit_id":"89a3ef60a340c0c9ef0d1aa59b9586af7c5a7e6a"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"4727a957ad86037ce4d0b051b567044ea1078b0e","unresolved":false,"context_lines":[{"line_number":16429,"context_line":"            \u0027ramdisk.rescue\u0027: \u0027raw\u0027,"},{"line_number":16430,"context_line":"            \u0027disk.rescue\u0027: \u0027default\u0027"},{"line_number":16431,"context_line":"        }"},{"line_number":16432,"context_line":"        # cache() should have been called on the 3 disk backends"},{"line_number":16433,"context_line":"        for name in (\u0027kernel\u0027, \u0027ramdisk\u0027, \u0027disk\u0027):"},{"line_number":16434,"context_line":"            name \u003d name + \u0027.rescue\u0027"},{"line_number":16435,"context_line":"            disk \u003d disks[name]"}],"source_content_type":"text/x-python","patch_set":15,"id":"3ac371cc_2108322d","side":"PARENT","line":16432,"in_reply_to":"3ac371cc_c36869bd","updated":"2016-08-16 09:02:31.000000000","message":"Have a look at line 16440. We assert that the set of created disks is what we expect.\n\nAlso, note that cache() is a really bad name as this method has (almost) nothing to do with the image cache. It\u0027s really a create method. The specific imagebackend decides how to create the disk, which may or may not involve the imagecache: Qcow2 and Rbd use it very differently for images, and Lvm sometimes doesn\u0027t use it at all. IOW the image cache is opaque to libvirt.driver.\n\nThis series eventually kills cache().","commit_id":"89a3ef60a340c0c9ef0d1aa59b9586af7c5a7e6a"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"c1034ab1b1cd1dd39dbc3ba0d899fe16121cf803","unresolved":false,"context_lines":[{"line_number":15135,"context_line":"                                rxtx_factor\u003d1.0,"},{"line_number":15136,"context_line":"                                flavorid\u003du\u00271\u0027,"},{"line_number":15137,"context_line":"                                vcpus\u003d1,"},{"line_number":15138,"context_line":"                                extra_specs\u003d{})"},{"line_number":15139,"context_line":"        flavor.update(params.pop(\u0027flavor\u0027, {}))"},{"line_number":15140,"context_line":""},{"line_number":15141,"context_line":"        inst \u003d {}"}],"source_content_type":"text/x-python","patch_set":15,"id":"3ac371cc_23797d94","line":15138,"updated":"2016-08-15 15:19:48.000000000","message":"Just do this:\n\n     extra_specs\u003d{},  # avoid hitting DB...\n )","commit_id":"7599e67406699d6886afb1ed15213d38aeadb3aa"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"4727a957ad86037ce4d0b051b567044ea1078b0e","unresolved":false,"context_lines":[{"line_number":15135,"context_line":"                                rxtx_factor\u003d1.0,"},{"line_number":15136,"context_line":"                                flavorid\u003du\u00271\u0027,"},{"line_number":15137,"context_line":"                                vcpus\u003d1,"},{"line_number":15138,"context_line":"                                extra_specs\u003d{})"},{"line_number":15139,"context_line":"        flavor.update(params.pop(\u0027flavor\u0027, {}))"},{"line_number":15140,"context_line":""},{"line_number":15141,"context_line":"        inst \u003d {}"}],"source_content_type":"text/x-python","patch_set":15,"id":"3ac371cc_474c5e42","line":15138,"in_reply_to":"3ac371cc_23797d94","updated":"2016-08-16 09:02:31.000000000","message":"Ok.","commit_id":"7599e67406699d6886afb1ed15213d38aeadb3aa"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"c1034ab1b1cd1dd39dbc3ba0d899fe16121cf803","unresolved":false,"context_lines":[{"line_number":16416,"context_line":""},{"line_number":16417,"context_line":"        with test.nested("},{"line_number":16418,"context_line":"            mock.patch.object(self.drvr, \u0027_create_domain\u0027,"},{"line_number":16419,"context_line":"                              side_effect\u003dorig_create_domain),"},{"line_number":16420,"context_line":"            self._get_guest_xml_mocks()"},{"line_number":16421,"context_line":"        ) as (mock_create_domain, mock_ignored):"},{"line_number":16422,"context_line":"            self.drvr.rescue(self.context, instance,"}],"source_content_type":"text/x-python","patch_set":15,"id":"3ac371cc_836e71c7","line":16419,"updated":"2016-08-15 15:19:48.000000000","message":"wait... what? why would you set the mock to return the original method?","commit_id":"7599e67406699d6886afb1ed15213d38aeadb3aa"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"4727a957ad86037ce4d0b051b567044ea1078b0e","unresolved":false,"context_lines":[{"line_number":16416,"context_line":""},{"line_number":16417,"context_line":"        with test.nested("},{"line_number":16418,"context_line":"            mock.patch.object(self.drvr, \u0027_create_domain\u0027,"},{"line_number":16419,"context_line":"                              side_effect\u003dorig_create_domain),"},{"line_number":16420,"context_line":"            self._get_guest_xml_mocks()"},{"line_number":16421,"context_line":"        ) as (mock_create_domain, mock_ignored):"},{"line_number":16422,"context_line":"            self.drvr.rescue(self.context, instance,"}],"source_content_type":"text/x-python","patch_set":15,"id":"3ac371cc_44261ccf","line":16419,"in_reply_to":"3ac371cc_836e71c7","updated":"2016-08-16 09:02:31.000000000","message":"I\u0027m doing this here to allow me to intercept the arguments to _create_domain without interfering with its execution. I want it to execute because one of its arguments is a callback function which I\u0027m testing here (it creates the config disk). On line 16430 I pull the XML argument out of call. So this is just setting up a proxy so I can allow it to run, and assert about how it ran later.\n\nHowever, elsewhere I have used a fake for this instead, where the fake simply executes the callback argument directly. That might also simplify pulling out the xml argument: the call_args_list stuff is a bit obtuse.\n\nI might change this.","commit_id":"7599e67406699d6886afb1ed15213d38aeadb3aa"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"c1034ab1b1cd1dd39dbc3ba0d899fe16121cf803","unresolved":false,"context_lines":[{"line_number":16424,"context_line":""},{"line_number":16425,"context_line":"            # Get the generated domain XML by extracting it from the call to"},{"line_number":16426,"context_line":"            # _create_domain()"},{"line_number":16427,"context_line":"            mock_create_domain.assert_called_once()"},{"line_number":16428,"context_line":"            # call_args_list contains a tuple, the first entry of which is"},{"line_number":16429,"context_line":"            # *args (positional arguments)"},{"line_number":16430,"context_line":"            create_domain_args \u003d mock_create_domain.call_args_list[0][0]"}],"source_content_type":"text/x-python","patch_set":15,"id":"3ac371cc_637375b2","line":16427,"updated":"2016-08-15 15:19:48.000000000","message":"assert_called_once() isn\u0027t a method on mock.Mock() therefore you aren\u0027t testing anything above, since assert_called_once() will return a mock.MagicMock(). assert_called_once_with() is the method you are looking for.","commit_id":"7599e67406699d6886afb1ed15213d38aeadb3aa"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"4727a957ad86037ce4d0b051b567044ea1078b0e","unresolved":false,"context_lines":[{"line_number":16424,"context_line":""},{"line_number":16425,"context_line":"            # Get the generated domain XML by extracting it from the call to"},{"line_number":16426,"context_line":"            # _create_domain()"},{"line_number":16427,"context_line":"            mock_create_domain.assert_called_once()"},{"line_number":16428,"context_line":"            # call_args_list contains a tuple, the first entry of which is"},{"line_number":16429,"context_line":"            # *args (positional arguments)"},{"line_number":16430,"context_line":"            create_domain_args \u003d mock_create_domain.call_args_list[0][0]"}],"source_content_type":"text/x-python","patch_set":15,"id":"3ac371cc_e4f628f8","line":16427,"in_reply_to":"3ac371cc_637375b2","updated":"2016-08-16 09:02:31.000000000","message":"There have been some relatively recent (months, not weeks) additions to Mock. I thought this one had been around for a while, but it\u0027s definitely there now.\n\nAlso, fun fact: latest mock, unless you explicitly disable it, won\u0027t provide mocks for non-existent methods which start with assert:\n\n\u003e\u003e\u003e mock.MagicMock().foo()\n\u003cMagicMock name\u003d\u0027mock.foo()\u0027 id\u003d\u0027140034083867984\u0027\u003e\n\n\u003e\u003e\u003e mock.MagicMock().assert_called_once()\nTraceback (most recent call last):\n  File \"\u003cstdin\u003e\", line 1, in \u003cmodule\u003e\n  File \"/usr/lib/python2.7/site-packages/mock/mock.py\", line 915, in assert_called_once\n    raise AssertionError(msg)\nAssertionError: Expected \u0027mock\u0027 to have been called once. Called 0 times.\n\n\u003e\u003e\u003e mock.MagicMock().assert_called_onc()\nTraceback (most recent call last):\n  File \"\u003cstdin\u003e\", line 1, in \u003cmodule\u003e\n  File \"/usr/lib/python2.7/site-packages/mock/mock.py\", line 703, in __getattr__\n    raise AttributeError(name)\nAttributeError: assert_called_onc","commit_id":"7599e67406699d6886afb1ed15213d38aeadb3aa"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"c1034ab1b1cd1dd39dbc3ba0d899fe16121cf803","unresolved":false,"context_lines":[{"line_number":16486,"context_line":"                          for name in (\u0027kernel\u0027, \u0027ramdisk\u0027)]"},{"line_number":16487,"context_line":""},{"line_number":16488,"context_line":"        # Assert that we imported the config disk"},{"line_number":16489,"context_line":"        config_disk.import_file.assert_called_once()"},{"line_number":16490,"context_line":""},{"line_number":16491,"context_line":"        # Assert that the config disk, kernel and ramdisk were created as raw"},{"line_number":16492,"context_line":"        for disk in [config_disk] + kernel_ramdisk:"}],"source_content_type":"text/x-python","patch_set":15,"id":"3ac371cc_a3656de2","line":16489,"updated":"2016-08-15 15:19:48.000000000","message":"See comment about about how assert_called_once() isn\u0027t an assertion method of mock.Mock().","commit_id":"7599e67406699d6886afb1ed15213d38aeadb3aa"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"4727a957ad86037ce4d0b051b567044ea1078b0e","unresolved":false,"context_lines":[{"line_number":16486,"context_line":"                          for name in (\u0027kernel\u0027, \u0027ramdisk\u0027)]"},{"line_number":16487,"context_line":""},{"line_number":16488,"context_line":"        # Assert that we imported the config disk"},{"line_number":16489,"context_line":"        config_disk.import_file.assert_called_once()"},{"line_number":16490,"context_line":""},{"line_number":16491,"context_line":"        # Assert that the config disk, kernel and ramdisk were created as raw"},{"line_number":16492,"context_line":"        for disk in [config_disk] + kernel_ramdisk:"}],"source_content_type":"text/x-python","patch_set":15,"id":"3ac371cc_47429e3f","line":16489,"in_reply_to":"3ac371cc_a3656de2","updated":"2016-08-16 09:02:31.000000000","message":"See reply.","commit_id":"7599e67406699d6886afb1ed15213d38aeadb3aa"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"e928bf4a5ea45800d99f8af3fba27a95640b4be6","unresolved":false,"context_lines":[{"line_number":15171,"context_line":"                                rxtx_factor\u003d1.0,"},{"line_number":15172,"context_line":"                                flavorid\u003du\u00271\u0027,"},{"line_number":15173,"context_line":"                                vcpus\u003d1,"},{"line_number":15174,"context_line":"                                # avoid hitting DB when accessing extra_specs"},{"line_number":15175,"context_line":"                                extra_specs\u003d{})"},{"line_number":15176,"context_line":"        flavor.update(params.pop(\u0027flavor\u0027, {}))"},{"line_number":15177,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"1ac06dbe_c414574e","line":15174,"updated":"2016-08-23 19:49:14.000000000","message":"I don\u0027t think this is really necessary to document and I hate comments in the middle of argument lists like this.","commit_id":"598f3c3230d21ad2b6dba30968b839735fbf6427"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"73819df97e2229a756253762fb7aab9d2e39c749","unresolved":false,"context_lines":[{"line_number":15171,"context_line":"                                rxtx_factor\u003d1.0,"},{"line_number":15172,"context_line":"                                flavorid\u003du\u00271\u0027,"},{"line_number":15173,"context_line":"                                vcpus\u003d1,"},{"line_number":15174,"context_line":"                                # avoid hitting DB when accessing extra_specs"},{"line_number":15175,"context_line":"                                extra_specs\u003d{})"},{"line_number":15176,"context_line":"        flavor.update(params.pop(\u0027flavor\u0027, {}))"},{"line_number":15177,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"9a89bdaa_bbb56cce","line":15174,"in_reply_to":"1ac06dbe_c414574e","updated":"2016-09-05 16:09:19.000000000","message":"+1","commit_id":"598f3c3230d21ad2b6dba30968b839735fbf6427"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"f89c080b9f5b341b03cfe1fd976e8aa7c41ba530","unresolved":false,"context_lines":[{"line_number":15171,"context_line":"                                rxtx_factor\u003d1.0,"},{"line_number":15172,"context_line":"                                flavorid\u003du\u00271\u0027,"},{"line_number":15173,"context_line":"                                vcpus\u003d1,"},{"line_number":15174,"context_line":"                                # avoid hitting DB when accessing extra_specs"},{"line_number":15175,"context_line":"                                extra_specs\u003d{})"},{"line_number":15176,"context_line":"        flavor.update(params.pop(\u0027flavor\u0027, {}))"},{"line_number":15177,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"7a8ec9b2_6a4cd970","line":15174,"in_reply_to":"9a89bdaa_92edd8f1","updated":"2016-09-14 20:55:45.000000000","message":"Matthew, I wrote that instead of putting the following comment in the commit message in PS15:\n\n\"We add empty extra_specs to our fake flavor to avoid attempting to hit\nthe DB.\"\n\nThat you could just put:\n\n extra_specs\u003d{},  # Avoid hitting DB\n\nin the test code.\n\nAnd not put so much sausage-making in the commit message. Note that I suggested putting the comment on the same line as the list item.","commit_id":"598f3c3230d21ad2b6dba30968b839735fbf6427"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"2c658369aa0e5362fe353e7e59913ce3abdaf7c6","unresolved":false,"context_lines":[{"line_number":15171,"context_line":"                                rxtx_factor\u003d1.0,"},{"line_number":15172,"context_line":"                                flavorid\u003du\u00271\u0027,"},{"line_number":15173,"context_line":"                                vcpus\u003d1,"},{"line_number":15174,"context_line":"                                # avoid hitting DB when accessing extra_specs"},{"line_number":15175,"context_line":"                                extra_specs\u003d{})"},{"line_number":15176,"context_line":"        flavor.update(params.pop(\u0027flavor\u0027, {}))"},{"line_number":15177,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"9a89bdaa_92edd8f1","line":15174,"in_reply_to":"9a89bdaa_bbb56cce","updated":"2016-09-06 09:26:59.000000000","message":"I put this here because Jay asked for it. I\u0027d take it out, but he might ask me to put it back.\n\nSeriously, though, I\u0027m completely ambivalent. I didn\u0027t put it there originally; I put it as a comment in the commit message because I felt that otherwise the change looked incongruous. I don\u0027t care, but as it\u0027s not terribly important I\u0027m just going to leave it as is until it\u0027s clear which side has the strongest objection.","commit_id":"598f3c3230d21ad2b6dba30968b839735fbf6427"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"a38efb6c45de5ea97306aaedcd6e041d38cfffea","unresolved":false,"context_lines":[{"line_number":15219,"context_line":"            mock.patch.object(libvirt_driver.LibvirtDriver,"},{"line_number":15220,"context_line":"                              \u0027_set_host_enabled\u0027),"},{"line_number":15221,"context_line":"            mock.patch.object(libvirt_driver.LibvirtDriver,"},{"line_number":15222,"context_line":"                              \u0027_build_device_metadata\u0027,"},{"line_number":15223,"context_line":"                              mock.Mock(return_value\u003dNone)),"},{"line_number":15224,"context_line":"            mock.patch.object(libvirt_driver.LibvirtDriver,"},{"line_number":15225,"context_line":"                              \u0027_supports_direct_io\u0027,"}],"source_content_type":"text/x-python","patch_set":16,"id":"9a89bdaa_dbc4f093","line":15222,"updated":"2016-09-05 15:59:02.000000000","message":"As i understand, both _build_device_metadata and _set_host_enabled are not called from _get_guest_xml. Moreover _supports_direct_io is called from about the 5th level of _get_guest_xml\u0027s call stack.\n\nAll of this does not bring clarity to the tests. Are use sure that we need here such deep integration tests? Instad of simple unit ones, where all called crucial methods are mocked and parameters of the calls are checked.","commit_id":"598f3c3230d21ad2b6dba30968b839735fbf6427"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"f7b55c3bef8b17c1293b8f2532a2cb152f658d17","unresolved":false,"context_lines":[{"line_number":15219,"context_line":"            mock.patch.object(libvirt_driver.LibvirtDriver,"},{"line_number":15220,"context_line":"                              \u0027_set_host_enabled\u0027),"},{"line_number":15221,"context_line":"            mock.patch.object(libvirt_driver.LibvirtDriver,"},{"line_number":15222,"context_line":"                              \u0027_build_device_metadata\u0027,"},{"line_number":15223,"context_line":"                              mock.Mock(return_value\u003dNone)),"},{"line_number":15224,"context_line":"            mock.patch.object(libvirt_driver.LibvirtDriver,"},{"line_number":15225,"context_line":"                              \u0027_supports_direct_io\u0027,"}],"source_content_type":"text/x-python","patch_set":16,"id":"9a89bdaa_87066acc","line":15222,"in_reply_to":"9a89bdaa_b279fcfe","updated":"2016-09-06 15:29:52.000000000","message":"Well... Honestly, looking at some next patches in the series, i\u0027ve understood your idea. A set of integration tests, which proves that all crucial things happen indeed and no more. Not considering expenses to support it, but perhaps assuming to simplify it later. It is a very healthy approach to such huge refactor task.\n\nSo, my concern is the function name. The function is intended to mock in custom cases, but its name (and the comment) does not explain this. If someone want to use it in a adifferent case, he may get side effects, spend time to fix them, get his tests over coded.\n\nIf you convince core reviewrs to leave this method, it will be perfect if you find a better name for it.","commit_id":"598f3c3230d21ad2b6dba30968b839735fbf6427"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"2c658369aa0e5362fe353e7e59913ce3abdaf7c6","unresolved":false,"context_lines":[{"line_number":15219,"context_line":"            mock.patch.object(libvirt_driver.LibvirtDriver,"},{"line_number":15220,"context_line":"                              \u0027_set_host_enabled\u0027),"},{"line_number":15221,"context_line":"            mock.patch.object(libvirt_driver.LibvirtDriver,"},{"line_number":15222,"context_line":"                              \u0027_build_device_metadata\u0027,"},{"line_number":15223,"context_line":"                              mock.Mock(return_value\u003dNone)),"},{"line_number":15224,"context_line":"            mock.patch.object(libvirt_driver.LibvirtDriver,"},{"line_number":15225,"context_line":"                              \u0027_supports_direct_io\u0027,"}],"source_content_type":"text/x-python","patch_set":16,"id":"9a89bdaa_b279fcfe","line":15222,"in_reply_to":"9a89bdaa_dbc4f093","updated":"2016-09-06 09:26:59.000000000","message":"So, if you go back to the commit message, you\u0027ll see that this rewrite was specifically prompted by a bug. Specifically, for a long time we had been checking that we were creating disks, but not that they ended up in the resulting domain. For better or for worse, the data structures describing an instance\u0027s storage are currently fiendishly complex. Consequently, to treat the contents of those data structures as unit test boundaries and actually test them thoroughly enough to find bugs such as this one would require an exponential explosion of unit test cases.\n\nAs it stands today, this is the test which needs to exist. We can have academic arguments about what\u0027s a unit or a functional test, but practically speaking we\u0027d just be discussing semantics of which directory to put this test in, because this test is the one that needs to exist if the goal is to provide assurance that the code is correct.\n\nLater in this refactor series I introduce the StoragePolicy class. Notice that StoragePolicy takes the data structure complexity upon itself and thoroughly tests it, then exposes a relatively simple interface. I hadn\u0027t originally intended it, but get_guest_xml could reasonably also use the StoragePolicy interface, which would get closer to the unit test boundary that you\u0027re looking for whilst also being thorough. But crucially it doesn\u0027t exist at this point in the series, so we can\u0027t use it.\n\nThe original purpose of this test, like the whole of this part of the refactor series, was to move the unit test boundary somewhere which wouldn\u0027t be affected by the refactor itself. That is, ensure that the existing tests continue to provide good coverage, and don\u0027t change during the refactor.\n\nIf I refactored the whole of the libvirt storage code and simultaneously rewrote all of its tests, I\u0027d have no assurance that I hadn\u0027t broken the previous code. This series changes a lot of code. However, the key point is that the unit tests run (almost) unchanged against:\n\n\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\n** BOTH THE OLD AND NEW VERSIONS OF THE CODE **\n\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\n\nConsequently, I can be pretty sure I didn\u0027t accidentally break anything important enough to test. You can\u0027t do that if your unit test boundary is the very thing you need to change.\n\nAs to whether specific things are called: I\u0027m almost 100% certain that they are, otherwise I wouldn\u0027t have put them here when I originally wrote this code. I don\u0027t recall the specifics now as it was some time ago, but I\u0027m unlikely to have put them here without a good reason. Incidentally, I completely agree that having to have a grab-bag of mocks like this is a mess, which is precisely why I put them in a library function with an obvious name. However, I don\u0027t see an alternative which is also a useful test.\n\nI\u0027d also like to come back to bug 1597669, which is an indictment of the way much of this code is unit tested: for a long time we have been diligently creating config disks for rescue instances, and then not using them, and nobody noticed. With this test, now we\u0027d notice.\n\nIf I can get far enough through this refactor series to introduce StoragePolicy, we can simplify this code a bit and also its tests, which in fact I\u0027ve already started doing. But later in the series, obviously, because we need to get there first.\n\nSorry if that comes over a bit ranty, btw. It is a rant if I\u0027m honest. Don\u0027t take it personally, though, you\u0027ve been a fantastic reviewer.","commit_id":"598f3c3230d21ad2b6dba30968b839735fbf6427"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"e928bf4a5ea45800d99f8af3fba27a95640b4be6","unresolved":false,"context_lines":[{"line_number":15225,"context_line":"                              \u0027_supports_direct_io\u0027,"},{"line_number":15226,"context_line":"                              mock.Mock(return_value\u003dTrue)),"},{"line_number":15227,"context_line":"            mock.patch(\u0027nova.api.metadata.base.InstanceMetadata\u0027)"},{"line_number":15228,"context_line":"        ):"},{"line_number":15229,"context_line":"            yield"},{"line_number":15230,"context_line":""},{"line_number":15231,"context_line":"    def test_migrate_disk_and_power_off_exception(self):"}],"source_content_type":"text/x-python","patch_set":16,"id":"1ac06dbe_c7b3ac96","line":15228,"updated":"2016-08-23 19:49:14.000000000","message":"I don\u0027t like this meta-mock approach here and I think these could be four decorators on top of the inner method _test_rescue(). It would be fewer lines and more clear (IMHO) about what is getting mocked in that method. You have a nested \"with with\" here. I know it works and maybe it\u0027s not a big deal, but .. ugh, I just don\u0027t like it. Interested to see what others think.","commit_id":"598f3c3230d21ad2b6dba30968b839735fbf6427"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"b0c0455b38d36463d69fdf558cfdb0cb9be69d4a","unresolved":false,"context_lines":[{"line_number":15225,"context_line":"                              \u0027_supports_direct_io\u0027,"},{"line_number":15226,"context_line":"                              mock.Mock(return_value\u003dTrue)),"},{"line_number":15227,"context_line":"            mock.patch(\u0027nova.api.metadata.base.InstanceMetadata\u0027)"},{"line_number":15228,"context_line":"        ):"},{"line_number":15229,"context_line":"            yield"},{"line_number":15230,"context_line":""},{"line_number":15231,"context_line":"    def test_migrate_disk_and_power_off_exception(self):"}],"source_content_type":"text/x-python","patch_set":16,"id":"fa7ab95a_499b1c18","line":15228,"in_reply_to":"1ac06dbe_c7b3ac96","updated":"2016-08-30 14:12:54.000000000","message":"It\u0027s used in a couple of different tests. Maybe not here, but later. I could turn it into a fixture?","commit_id":"598f3c3230d21ad2b6dba30968b839735fbf6427"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e56be267427b21a6411feea7c658ee3a4ba8f11a","unresolved":false,"context_lines":[{"line_number":15225,"context_line":"                              \u0027_supports_direct_io\u0027,"},{"line_number":15226,"context_line":"                              mock.Mock(return_value\u003dTrue)),"},{"line_number":15227,"context_line":"            mock.patch(\u0027nova.api.metadata.base.InstanceMetadata\u0027)"},{"line_number":15228,"context_line":"        ):"},{"line_number":15229,"context_line":"            yield"},{"line_number":15230,"context_line":""},{"line_number":15231,"context_line":"    def test_migrate_disk_and_power_off_exception(self):"}],"source_content_type":"text/x-python","patch_set":16,"id":"9a89bdaa_30f011c9","line":15228,"in_reply_to":"9a89bdaa_06851c58","updated":"2016-09-13 15:13:13.000000000","message":"I might update this, but I don\u0027t think I\u0027d replace a context manager with a decorator. They\u0027re essentially equivalent abstractions, but the context manager has tighter scope and is much easier to read.\n\nI could be persuaded to do something else entirely, but if we\u0027re going to call get_guest_xml in a unit test I think it\u0027s either this or flat mocks.","commit_id":"598f3c3230d21ad2b6dba30968b839735fbf6427"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"47a8dbb6f5bb23ec61d6d649413b3b289c38ce6f","unresolved":false,"context_lines":[{"line_number":15225,"context_line":"                              \u0027_supports_direct_io\u0027,"},{"line_number":15226,"context_line":"                              mock.Mock(return_value\u003dTrue)),"},{"line_number":15227,"context_line":"            mock.patch(\u0027nova.api.metadata.base.InstanceMetadata\u0027)"},{"line_number":15228,"context_line":"        ):"},{"line_number":15229,"context_line":"            yield"},{"line_number":15230,"context_line":""},{"line_number":15231,"context_line":"    def test_migrate_disk_and_power_off_exception(self):"}],"source_content_type":"text/x-python","patch_set":16,"id":"7a8ec9b2_2495f63f","line":15228,"in_reply_to":"9a89bdaa_30f011c9","updated":"2016-09-13 18:01:07.000000000","message":"Yeah, agreed they are equivalent abstractions, but I personally find the decorator much much easier to read/maintain than the context manager. Oh well. :)","commit_id":"598f3c3230d21ad2b6dba30968b839735fbf6427"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"de490c3d91405210cc62e54879de159b5443d28e","unresolved":false,"context_lines":[{"line_number":15225,"context_line":"                              \u0027_supports_direct_io\u0027,"},{"line_number":15226,"context_line":"                              mock.Mock(return_value\u003dTrue)),"},{"line_number":15227,"context_line":"            mock.patch(\u0027nova.api.metadata.base.InstanceMetadata\u0027)"},{"line_number":15228,"context_line":"        ):"},{"line_number":15229,"context_line":"            yield"},{"line_number":15230,"context_line":""},{"line_number":15231,"context_line":"    def test_migrate_disk_and_power_off_exception(self):"}],"source_content_type":"text/x-python","patch_set":16,"id":"fa7ab95a_e49ed561","line":15228,"in_reply_to":"fa7ab95a_499b1c18","updated":"2016-08-30 14:19:59.000000000","message":"I agree with Dan. This reminds me, on a smaller scale, of the massive stub/setup in the neutron API unit tests, which always makes debugging failures in those tests and what they are actually doing much harder since you have to look several hundred LOC up in the file to see what\u0027s being stubbed out and how it\u0027s all related. I\u0027d prefer to just see the mocks done in place where they are needed.","commit_id":"598f3c3230d21ad2b6dba30968b839735fbf6427"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"dda50214dd8b5fb88b65c2333e5e934e55584b19","unresolved":false,"context_lines":[{"line_number":15225,"context_line":"                              \u0027_supports_direct_io\u0027,"},{"line_number":15226,"context_line":"                              mock.Mock(return_value\u003dTrue)),"},{"line_number":15227,"context_line":"            mock.patch(\u0027nova.api.metadata.base.InstanceMetadata\u0027)"},{"line_number":15228,"context_line":"        ):"},{"line_number":15229,"context_line":"            yield"},{"line_number":15230,"context_line":""},{"line_number":15231,"context_line":"    def test_migrate_disk_and_power_off_exception(self):"}],"source_content_type":"text/x-python","patch_set":16,"id":"9a89bdaa_06851c58","line":15228,"in_reply_to":"fa7ab95a_69753d62","updated":"2016-09-10 00:49:13.000000000","message":"In general, I\u0027m not a fan of the more hidden mock approach because it\u0027s not easy to maintain when things change. And if there is too big a stack of mocks on something, then I\u0027d argue the function being tested is too large and needs to be broken into smaller units that are themselves unit tested. And then those boundaries are the only things mocked when testing \"the big function.\"\n\nThat said, I spent some time understanding the rationale here and it appears in the past a bug was hidden because the mocked \"dummy xml\" was faulty and covering up a problem.\n\nIf that\u0027s the case, I can see why you want a reusable way to say \"execute _get_guest_xml legally.\"\n\nI was thinking a more obvious/intuitive way to do this would be to make it a decorator instead like:\n\n    def _execute_get_guest_xml_without_db_access(f):\n        @functools.wraps(f)\n        def wrapped(*args, **kwargs):\n            with test.nested(\n                mock.patch(\u0027nova.virt.libvirt.LibvirtDriver._set_host_enabled\u0027),\n                mock.patch(\u0027nova.virt.libvirt.LibvirtDriver._build_device_metadata\u0027,\n                           return_value\u003dNone),\n                mock.patch(\u0027nova.virt.libvirt.LibvirtDriver._supports_direct_io\u0027,\n                           return_value\u003dTrue),\n                mock.patch(\u0027nova.api.metadata.base.InstanceMetadata\u0027)\n            ):\n                return f(*args, **kwargs)\n        return wrapped\n\nand then decorate _test_rescue with it like:\n\n @_execute_get_guest_xml_without_db_access\n @mock.patch(\u0027nova.virt.libvirt.utils.write_to_file\u0027)\n     def _test_rescue(...):\n\nJust MHO that this is something that\u0027s a lot less obscure and be more maintainable while doing the thing you want. I tried it locally and it worked for test_rescue but test_rescue_config_drive is failing for me locally, both with my experimental change and without it. So there might be some problem.","commit_id":"598f3c3230d21ad2b6dba30968b839735fbf6427"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"05a5e682f34139f507c7164b26018c57c359cb39","unresolved":false,"context_lines":[{"line_number":15225,"context_line":"                              \u0027_supports_direct_io\u0027,"},{"line_number":15226,"context_line":"                              mock.Mock(return_value\u003dTrue)),"},{"line_number":15227,"context_line":"            mock.patch(\u0027nova.api.metadata.base.InstanceMetadata\u0027)"},{"line_number":15228,"context_line":"        ):"},{"line_number":15229,"context_line":"            yield"},{"line_number":15230,"context_line":""},{"line_number":15231,"context_line":"    def test_migrate_disk_and_power_off_exception(self):"}],"source_content_type":"text/x-python","patch_set":16,"id":"fa7ab95a_69753d62","line":15228,"in_reply_to":"fa7ab95a_e49ed561","updated":"2016-08-30 14:41:39.000000000","message":"I can certainly do that without any trouble. However, I moved this into this mini library function because there are 4 of them, it\u0027s not entirely obvious what they are, and if you get them wrong they cause a warning, not an exception, so you probably won\u0027t notice. The warning comes with portents of non-deterministic future doom due to touching unmentionable parts of SQLA when DB access is disabled in the test.\n\nTBH, used sparingly I\u0027d like to see this used more. We over-mock a lot, and the issue with big lists of mocks is that you don\u0027t know why they were added and if they\u0027re still required. In this case, this is a minimal set of 4 mocks required if you want to execute _get_guest_xml without touching the DB.\n\nSo yeah, inlining these is no hassle, but my personal preference is still this. Let me know if I haven\u0027t persuaded you and I\u0027ll update.","commit_id":"598f3c3230d21ad2b6dba30968b839735fbf6427"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"73819df97e2229a756253762fb7aab9d2e39c749","unresolved":false,"context_lines":[{"line_number":16438,"context_line":"        self.assertIn(\u0027the device is no longer found on the guest\u0027,"},{"line_number":16439,"context_line":"                      six.text_type(mock_log.warning.call_args[0]))"},{"line_number":16440,"context_line":""},{"line_number":16441,"context_line":"    # Avoid writing to disk"},{"line_number":16442,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.utils.write_to_file\u0027)"},{"line_number":16443,"context_line":"    def _test_rescue(self, instance, mock_write_to_file, exists\u003dNone):"},{"line_number":16444,"context_line":"        backend \u003d self.useFixture("}],"source_content_type":"text/x-python","patch_set":16,"id":"9a89bdaa_fb8614b8","line":16441,"updated":"2016-09-05 16:09:19.000000000","message":"Such comments do not look very useful.","commit_id":"598f3c3230d21ad2b6dba30968b839735fbf6427"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"2c658369aa0e5362fe353e7e59913ce3abdaf7c6","unresolved":false,"context_lines":[{"line_number":16438,"context_line":"        self.assertIn(\u0027the device is no longer found on the guest\u0027,"},{"line_number":16439,"context_line":"                      six.text_type(mock_log.warning.call_args[0]))"},{"line_number":16440,"context_line":""},{"line_number":16441,"context_line":"    # Avoid writing to disk"},{"line_number":16442,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.utils.write_to_file\u0027)"},{"line_number":16443,"context_line":"    def _test_rescue(self, instance, mock_write_to_file, exists\u003dNone):"},{"line_number":16444,"context_line":"        backend \u003d self.useFixture("}],"source_content_type":"text/x-python","patch_set":16,"id":"9a89bdaa_12b468cf","line":16441,"in_reply_to":"9a89bdaa_fb8614b8","updated":"2016-09-06 09:26:59.000000000","message":"I wrote this comment about a bazillion revisions of this patch ago when there were several sets of mocks here with different purposes. I guess with only 1 left it\u0027s kinda obvious.","commit_id":"598f3c3230d21ad2b6dba30968b839735fbf6427"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"dda50214dd8b5fb88b65c2333e5e934e55584b19","unresolved":false,"context_lines":[{"line_number":16460,"context_line":"        with test.nested("},{"line_number":16461,"context_line":"            mock.patch.object(self.drvr, \u0027_create_domain\u0027,"},{"line_number":16462,"context_line":"                              side_effect\u003dfake_create_domain),"},{"line_number":16463,"context_line":"            self._get_guest_xml_mocks()"},{"line_number":16464,"context_line":"        ) as (mock_create_domain, mock_ignored):"},{"line_number":16465,"context_line":"            self.drvr.rescue(self.context, instance,"},{"line_number":16466,"context_line":"                             network_info, image_meta, rescue_password)"}],"source_content_type":"text/x-python","patch_set":16,"id":"9a89bdaa_664b482c","line":16463,"range":{"start_line":16463,"start_character":12,"end_line":16463,"end_character":39},"updated":"2016-09-10 00:49:13.000000000","message":"Isn\u0027t this mocking more than was originally there? That is, instead of mocking _get_guest_xml as a unit, you\u0027re now mocking every little piece inside of _get_guest_xml and letting it call down instead. Is that what you intended?\n\nEdit: I\u0027m clearly late to this party, so feel free to ignore me. I would have thought _get_guest_xml would be tested by itself as a unit and then mocked everywhere else. I\u0027m guessing the old testing was using a faulty dummy xml which was hiding the bug.","commit_id":"598f3c3230d21ad2b6dba30968b839735fbf6427"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"5d297d9f4b06c6c7462062b99bc092957196e8f0","unresolved":false,"context_lines":[{"line_number":16460,"context_line":"        with test.nested("},{"line_number":16461,"context_line":"            mock.patch.object(self.drvr, \u0027_create_domain\u0027,"},{"line_number":16462,"context_line":"                              side_effect\u003dfake_create_domain),"},{"line_number":16463,"context_line":"            self._get_guest_xml_mocks()"},{"line_number":16464,"context_line":"        ) as (mock_create_domain, mock_ignored):"},{"line_number":16465,"context_line":"            self.drvr.rescue(self.context, instance,"},{"line_number":16466,"context_line":"                             network_info, image_meta, rescue_password)"}],"source_content_type":"text/x-python","patch_set":16,"id":"9a89bdaa_20d68327","line":16463,"range":{"start_line":16463,"start_character":12,"end_line":16463,"end_character":39},"in_reply_to":"9a89bdaa_664b482c","updated":"2016-09-10 00:50:44.000000000","message":"Gah, sorry I meant to discard this comment after rewriting the earlier one. Please ignore.","commit_id":"598f3c3230d21ad2b6dba30968b839735fbf6427"}]}