)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"9938f0bc1a66054f7cb317a4bca165b9e2e30fb3","unresolved":false,"context_lines":[{"line_number":7,"context_line":"libvirt: Rewrite _test_finish_migration"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Rewrite the finish_migration tests in anticipation of a major update."},{"line_number":10,"context_line":"Apart from switching from mox to mock, the principal change is that"},{"line_number":11,"context_line":"we\u0027re now mocking at the backend boundary rather than the"},{"line_number":12,"context_line":"_create_image boundary, which means we can test what we do to"},{"line_number":13,"context_line":"individual disks. In the updated finish_migration we no longer use"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"7a77a97e_be1561e6","line":10,"range":{"start_line":10,"start_character":0,"end_line":10,"end_character":37},"updated":"2016-11-22 11:21:10.000000000","message":"I\u0027m certainly too late to the game to ask for this, but I can only guess this would be easier to review had the Mox-mock conversion occurred separately","commit_id":"0a02b9bcb27a1c810696b44d0829710c86d9f5e8"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"87b22df57ac9d1abb4d599d814b7149591bf3d2b","unresolved":false,"context_lines":[{"line_number":7,"context_line":"libvirt: Rewrite _test_finish_migration"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Rewrite the finish_migration tests in anticipation of a major update."},{"line_number":10,"context_line":"Apart from switching from mox to mock, the principal change is that"},{"line_number":11,"context_line":"we\u0027re now mocking at the backend boundary rather than the"},{"line_number":12,"context_line":"_create_image boundary, which means we can test what we do to"},{"line_number":13,"context_line":"individual disks. In the updated finish_migration we no longer use"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"7a77a97e_92ab341e","line":10,"range":{"start_line":10,"start_character":0,"end_line":10,"end_character":37},"in_reply_to":"7a77a97e_be1561e6","updated":"2016-11-22 12:11:56.000000000","message":"It\u0027s a complete rewrite. Mox to mock is incidental.","commit_id":"0a02b9bcb27a1c810696b44d0829710c86d9f5e8"}],"nova/tests/unit/virt/libvirt/test_driver.py":[{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"ea4cd72b5b8a4a892ef88675eadf02c09cef7d14","unresolved":false,"context_lines":[{"line_number":15771,"context_line":""},{"line_number":15772,"context_line":"        # Assert that we created a snapshot for the root disk"},{"line_number":15773,"context_line":"        root_disk \u003d disks[\u0027disk\u0027]"},{"line_number":15774,"context_line":"        root_disk.create_snap.assert_called_once()"},{"line_number":15775,"context_line":""},{"line_number":15776,"context_line":"        # Assert that we didn\u0027t import a config disk"},{"line_number":15777,"context_line":"        # Note that some path currently creates a config disk object,"}],"source_content_type":"text/x-python","patch_set":14,"id":"3ac371cc_e34645be","line":15774,"updated":"2016-08-15 15:26:26.000000000","message":"assert_called_once() not a method...","commit_id":"2666484c3b9aec76d4f6ea8f30d76b488bec4770"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"ea4cd72b5b8a4a892ef88675eadf02c09cef7d14","unresolved":false,"context_lines":[{"line_number":15778,"context_line":"        # but only uses it for an exists() check. Therefore the object may"},{"line_number":15779,"context_line":"        # exist, but shouldn\u0027t have been imported."},{"line_number":15780,"context_line":"        if \u0027disk.config\u0027 in disks:"},{"line_number":15781,"context_line":"            disks[\u0027disk.config\u0027].import_file.assert_not_called()"},{"line_number":15782,"context_line":""},{"line_number":15783,"context_line":"        # We shouldn\u0027t be injecting data during migration"},{"line_number":15784,"context_line":"        mock_inject_data.assert_not_called()"}],"source_content_type":"text/x-python","patch_set":14,"id":"3ac371cc_4312b9b5","line":15781,"updated":"2016-08-15 15:26:26.000000000","message":"assert_not_called() is only a method of py3k mock library. It does not exist in py27 which means the above may falsely return None (i.e. not raise an exception) on py27. Please replace all use of assert_not_called() with:\n\n self.assertFalse(mock_obj.called)","commit_id":"2666484c3b9aec76d4f6ea8f30d76b488bec4770"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"6bf710456889b40f2dd7cc6f2a1b793d51561871","unresolved":false,"context_lines":[{"line_number":15778,"context_line":"        # but only uses it for an exists() check. Therefore the object may"},{"line_number":15779,"context_line":"        # exist, but shouldn\u0027t have been imported."},{"line_number":15780,"context_line":"        if \u0027disk.config\u0027 in disks:"},{"line_number":15781,"context_line":"            disks[\u0027disk.config\u0027].import_file.assert_not_called()"},{"line_number":15782,"context_line":""},{"line_number":15783,"context_line":"        # We shouldn\u0027t be injecting data during migration"},{"line_number":15784,"context_line":"        mock_inject_data.assert_not_called()"}],"source_content_type":"text/x-python","patch_set":14,"id":"3ac371cc_67a99a7b","line":15781,"in_reply_to":"3ac371cc_4312b9b5","updated":"2016-08-16 09:06:03.000000000","message":"See comments on previous about latest py27 mock:\n\n\u003e\u003e\u003e foo \u003d mock.MagicMock()\n\n\u003e\u003e\u003e foo()\n\u003cMagicMock name\u003d\u0027mock()\u0027 id\u003d\u0027140034084026896\u0027\u003e\n\n\u003e\u003e\u003e foo.assert_not_called()\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 897, in assert_not_called\n    raise AssertionError(msg)\nAssertionError: Expected \u0027mock\u0027 to not have been called. Called 1 times.","commit_id":"2666484c3b9aec76d4f6ea8f30d76b488bec4770"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"2954982e4e39707b1834b1a1eb71f37f6640472c","unresolved":false,"context_lines":[{"line_number":15745,"context_line":"    @mock.patch.object(libvirt_driver.LibvirtDriver,"},{"line_number":15746,"context_line":"                       \u0027_create_domain_and_network\u0027)"},{"line_number":15747,"context_line":"    @mock.patch.object(libvirt_driver.LibvirtDriver, \u0027_disk_raw_to_qcow2\u0027)"},{"line_number":15748,"context_line":"    # Don\u0027t write libvirt xml to disk"},{"line_number":15749,"context_line":"    @mock.patch.object(libvirt_utils, \u0027write_to_file\u0027, mock.Mock())"},{"line_number":15750,"context_line":"    def _test_finish_migration(self, mock_raw_to_qcow2,"},{"line_number":15751,"context_line":"                               mock_create_domain_and_network,"}],"source_content_type":"text/x-python","patch_set":16,"id":"9a89bdaa_db7790d2","line":15748,"updated":"2016-09-05 16:33:25.000000000","message":"This comment does not look useful.","commit_id":"a2ce4510e77b597487f75519c339d79697deb793"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"f00d9b7d4747942f031dd02ee618e7a405d65890","unresolved":false,"context_lines":[{"line_number":15746,"context_line":"                       \u0027_create_domain_and_network\u0027)"},{"line_number":15747,"context_line":"    @mock.patch.object(libvirt_driver.LibvirtDriver, \u0027_disk_raw_to_qcow2\u0027)"},{"line_number":15748,"context_line":"    # Don\u0027t write libvirt xml to disk"},{"line_number":15749,"context_line":"    @mock.patch.object(libvirt_utils, \u0027write_to_file\u0027, mock.Mock())"},{"line_number":15750,"context_line":"    def _test_finish_migration(self, mock_raw_to_qcow2,"},{"line_number":15751,"context_line":"                               mock_create_domain_and_network,"},{"line_number":15752,"context_line":"                               mock_get_info, mock_inject_data,"}],"source_content_type":"text/x-python","patch_set":16,"id":"9a89bdaa_5acb7953","line":15749,"range":{"start_line":15749,"start_character":55,"end_line":15749,"end_character":66},"updated":"2016-09-05 16:48:23.000000000","message":"What was a reason for this? I thought it is no matter how to stub such things.","commit_id":"a2ce4510e77b597487f75519c339d79697deb793"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"71c2acaf15881d82a8a14f3888d4d31ba392d125","unresolved":false,"context_lines":[{"line_number":15746,"context_line":"                       \u0027_create_domain_and_network\u0027)"},{"line_number":15747,"context_line":"    @mock.patch.object(libvirt_driver.LibvirtDriver, \u0027_disk_raw_to_qcow2\u0027)"},{"line_number":15748,"context_line":"    # Don\u0027t write libvirt xml to disk"},{"line_number":15749,"context_line":"    @mock.patch.object(libvirt_utils, \u0027write_to_file\u0027, mock.Mock())"},{"line_number":15750,"context_line":"    def _test_finish_migration(self, mock_raw_to_qcow2,"},{"line_number":15751,"context_line":"                               mock_create_domain_and_network,"},{"line_number":15752,"context_line":"                               mock_get_info, mock_inject_data,"}],"source_content_type":"text/x-python","patch_set":16,"id":"9a89bdaa_fe86ebd9","line":15749,"range":{"start_line":15749,"start_character":55,"end_line":15749,"end_character":66},"in_reply_to":"9a89bdaa_5acb7953","updated":"2016-09-06 09:36:11.000000000","message":"Jay had the same objection to an earlier patch, and I ended up taking it out. The purpose is that if you pass a new argument to mock.patch() when used as a decorator, it doesn\u0027t add an argument to the function. As this argument isn\u0027t required in this case, it just avoids an unused argument.\n\nDespite Jay\u0027s objection I continue to think it\u0027s tidier. However, it\u0027s hardly important enough to wrangle over, so I\u0027ll just take it out.","commit_id":"a2ce4510e77b597487f75519c339d79697deb793"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"fe30fe93682f822fbced8b0b2b63151e59df5798","unresolved":false,"context_lines":[{"line_number":15746,"context_line":"                       \u0027_create_domain_and_network\u0027)"},{"line_number":15747,"context_line":"    @mock.patch.object(libvirt_driver.LibvirtDriver, \u0027_disk_raw_to_qcow2\u0027)"},{"line_number":15748,"context_line":"    # Don\u0027t write libvirt xml to disk"},{"line_number":15749,"context_line":"    @mock.patch.object(libvirt_utils, \u0027write_to_file\u0027, mock.Mock())"},{"line_number":15750,"context_line":"    def _test_finish_migration(self, mock_raw_to_qcow2,"},{"line_number":15751,"context_line":"                               mock_create_domain_and_network,"},{"line_number":15752,"context_line":"                               mock_get_info, mock_inject_data,"}],"source_content_type":"text/x-python","patch_set":16,"id":"9a89bdaa_7c30149f","line":15749,"range":{"start_line":15749,"start_character":55,"end_line":15749,"end_character":66},"in_reply_to":"9a89bdaa_fe86ebd9","updated":"2016-09-06 15:29:28.000000000","message":"nice trick!","commit_id":"a2ce4510e77b597487f75519c339d79697deb793"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"2954982e4e39707b1834b1a1eb71f37f6640472c","unresolved":false,"context_lines":[{"line_number":15780,"context_line":""},{"line_number":15781,"context_line":"        with test.nested("},{"line_number":15782,"context_line":"            self._get_guest_xml_mocks(),"},{"line_number":15783,"context_line":"            mock.patch.object(self.drvr, \u0027_disk_resize\u0027)"},{"line_number":15784,"context_line":"        ):"},{"line_number":15785,"context_line":"            self.drvr.finish_migration("},{"line_number":15786,"context_line":"                          context.get_admin_context(), migration, instance,"}],"source_content_type":"text/x-python","patch_set":16,"id":"9a89bdaa_ba19e57b","line":15783,"updated":"2016-09-05 16:33:25.000000000","message":"why not to move this to a decorator as well?","commit_id":"a2ce4510e77b597487f75519c339d79697deb793"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"69440266c4cd42a7277ff88a961c863c0eb2c275","unresolved":false,"context_lines":[{"line_number":15669,"context_line":"                       \u0027_create_domain_and_network\u0027)"},{"line_number":15670,"context_line":"    @mock.patch.object(libvirt_driver.LibvirtDriver, \u0027_disk_raw_to_qcow2\u0027)"},{"line_number":15671,"context_line":"    # Don\u0027t write libvirt xml to disk"},{"line_number":15672,"context_line":"    @mock.patch.object(libvirt_utils, \u0027write_to_file\u0027, mock.Mock())"},{"line_number":15673,"context_line":"    # NOTE(mdbooth): The following 4 mocks are required to execute"},{"line_number":15674,"context_line":"    #                get_guest_xml()."},{"line_number":15675,"context_line":"    @mock.patch.object(libvirt_driver.LibvirtDriver, \u0027_set_host_enabled\u0027)"}],"source_content_type":"text/x-python","patch_set":17,"id":"9a629dbe_3ed364d6","line":15672,"updated":"2016-11-11 20:20:47.000000000","message":"As mentioned on previous reviews, doing this to remove the mock from the list of args to the test method is not a practice we use in Nova. I personally like the explicit inclusion of mock arguments. I think it adds to the readability of the method.","commit_id":"2a6849c3296a8a852c7a7f927d26f2513da0f057"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"7d95c8a93455564c757bf0b5a5c1913894f3f18c","unresolved":false,"context_lines":[{"line_number":15669,"context_line":"                       \u0027_create_domain_and_network\u0027)"},{"line_number":15670,"context_line":"    @mock.patch.object(libvirt_driver.LibvirtDriver, \u0027_disk_raw_to_qcow2\u0027)"},{"line_number":15671,"context_line":"    # Don\u0027t write libvirt xml to disk"},{"line_number":15672,"context_line":"    @mock.patch.object(libvirt_utils, \u0027write_to_file\u0027, mock.Mock())"},{"line_number":15673,"context_line":"    # NOTE(mdbooth): The following 4 mocks are required to execute"},{"line_number":15674,"context_line":"    #                get_guest_xml()."},{"line_number":15675,"context_line":"    @mock.patch.object(libvirt_driver.LibvirtDriver, \u0027_set_host_enabled\u0027)"}],"source_content_type":"text/x-python","patch_set":17,"id":"7a77a97e_44866c14","line":15672,"in_reply_to":"9a629dbe_3ed364d6","updated":"2016-11-21 13:22:39.000000000","message":"I\u0027d missed this when updating this patch most recently. As I\u0027ve had to touch it again to resolve a merge conflict I\u0027ve updated this.","commit_id":"2a6849c3296a8a852c7a7f927d26f2513da0f057"},{"author":{"_account_id":14819,"name":"Timofey Durakov","email":"timofei.nd@gmail.com","username":"tdurakov"},"change_message_id":"2c76c82332a1460fc485b67136eab4d00d780c2e","unresolved":false,"context_lines":[{"line_number":15694,"context_line":"        mock_get_info.return_value \u003d hardware.InstanceInfo(state\u003dstate)"},{"line_number":15695,"context_line":""},{"line_number":15696,"context_line":"        instance \u003d self._create_instance("},{"line_number":15697,"context_line":"            {\u0027config_drive\u0027: str(True),"},{"line_number":15698,"context_line":"             \u0027task_state\u0027: task_states.RESIZE_FINISH,"},{"line_number":15699,"context_line":"             \u0027flavor\u0027: {\u0027swap\u0027: 500}})"},{"line_number":15700,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"9a629dbe_c1809f8e","line":15697,"range":{"start_line":15697,"start_character":29,"end_line":15697,"end_character":38},"updated":"2016-11-14 13:41:46.000000000","message":"not sure we actually need this, as it\u0027s not a variable that is reused somewhere else, I\u0027ll be ok to have hard coded \u0027True\u0027 here.","commit_id":"2a6849c3296a8a852c7a7f927d26f2513da0f057"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"52f4d5cc85b4bc0e73fcc3c08502a777bd14728b","unresolved":false,"context_lines":[{"line_number":15694,"context_line":"        mock_get_info.return_value \u003d hardware.InstanceInfo(state\u003dstate)"},{"line_number":15695,"context_line":""},{"line_number":15696,"context_line":"        instance \u003d self._create_instance("},{"line_number":15697,"context_line":"            {\u0027config_drive\u0027: str(True),"},{"line_number":15698,"context_line":"             \u0027task_state\u0027: task_states.RESIZE_FINISH,"},{"line_number":15699,"context_line":"             \u0027flavor\u0027: {\u0027swap\u0027: 500}})"},{"line_number":15700,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"7a77a97e_52eb5c78","line":15697,"range":{"start_line":15697,"start_character":29,"end_line":15697,"end_character":38},"in_reply_to":"7a77a97e_e47826fb","updated":"2016-11-22 12:12:26.000000000","message":"Ah, that\u0027s because of a bug in the Instance object. It\u0027s a string, not a boolean.","commit_id":"2a6849c3296a8a852c7a7f927d26f2513da0f057"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"39e75712ebd22ba4204d967240f6443b6026e9f1","unresolved":false,"context_lines":[{"line_number":15694,"context_line":"        mock_get_info.return_value \u003d hardware.InstanceInfo(state\u003dstate)"},{"line_number":15695,"context_line":""},{"line_number":15696,"context_line":"        instance \u003d self._create_instance("},{"line_number":15697,"context_line":"            {\u0027config_drive\u0027: str(True),"},{"line_number":15698,"context_line":"             \u0027task_state\u0027: task_states.RESIZE_FINISH,"},{"line_number":15699,"context_line":"             \u0027flavor\u0027: {\u0027swap\u0027: 500}})"},{"line_number":15700,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"7a77a97e_e47826fb","line":15697,"range":{"start_line":15697,"start_character":29,"end_line":15697,"end_character":38},"in_reply_to":"7a77a97e_e49f7863","updated":"2016-11-22 11:21:22.000000000","message":"I think he\u0027s asking why you don\u0027t write\n\n    \u0027True\u0027\n\ninstead of\n\n    str(True)","commit_id":"2a6849c3296a8a852c7a7f927d26f2513da0f057"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"7d95c8a93455564c757bf0b5a5c1913894f3f18c","unresolved":false,"context_lines":[{"line_number":15694,"context_line":"        mock_get_info.return_value \u003d hardware.InstanceInfo(state\u003dstate)"},{"line_number":15695,"context_line":""},{"line_number":15696,"context_line":"        instance \u003d self._create_instance("},{"line_number":15697,"context_line":"            {\u0027config_drive\u0027: str(True),"},{"line_number":15698,"context_line":"             \u0027task_state\u0027: task_states.RESIZE_FINISH,"},{"line_number":15699,"context_line":"             \u0027flavor\u0027: {\u0027swap\u0027: 500}})"},{"line_number":15700,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"7a77a97e_e49f7863","line":15697,"range":{"start_line":15697,"start_character":29,"end_line":15697,"end_character":38},"in_reply_to":"9a629dbe_c1809f8e","updated":"2016-11-21 13:22:39.000000000","message":"Not sure I follow. Where are you suggesting config_drive could be hard-coded? In _create_instance?","commit_id":"2a6849c3296a8a852c7a7f927d26f2513da0f057"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"69440266c4cd42a7277ff88a961c863c0eb2c275","unresolved":false,"context_lines":[{"line_number":15756,"context_line":"        mock_create_domain_and_network.assert_called_once_with("},{"line_number":15757,"context_line":"            mock.ANY, mock.ANY, instance, [], mock.ANY,"},{"line_number":15758,"context_line":"            block_device_info\u003dNone, power_on\u003dpower_on,"},{"line_number":15759,"context_line":"            vifs_already_plugged\u003dTrue, post_xml_callback\u003dmock.ANY)"},{"line_number":15760,"context_line":""},{"line_number":15761,"context_line":"    def test_finish_migration_resize(self):"},{"line_number":15762,"context_line":"        with mock.patch(\u0027nova.virt.libvirt.guest.Guest.sync_guest_time\u0027"}],"source_content_type":"text/x-python","patch_set":17,"id":"9a629dbe_1e43006d","line":15759,"updated":"2016-11-11 20:20:47.000000000","message":"++ to all of the above well-documented assertions. Nice work refactoring this to a more readable state.","commit_id":"2a6849c3296a8a852c7a7f927d26f2513da0f057"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"9938f0bc1a66054f7cb317a4bca165b9e2e30fb3","unresolved":false,"context_lines":[{"line_number":15748,"context_line":"    @mock.patch.object(libvirt_driver.LibvirtDriver, \u0027_disk_raw_to_qcow2\u0027)"},{"line_number":15749,"context_line":"    # Don\u0027t write libvirt xml to disk"},{"line_number":15750,"context_line":"    @mock.patch.object(libvirt_utils, \u0027write_to_file\u0027)"},{"line_number":15751,"context_line":"    # NOTE(mdbooth): The following 4 mocks are required to execute"},{"line_number":15752,"context_line":"    #                get_guest_xml()."},{"line_number":15753,"context_line":"    @mock.patch.object(libvirt_driver.LibvirtDriver, \u0027_set_host_enabled\u0027)"},{"line_number":15754,"context_line":"    @mock.patch.object(libvirt_driver.LibvirtDriver, \u0027_build_device_metadata\u0027)"},{"line_number":15755,"context_line":"    @mock.patch.object(libvirt_driver.LibvirtDriver, \u0027_supports_direct_io\u0027)"}],"source_content_type":"text/x-python","patch_set":18,"id":"7a77a97e_be5001d6","line":15752,"range":{"start_line":15751,"start_character":0,"end_line":15752,"end_character":37},"updated":"2016-11-22 11:21:10.000000000","message":"These are helpful - cheers. However is this something that would make sense as a fixture? I have no idea, hence why I\u0027m asking :)","commit_id":"0a02b9bcb27a1c810696b44d0829710c86d9f5e8"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"5221b68afb5fe90ad1bfb727c5002920f93d9dd8","unresolved":false,"context_lines":[{"line_number":15748,"context_line":"    @mock.patch.object(libvirt_driver.LibvirtDriver, \u0027_disk_raw_to_qcow2\u0027)"},{"line_number":15749,"context_line":"    # Don\u0027t write libvirt xml to disk"},{"line_number":15750,"context_line":"    @mock.patch.object(libvirt_utils, \u0027write_to_file\u0027)"},{"line_number":15751,"context_line":"    # NOTE(mdbooth): The following 4 mocks are required to execute"},{"line_number":15752,"context_line":"    #                get_guest_xml()."},{"line_number":15753,"context_line":"    @mock.patch.object(libvirt_driver.LibvirtDriver, \u0027_set_host_enabled\u0027)"},{"line_number":15754,"context_line":"    @mock.patch.object(libvirt_driver.LibvirtDriver, \u0027_build_device_metadata\u0027)"},{"line_number":15755,"context_line":"    @mock.patch.object(libvirt_driver.LibvirtDriver, \u0027_supports_direct_io\u0027)"}],"source_content_type":"text/x-python","patch_set":18,"id":"5a74a57a_a99ac5c3","line":15752,"range":{"start_line":15751,"start_character":0,"end_line":15752,"end_character":37},"in_reply_to":"7a77a97e_9259745e","updated":"2016-11-23 00:14:11.000000000","message":"Yes, originally it was a nested context manager, I believe, which was more confusing than it was worth, IMHO.\n\nA fixture would be a good way to refactor this, now that Stephen mentions it. But, that would be a good standalone patch, not something to do here.","commit_id":"0a02b9bcb27a1c810696b44d0829710c86d9f5e8"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"87b22df57ac9d1abb4d599d814b7149591bf3d2b","unresolved":false,"context_lines":[{"line_number":15748,"context_line":"    @mock.patch.object(libvirt_driver.LibvirtDriver, \u0027_disk_raw_to_qcow2\u0027)"},{"line_number":15749,"context_line":"    # Don\u0027t write libvirt xml to disk"},{"line_number":15750,"context_line":"    @mock.patch.object(libvirt_utils, \u0027write_to_file\u0027)"},{"line_number":15751,"context_line":"    # NOTE(mdbooth): The following 4 mocks are required to execute"},{"line_number":15752,"context_line":"    #                get_guest_xml()."},{"line_number":15753,"context_line":"    @mock.patch.object(libvirt_driver.LibvirtDriver, \u0027_set_host_enabled\u0027)"},{"line_number":15754,"context_line":"    @mock.patch.object(libvirt_driver.LibvirtDriver, \u0027_build_device_metadata\u0027)"},{"line_number":15755,"context_line":"    @mock.patch.object(libvirt_driver.LibvirtDriver, \u0027_supports_direct_io\u0027)"}],"source_content_type":"text/x-python","patch_set":18,"id":"7a77a97e_9259745e","line":15752,"range":{"start_line":15751,"start_character":0,"end_line":15752,"end_character":37},"in_reply_to":"7a77a97e_be5001d6","updated":"2016-11-22 12:11:56.000000000","message":"I originally implemented it as a wrapper, and replaced it with this following review. I guess it could also be a fixture, but I don\u0027t want to repaint this bikeshed again.","commit_id":"0a02b9bcb27a1c810696b44d0829710c86d9f5e8"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"9938f0bc1a66054f7cb317a4bca165b9e2e30fb3","unresolved":false,"context_lines":[{"line_number":15762,"context_line":"                               mock_create_domain_and_network,"},{"line_number":15763,"context_line":"                               mock_get_info, mock_inject_data,"},{"line_number":15764,"context_line":"                               power_on\u003dTrue, resize_instance\u003dFalse):"},{"line_number":15765,"context_line":"        \"\"\"Test for nova.virt.libvirt.libvirt_driver.LivirtConnection"},{"line_number":15766,"context_line":"        .finish_migration."},{"line_number":15767,"context_line":"        \"\"\""},{"line_number":15768,"context_line":"        self.flags(use_cow_images\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":18,"id":"7a77a97e_c1507cd7","line":15765,"range":{"start_line":15765,"start_character":38,"end_line":15765,"end_character":69},"updated":"2016-11-22 11:21:10.000000000","message":"s/libvirt_driver.LibvirtConnection/driver.LibvirtDriver/","commit_id":"0a02b9bcb27a1c810696b44d0829710c86d9f5e8"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"87b22df57ac9d1abb4d599d814b7149591bf3d2b","unresolved":false,"context_lines":[{"line_number":15762,"context_line":"                               mock_create_domain_and_network,"},{"line_number":15763,"context_line":"                               mock_get_info, mock_inject_data,"},{"line_number":15764,"context_line":"                               power_on\u003dTrue, resize_instance\u003dFalse):"},{"line_number":15765,"context_line":"        \"\"\"Test for nova.virt.libvirt.libvirt_driver.LivirtConnection"},{"line_number":15766,"context_line":"        .finish_migration."},{"line_number":15767,"context_line":"        \"\"\""},{"line_number":15768,"context_line":"        self.flags(use_cow_images\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":18,"id":"7a77a97e_523efcda","line":15765,"range":{"start_line":15765,"start_character":38,"end_line":15765,"end_character":69},"in_reply_to":"7a77a97e_c1507cd7","updated":"2016-11-22 12:11:56.000000000","message":"True, but not a change in this patch.","commit_id":"0a02b9bcb27a1c810696b44d0829710c86d9f5e8"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"9938f0bc1a66054f7cb317a4bca165b9e2e30fb3","unresolved":false,"context_lines":[{"line_number":15776,"context_line":"            {\u0027config_drive\u0027: str(True),"},{"line_number":15777,"context_line":"             \u0027task_state\u0027: task_states.RESIZE_FINISH,"},{"line_number":15778,"context_line":"             \u0027flavor\u0027: {\u0027swap\u0027: 500}})"},{"line_number":15779,"context_line":"        bdi \u003d {\u0027block_device_mapping\u0027: []}"},{"line_number":15780,"context_line":""},{"line_number":15781,"context_line":"        migration \u003d objects.Migration()"},{"line_number":15782,"context_line":"        migration.source_compute \u003d \u0027fake-source-compute\u0027"}],"source_content_type":"text/x-python","patch_set":18,"id":"7a77a97e_a44e2ebf","line":15779,"range":{"start_line":15779,"start_character":8,"end_line":15779,"end_character":42},"updated":"2016-11-22 11:21:10.000000000","message":"For my own knowledge, why did you add the bdi argument to the \u0027_create_domain_and_network\u0027 call when it wasn\u0027t there before?","commit_id":"0a02b9bcb27a1c810696b44d0829710c86d9f5e8"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"87b22df57ac9d1abb4d599d814b7149591bf3d2b","unresolved":false,"context_lines":[{"line_number":15776,"context_line":"            {\u0027config_drive\u0027: str(True),"},{"line_number":15777,"context_line":"             \u0027task_state\u0027: task_states.RESIZE_FINISH,"},{"line_number":15778,"context_line":"             \u0027flavor\u0027: {\u0027swap\u0027: 500}})"},{"line_number":15779,"context_line":"        bdi \u003d {\u0027block_device_mapping\u0027: []}"},{"line_number":15780,"context_line":""},{"line_number":15781,"context_line":"        migration \u003d objects.Migration()"},{"line_number":15782,"context_line":"        migration.source_compute \u003d \u0027fake-source-compute\u0027"}],"source_content_type":"text/x-python","patch_set":18,"id":"7a77a97e_ade7f13e","line":15779,"range":{"start_line":15779,"start_character":8,"end_line":15779,"end_character":42},"in_reply_to":"7a77a97e_a44e2ebf","updated":"2016-11-22 12:11:56.000000000","message":"It was there before, but it wasn\u0027t checked.","commit_id":"0a02b9bcb27a1c810696b44d0829710c86d9f5e8"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"5221b68afb5fe90ad1bfb727c5002920f93d9dd8","unresolved":false,"context_lines":[{"line_number":15792,"context_line":"        mock_create_domain_and_network.return_value \u003d \\"},{"line_number":15793,"context_line":"            libvirt_guest.Guest(\u0027fake_dom\u0027)"},{"line_number":15794,"context_line":""},{"line_number":15795,"context_line":"        with mock.patch.object(self.drvr, \u0027_disk_resize\u0027):"},{"line_number":15796,"context_line":"            self.drvr.finish_migration("},{"line_number":15797,"context_line":"                          context.get_admin_context(), migration, instance,"},{"line_number":15798,"context_line":"                          disk_info, [], image_meta,"}],"source_content_type":"text/x-python","patch_set":18,"id":"5a74a57a_c962a1ad","line":15795,"updated":"2016-11-23 00:14:11.000000000","message":"I would think we still want to assert that _disk_resize was called or not called being that it\u0027s an input to the test (resize_instance\u003dTrue/False), right?","commit_id":"0a02b9bcb27a1c810696b44d0829710c86d9f5e8"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"cb937f639b971d0a2d6df52ce5277c4b12190991","unresolved":false,"context_lines":[{"line_number":15792,"context_line":"        mock_create_domain_and_network.return_value \u003d \\"},{"line_number":15793,"context_line":"            libvirt_guest.Guest(\u0027fake_dom\u0027)"},{"line_number":15794,"context_line":""},{"line_number":15795,"context_line":"        with mock.patch.object(self.drvr, \u0027_disk_resize\u0027):"},{"line_number":15796,"context_line":"            self.drvr.finish_migration("},{"line_number":15797,"context_line":"                          context.get_admin_context(), migration, instance,"},{"line_number":15798,"context_line":"                          disk_info, [], image_meta,"}],"source_content_type":"text/x-python","patch_set":18,"id":"5a74a57a_cde8bf7f","line":15795,"in_reply_to":"5a74a57a_19f04f90","updated":"2016-11-23 20:55:13.000000000","message":"I see, it\u0027s 5 patches after this one. I guess I\u0027m not used to thinking of several patch series merging as a chunk. It\u0027s unknown how long it will be before patch 6 lands.\n\nI understand you probably worked backward from the end state to get these test refactors and lost some of the test coverage by accident. From my perspective though, I\u0027m reviewing a standalone patch, and a test refactor that loses coverage of a call that\u0027s the basis for the test (\"test_finish_migration_resize\") doesn\u0027t seem right IMHO.","commit_id":"0a02b9bcb27a1c810696b44d0829710c86d9f5e8"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"4cb2d1bc8035c9bcaac4ab89798d25bdecbdec74","unresolved":false,"context_lines":[{"line_number":15792,"context_line":"        mock_create_domain_and_network.return_value \u003d \\"},{"line_number":15793,"context_line":"            libvirt_guest.Guest(\u0027fake_dom\u0027)"},{"line_number":15794,"context_line":""},{"line_number":15795,"context_line":"        with mock.patch.object(self.drvr, \u0027_disk_resize\u0027):"},{"line_number":15796,"context_line":"            self.drvr.finish_migration("},{"line_number":15797,"context_line":"                          context.get_admin_context(), migration, instance,"},{"line_number":15798,"context_line":"                          disk_info, [], image_meta,"}],"source_content_type":"text/x-python","patch_set":18,"id":"3a71b18c_632b5731","line":15795,"in_reply_to":"5a74a57a_3a86b065","updated":"2016-12-01 15:25:20.000000000","message":"Matt, go ahead and just fix this _disk_resize() mock and assert it being called and rebase this series. We\u0027ll push on ahead with it.","commit_id":"0a02b9bcb27a1c810696b44d0829710c86d9f5e8"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"ef870d26a02ed05b798735de35ace350322ddb60","unresolved":false,"context_lines":[{"line_number":15792,"context_line":"        mock_create_domain_and_network.return_value \u003d \\"},{"line_number":15793,"context_line":"            libvirt_guest.Guest(\u0027fake_dom\u0027)"},{"line_number":15794,"context_line":""},{"line_number":15795,"context_line":"        with mock.patch.object(self.drvr, \u0027_disk_resize\u0027):"},{"line_number":15796,"context_line":"            self.drvr.finish_migration("},{"line_number":15797,"context_line":"                          context.get_admin_context(), migration, instance,"},{"line_number":15798,"context_line":"                          disk_info, [], image_meta,"}],"source_content_type":"text/x-python","patch_set":18,"id":"5a74a57a_19f04f90","line":15795,"in_reply_to":"5a74a57a_c962a1ad","updated":"2016-11-23 14:03:48.000000000","message":"Good point. However, _disk_resize dies entirely a couple of patches down the line:\n\n  https://review.openstack.org/#/c/331118/\n\nAny chance you could let this slide and save some electricity in the CI infrastructure? ;)","commit_id":"0a02b9bcb27a1c810696b44d0829710c86d9f5e8"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"b632f7a352bec8ff0bc763409297d12f58ef771c","unresolved":false,"context_lines":[{"line_number":15792,"context_line":"        mock_create_domain_and_network.return_value \u003d \\"},{"line_number":15793,"context_line":"            libvirt_guest.Guest(\u0027fake_dom\u0027)"},{"line_number":15794,"context_line":""},{"line_number":15795,"context_line":"        with mock.patch.object(self.drvr, \u0027_disk_resize\u0027):"},{"line_number":15796,"context_line":"            self.drvr.finish_migration("},{"line_number":15797,"context_line":"                          context.get_admin_context(), migration, instance,"},{"line_number":15798,"context_line":"                          disk_info, [], image_meta,"}],"source_content_type":"text/x-python","patch_set":18,"id":"5a74a57a_3a86b065","line":15795,"in_reply_to":"5a74a57a_cde8bf7f","updated":"2016-11-24 09:47:24.000000000","message":"I just missed it first time round, tbh.\n\nHowever, note that _disk_resize is a no-op, which is why it\u0027s removed down the line anyway. You\u0027re right that, strictly speaking, we\u0027re no longer testing that we\u0027re calling it. However, it doesn\u0027t do anything anyway. I\u0027d prefer not to rebase 10 patches for this if at all possible.","commit_id":"0a02b9bcb27a1c810696b44d0829710c86d9f5e8"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"9938f0bc1a66054f7cb317a4bca165b9e2e30fb3","unresolved":false,"context_lines":[{"line_number":15847,"context_line":"    def test_finish_migration_power_on(self):"},{"line_number":15848,"context_line":"        with mock.patch(\u0027nova.virt.libvirt.guest.Guest.sync_guest_time\u0027"},{"line_number":15849,"context_line":"            ) as mock_guest_time:"},{"line_number":15850,"context_line":"            self._test_finish_migration()"},{"line_number":15851,"context_line":"            self.assertTrue(mock_guest_time.called)"},{"line_number":15852,"context_line":""},{"line_number":15853,"context_line":"    def test_finish_migration_power_off(self):"}],"source_content_type":"text/x-python","patch_set":18,"id":"7a77a97e_1e99f56e","line":15850,"updated":"2016-11-22 11:21:10.000000000","message":"nit: while completely unnecessary, adding \u0027power_on\u003dTrue\u0027 would provide a nice contrast to the subsequent test","commit_id":"0a02b9bcb27a1c810696b44d0829710c86d9f5e8"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"5221b68afb5fe90ad1bfb727c5002920f93d9dd8","unresolved":false,"context_lines":[{"line_number":15847,"context_line":"    def test_finish_migration_power_on(self):"},{"line_number":15848,"context_line":"        with mock.patch(\u0027nova.virt.libvirt.guest.Guest.sync_guest_time\u0027"},{"line_number":15849,"context_line":"            ) as mock_guest_time:"},{"line_number":15850,"context_line":"            self._test_finish_migration()"},{"line_number":15851,"context_line":"            self.assertTrue(mock_guest_time.called)"},{"line_number":15852,"context_line":""},{"line_number":15853,"context_line":"    def test_finish_migration_power_off(self):"}],"source_content_type":"text/x-python","patch_set":18,"id":"5a74a57a_89c18931","line":15850,"in_reply_to":"7a77a97e_1e99f56e","updated":"2016-11-23 00:14:11.000000000","message":"T\u0027would. Along the same thinking, we\u0027d have resize_instance\u003d\u003cvalue\u003e, power_on\u003d\u003cvalue\u003e on the other calls too.","commit_id":"0a02b9bcb27a1c810696b44d0829710c86d9f5e8"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"6fe08c35e90931e4545d93e2721bfbd858af8957","unresolved":false,"context_lines":[{"line_number":15820,"context_line":"                mock.call(local_file(\u0027disk.swap\u0027), 0),"},{"line_number":15821,"context_line":"                mock.call(local_file(\u0027disk.config\u0027), 0),"},{"line_number":15822,"context_line":"            ]"},{"line_number":15823,"context_line":"            mock_disk_resize.assert_has_calls(resize_calls, any_order\u003dTrue)"},{"line_number":15824,"context_line":""},{"line_number":15825,"context_line":"        disks \u003d backend.disks"},{"line_number":15826,"context_line":""}],"source_content_type":"text/x-python","patch_set":19,"id":"3a71b18c_e9f2d699","line":15823,"updated":"2016-12-01 21:48:28.000000000","message":"++ thanks for this addition (even if it dies in a few patches!) :)","commit_id":"dc9b19a2df871b353d21e379d31198b300a14526"}]}