)]}'
{"nova/tests/unit/virt/vmwareapi/test_driver_api.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"5bded9831b01aec35389b284f2484836f32f5601","unresolved":false,"context_lines":[{"line_number":1722,"context_line":"                              \u0027_consolidate_vmdk_volume\u0027),"},{"line_number":1723,"context_line":"            mock.patch.object(volumeops.VMwareVolumeOps,"},{"line_number":1724,"context_line":"                              \u0027detach_disk_from_vm\u0027),"},{"line_number":1725,"context_line":"            mock.patch.object(vm_util, \u0027reconfigure_vm\u0027),"},{"line_number":1726,"context_line":"            mock.patch.object(vm_util, \u0027get_vm_extra_config_spec\u0027,"},{"line_number":1727,"context_line":"                              return_value\u003dmock.sentinel.extra_config)"},{"line_number":1728,"context_line":"        ) as (get_vm_ref, get_volume_ref, get_vmdk_backed_disk_device,"}],"source_content_type":"text/x-python","patch_set":5,"id":"fa1b9901_3fa22b4c","line":1725,"updated":"2015-08-19 17:07:16.000000000","message":"Same comment as in test_volumeops - and you see how tightly coupled these tests are now because of all of the lower level mocking going on.  The driver should be mocking volumeops and volumeops should be mocking vm_util (and it\u0027s own methods).  The method should be \"unit\" tested, meaning test the unit that\u0027s changing, which is the single detach method in volumeops, not the other utility/helper methods that it\u0027s adding/using - just mock out those calls, not their internals.  It\u0027s ridiculous that a one line code change in a volumeops method requires this much collateral change in other tests.","commit_id":"8ffed3d92319d00a33c044155dc0944e441d035b"},{"author":{"_account_id":9171,"name":"Vipin Balachandran","email":"vipin.bl@gmail.com","username":"vbala"},"change_message_id":"ced6483001b3513d1421c056e1935955454a44b4","unresolved":false,"context_lines":[{"line_number":1722,"context_line":"                              \u0027_consolidate_vmdk_volume\u0027),"},{"line_number":1723,"context_line":"            mock.patch.object(volumeops.VMwareVolumeOps,"},{"line_number":1724,"context_line":"                              \u0027detach_disk_from_vm\u0027),"},{"line_number":1725,"context_line":"            mock.patch.object(vm_util, \u0027reconfigure_vm\u0027),"},{"line_number":1726,"context_line":"            mock.patch.object(vm_util, \u0027get_vm_extra_config_spec\u0027,"},{"line_number":1727,"context_line":"                              return_value\u003dmock.sentinel.extra_config)"},{"line_number":1728,"context_line":"        ) as (get_vm_ref, get_volume_ref, get_vmdk_backed_disk_device,"}],"source_content_type":"text/x-python","patch_set":5,"id":"fa1b9901_95b99ece","line":1725,"in_reply_to":"fa1b9901_3fa22b4c","updated":"2015-08-20 16:09:38.000000000","message":"Done.\n\n\u003ehow tightly coupled these tests are now\n\nIt was tightly coupled before as well. \n\nIn PS 1, there was no change in this file, because I was not expecting a test to fail in this module.","commit_id":"8ffed3d92319d00a33c044155dc0944e441d035b"}],"nova/tests/unit/virt/vmwareapi/test_vm_util.py":[{"author":{"_account_id":6802,"name":"Joel Coffman","email":"jmc7tp@gmail.com","username":"joel-coffman"},"change_message_id":"1c414ed9e0e0ca53eca84c4397fc5f57bc6cd6d1","unresolved":false,"context_lines":[{"line_number":497,"context_line":"    def test_detach_virtual_disk_destroy_spec(self):"},{"line_number":498,"context_line":"        self._test_detach_virtual_disk_spec(destroy_disk\u003dTrue)"},{"line_number":499,"context_line":""},{"line_number":500,"context_line":"    def test_get_vm_extra_config_spec(self):"},{"line_number":501,"context_line":""},{"line_number":502,"context_line":"        fake_factory \u003d fake.FakeFactory()"},{"line_number":503,"context_line":"        extra_opts \u003d {mock.sentinel.key: mock.sentinel.value}"}],"source_content_type":"text/x-python","patch_set":3,"id":"3a50d1a3_99fc48b9","line":500,"updated":"2015-07-23 14:33:11.000000000","message":"Does not appear to be used?!","commit_id":"822a3da0f950b2be18ffc04b37a63256b0b45174"},{"author":{"_account_id":9171,"name":"Vipin Balachandran","email":"vipin.bl@gmail.com","username":"vbala"},"change_message_id":"31bb935ea2538c73f58c7051214f63a39f498465","unresolved":false,"context_lines":[{"line_number":497,"context_line":"    def test_detach_virtual_disk_destroy_spec(self):"},{"line_number":498,"context_line":"        self._test_detach_virtual_disk_spec(destroy_disk\u003dTrue)"},{"line_number":499,"context_line":""},{"line_number":500,"context_line":"    def test_get_vm_extra_config_spec(self):"},{"line_number":501,"context_line":""},{"line_number":502,"context_line":"        fake_factory \u003d fake.FakeFactory()"},{"line_number":503,"context_line":"        extra_opts \u003d {mock.sentinel.key: mock.sentinel.value}"}],"source_content_type":"text/x-python","patch_set":3,"id":"3a50d1a3_d40a9578","line":500,"in_reply_to":"3a50d1a3_99fc48b9","updated":"2015-07-23 14:46:57.000000000","message":"There was no unit test for public method vm_util.get_vm_extra_config_spec.","commit_id":"822a3da0f950b2be18ffc04b37a63256b0b45174"}],"nova/tests/unit/virt/vmwareapi/test_volumeops.py":[{"author":{"_account_id":6802,"name":"Joel Coffman","email":"jmc7tp@gmail.com","username":"joel-coffman"},"change_message_id":"6ca811ea80008b009cf8fb9ee1161399a593290c","unresolved":false,"context_lines":[{"line_number":185,"context_line":"                                                        instance,"},{"line_number":186,"context_line":"                                                        mock.sentinel.device)"},{"line_number":187,"context_line":"            update_volume_details.assert_called_once_with("},{"line_number":188,"context_line":"                mock.sentinel.vm_ref, connection_info[\u0027data\u0027][\u0027volume_id\u0027], \"\")"},{"line_number":189,"context_line":""},{"line_number":190,"context_line":"    def test_detach_volume_vmdk_invalid(self):"},{"line_number":191,"context_line":"        connection_info \u003d {\u0027driver_volume_type\u0027: \u0027vmdk\u0027,"}],"source_content_type":"text/x-python","patch_set":1,"id":"3a50d1a3_a55d9616","line":188,"updated":"2015-07-22 15:53:37.000000000","message":"I think it would be better to check the vm_ref to ensure that the information has been removed rather than just checking that mock was invoked.","commit_id":"207d1ceb6dd798cee89c20c27deec3c0d5d7a9cf"},{"author":{"_account_id":9171,"name":"Vipin Balachandran","email":"vipin.bl@gmail.com","username":"vbala"},"change_message_id":"4c92e18652ddc7ca1bb49f73a6e1abc671e2b84c","unresolved":false,"context_lines":[{"line_number":185,"context_line":"                                                        instance,"},{"line_number":186,"context_line":"                                                        mock.sentinel.device)"},{"line_number":187,"context_line":"            update_volume_details.assert_called_once_with("},{"line_number":188,"context_line":"                mock.sentinel.vm_ref, connection_info[\u0027data\u0027][\u0027volume_id\u0027], \"\")"},{"line_number":189,"context_line":""},{"line_number":190,"context_line":"    def test_detach_volume_vmdk_invalid(self):"},{"line_number":191,"context_line":"        connection_info \u003d {\u0027driver_volume_type\u0027: \u0027vmdk\u0027,"}],"source_content_type":"text/x-python","patch_set":1,"id":"3a50d1a3_9f768504","line":188,"in_reply_to":"3a50d1a3_a55d9616","updated":"2015-07-23 07:09:19.000000000","message":"Done","commit_id":"207d1ceb6dd798cee89c20c27deec3c0d5d7a9cf"},{"author":{"_account_id":6802,"name":"Joel Coffman","email":"jmc7tp@gmail.com","username":"joel-coffman"},"change_message_id":"1c414ed9e0e0ca53eca84c4397fc5f57bc6cd6d1","unresolved":false,"context_lines":[{"line_number":188,"context_line":"                                                        mock.sentinel.device)"},{"line_number":189,"context_line":"            get_vm_extra_config_spec.assert_called_once_with("},{"line_number":190,"context_line":"                self._volumeops._session.vim.client.factory,"},{"line_number":191,"context_line":"                {\u0027volume-%s\u0027 % connection_info[\u0027data\u0027][\u0027volume_id\u0027]: \"\"})"},{"line_number":192,"context_line":"            reconfigure_vm.assert_called_once_with(self._volumeops._session,"},{"line_number":193,"context_line":"                                                   mock.sentinel.vm_ref,"},{"line_number":194,"context_line":"                                                   mock.sentinel.extra_config)"}],"source_content_type":"text/x-python","patch_set":3,"id":"3a50d1a3_d9b2d005","line":191,"updated":"2015-07-23 14:33:11.000000000","message":"See comment on patch set 1: I think that the test case should check the vm_ref rather than just asserting that the mock was invoked.","commit_id":"822a3da0f950b2be18ffc04b37a63256b0b45174"},{"author":{"_account_id":9171,"name":"Vipin Balachandran","email":"vipin.bl@gmail.com","username":"vbala"},"change_message_id":"ff22fe16d2808c573f972ef211b7e3836ad129c0","unresolved":false,"context_lines":[{"line_number":188,"context_line":"                                                        mock.sentinel.device)"},{"line_number":189,"context_line":"            get_vm_extra_config_spec.assert_called_once_with("},{"line_number":190,"context_line":"                self._volumeops._session.vim.client.factory,"},{"line_number":191,"context_line":"                {\u0027volume-%s\u0027 % connection_info[\u0027data\u0027][\u0027volume_id\u0027]: \"\"})"},{"line_number":192,"context_line":"            reconfigure_vm.assert_called_once_with(self._volumeops._session,"},{"line_number":193,"context_line":"                                                   mock.sentinel.vm_ref,"},{"line_number":194,"context_line":"                                                   mock.sentinel.extra_config)"}],"source_content_type":"text/x-python","patch_set":3,"id":"3a50d1a3_7ade0bde","line":191,"in_reply_to":"3a50d1a3_74d5c1e2","updated":"2015-07-23 15:00:38.000000000","message":"Just to make things clear, the reconfigure_vm invokes vCenter API to update the VM\u0027s extra config and it is already covered by an existing unit test. In this test, we have to ensure reconfigure_vm is invoked with the correct arguments. One of the arguments is the return value of vm_util.get_vm_extra_config_spec (covered by a new unit test in this patch).","commit_id":"822a3da0f950b2be18ffc04b37a63256b0b45174"},{"author":{"_account_id":9171,"name":"Vipin Balachandran","email":"vipin.bl@gmail.com","username":"vbala"},"change_message_id":"31bb935ea2538c73f58c7051214f63a39f498465","unresolved":false,"context_lines":[{"line_number":188,"context_line":"                                                        mock.sentinel.device)"},{"line_number":189,"context_line":"            get_vm_extra_config_spec.assert_called_once_with("},{"line_number":190,"context_line":"                self._volumeops._session.vim.client.factory,"},{"line_number":191,"context_line":"                {\u0027volume-%s\u0027 % connection_info[\u0027data\u0027][\u0027volume_id\u0027]: \"\"})"},{"line_number":192,"context_line":"            reconfigure_vm.assert_called_once_with(self._volumeops._session,"},{"line_number":193,"context_line":"                                                   mock.sentinel.vm_ref,"},{"line_number":194,"context_line":"                                                   mock.sentinel.extra_config)"}],"source_content_type":"text/x-python","patch_set":3,"id":"3a50d1a3_74d5c1e2","line":191,"in_reply_to":"3a50d1a3_d9b2d005","updated":"2015-07-23 14:46:57.000000000","message":"I don\u0027t understand the vm_ref check you mentioned.\n\n_update_volume_details is calling two public methods: vm_util.get_vm_extra_config_spec and vm_util.reconfigure_vm. IMO, we have to assert whether public methods are called with correct arguments which is exactly what I am doing here.\n\nThere is already a unit test for reconfigure_vm. In this patch I added unit test for vm_util.get_vm_extra_config_spec.\n\nI hope this clears the confusion.","commit_id":"822a3da0f950b2be18ffc04b37a63256b0b45174"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"5bded9831b01aec35389b284f2484836f32f5601","unresolved":false,"context_lines":[{"line_number":155,"context_line":"                              return_value\u003dvmdk_info),"},{"line_number":156,"context_line":"            mock.patch.object(self._volumeops, \u0027_consolidate_vmdk_volume\u0027),"},{"line_number":157,"context_line":"            mock.patch.object(self._volumeops, \u0027detach_disk_from_vm\u0027),"},{"line_number":158,"context_line":"            mock.patch.object(vm_util, \u0027reconfigure_vm\u0027),"},{"line_number":159,"context_line":"            mock.patch.object(vm_util, \u0027get_vm_extra_config_spec\u0027,"},{"line_number":160,"context_line":"                              return_value\u003dmock.sentinel.extra_config)"},{"line_number":161,"context_line":"        ) as (get_vm_ref, get_volume_ref, get_vmdk_backed_disk_device,"}],"source_content_type":"text/x-python","patch_set":5,"id":"fa1b9901_5f14bf9e","line":158,"updated":"2015-08-19 17:07:16.000000000","message":"You should just mock out the call to self._volumeops._update_volume_details, not the internals of it - that\u0027s overly complicating the unit test and making this test, which should only be testing the detach volume method in volumeops, to the utilities called in the _update_volume_details method - the _update_volume_details method should just have it\u0027s own unit test if it doesn\u0027t already.","commit_id":"8ffed3d92319d00a33c044155dc0944e441d035b"},{"author":{"_account_id":9171,"name":"Vipin Balachandran","email":"vipin.bl@gmail.com","username":"vbala"},"change_message_id":"ced6483001b3513d1421c056e1935955454a44b4","unresolved":false,"context_lines":[{"line_number":155,"context_line":"                              return_value\u003dvmdk_info),"},{"line_number":156,"context_line":"            mock.patch.object(self._volumeops, \u0027_consolidate_vmdk_volume\u0027),"},{"line_number":157,"context_line":"            mock.patch.object(self._volumeops, \u0027detach_disk_from_vm\u0027),"},{"line_number":158,"context_line":"            mock.patch.object(vm_util, \u0027reconfigure_vm\u0027),"},{"line_number":159,"context_line":"            mock.patch.object(vm_util, \u0027get_vm_extra_config_spec\u0027,"},{"line_number":160,"context_line":"                              return_value\u003dmock.sentinel.extra_config)"},{"line_number":161,"context_line":"        ) as (get_vm_ref, get_volume_ref, get_vmdk_backed_disk_device,"}],"source_content_type":"text/x-python","patch_set":5,"id":"fa1b9901_35d0d2f5","line":158,"in_reply_to":"fa1b9901_5f14bf9e","updated":"2015-08-20 16:09:38.000000000","message":"Done\n\nIt was doing just that in PS1. I had to change the mocks (here and in test_driver_api)  to address a review comment.","commit_id":"8ffed3d92319d00a33c044155dc0944e441d035b"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"dd34bb13d65d59a1eac9fb2bc650095590829d4f","unresolved":false,"context_lines":[{"line_number":140,"context_line":"            self.assertTrue(get_vmdk_info.called)"},{"line_number":141,"context_line":""},{"line_number":142,"context_line":"    def test_update_volume_details(self):"},{"line_number":143,"context_line":"        with contextlib.nested("},{"line_number":144,"context_line":"            mock.patch.object(vm_util, \u0027get_vm_extra_config_spec\u0027,"},{"line_number":145,"context_line":"                              return_value\u003dmock.sentinel.extra_config),"},{"line_number":146,"context_line":"            mock.patch.object(vm_util, \u0027reconfigure_vm\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"fa1b9901_1c8ec727","line":143,"updated":"2015-08-22 00:48:23.000000000","message":"nit: I\u0027d prefer to just decorate the test method with these since it\u0027s a utility module so mocking via method decorator is easy, then you can avoid the nesting below.","commit_id":"d82b0315b6f13e652c5e7b30a63f8a4208455754"},{"author":{"_account_id":9171,"name":"Vipin Balachandran","email":"vipin.bl@gmail.com","username":"vbala"},"change_message_id":"ea9aaae8795b60ec9d9f9f84ebf094bc17b7dc7d","unresolved":false,"context_lines":[{"line_number":140,"context_line":"            self.assertTrue(get_vmdk_info.called)"},{"line_number":141,"context_line":""},{"line_number":142,"context_line":"    def test_update_volume_details(self):"},{"line_number":143,"context_line":"        with contextlib.nested("},{"line_number":144,"context_line":"            mock.patch.object(vm_util, \u0027get_vm_extra_config_spec\u0027,"},{"line_number":145,"context_line":"                              return_value\u003dmock.sentinel.extra_config),"},{"line_number":146,"context_line":"            mock.patch.object(vm_util, \u0027reconfigure_vm\u0027)"}],"source_content_type":"text/x-python","patch_set":6,"id":"fa1b9901_4e09be30","line":143,"in_reply_to":"fa1b9901_1c8ec727","updated":"2015-08-24 09:36:21.000000000","message":"Done","commit_id":"d82b0315b6f13e652c5e7b30a63f8a4208455754"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"e6d93484b8f858e1963b116628a7ed5c27437ff9","unresolved":false,"context_lines":[{"line_number":140,"context_line":"            self.assertTrue(get_vmdk_info.called)"},{"line_number":141,"context_line":""},{"line_number":142,"context_line":"    @mock.patch.object(vm_util, \u0027get_vm_extra_config_spec\u0027,"},{"line_number":143,"context_line":"                       return_value\u003dmock.sentinel.extra_config)"},{"line_number":144,"context_line":"    @mock.patch.object(vm_util, \u0027reconfigure_vm\u0027)"},{"line_number":145,"context_line":"    def test_update_volume_details(self, reconfigure_vm,"},{"line_number":146,"context_line":"                                   get_vm_extra_config_spec):"}],"source_content_type":"text/x-python","patch_set":7,"id":"fa1b9901_c8905b51","line":143,"updated":"2015-08-24 12:42:36.000000000","message":"Nit, I find these things a better done as part of the \"setup\" phase in the body of the unit test.","commit_id":"88721a65b5d2e07bba0e2b405c3cd93491d731dc"},{"author":{"_account_id":7461,"name":"Paul Murray","email":"pmurray@hpe.com","username":"ptm"},"change_message_id":"0f100ab9aa67a73954424c8ec8eacc5ca9179461","unresolved":false,"context_lines":[{"line_number":160,"context_line":""},{"line_number":161,"context_line":"        vmdk_info \u003d vm_util.VmdkInfo(\u0027fake-path\u0027, \u0027lsiLogic\u0027, \u0027thin\u0027,"},{"line_number":162,"context_line":"                                     1024, \u0027fake-device\u0027)"},{"line_number":163,"context_line":"        with contextlib.nested("},{"line_number":164,"context_line":"            mock.patch.object(vm_util, \u0027get_vm_ref\u0027,"},{"line_number":165,"context_line":"                              return_value\u003dmock.sentinel.vm_ref),"},{"line_number":166,"context_line":"            mock.patch.object(self._volumeops, \u0027_get_volume_ref\u0027,"}],"source_content_type":"text/x-python","patch_set":7,"id":"fa1b9901_62204ef8","line":163,"updated":"2015-08-24 12:04:49.000000000","message":"nit: this could be done with decorators too. The get_vmdk_info.return_value can be set inside the method.","commit_id":"88721a65b5d2e07bba0e2b405c3cd93491d731dc"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"e6d93484b8f858e1963b116628a7ed5c27437ff9","unresolved":false,"context_lines":[{"line_number":160,"context_line":""},{"line_number":161,"context_line":"        vmdk_info \u003d vm_util.VmdkInfo(\u0027fake-path\u0027, \u0027lsiLogic\u0027, \u0027thin\u0027,"},{"line_number":162,"context_line":"                                     1024, \u0027fake-device\u0027)"},{"line_number":163,"context_line":"        with contextlib.nested("},{"line_number":164,"context_line":"            mock.patch.object(vm_util, \u0027get_vm_ref\u0027,"},{"line_number":165,"context_line":"                              return_value\u003dmock.sentinel.vm_ref),"},{"line_number":166,"context_line":"            mock.patch.object(self._volumeops, \u0027_get_volume_ref\u0027,"}],"source_content_type":"text/x-python","patch_set":7,"id":"fa1b9901_08b733f6","line":163,"in_reply_to":"fa1b9901_62204ef8","updated":"2015-08-24 12:42:36.000000000","message":"+1","commit_id":"88721a65b5d2e07bba0e2b405c3cd93491d731dc"}],"nova/virt/vmwareapi/volumeops.py":[{"author":{"_account_id":6802,"name":"Joel Coffman","email":"jmc7tp@gmail.com","username":"joel-coffman"},"change_message_id":"1c414ed9e0e0ca53eca84c4397fc5f57bc6cd6d1","unresolved":false,"context_lines":[{"line_number":513,"context_line":"        self.detach_disk_from_vm(vm_ref, instance, device)"},{"line_number":514,"context_line":""},{"line_number":515,"context_line":"        # Remove key-value pair \u003cvolume_id, vmdk_uuid\u003e from instance\u0027s"},{"line_number":516,"context_line":"        # extra config. Setting value to empty string will remove the key."},{"line_number":517,"context_line":"        self._update_volume_details(vm_ref, data[\u0027volume_id\u0027], \"\")"},{"line_number":518,"context_line":""},{"line_number":519,"context_line":"        LOG.debug(\"Detached VMDK: %s\", connection_info, instance\u003dinstance)"}],"source_content_type":"text/x-python","patch_set":3,"id":"3a50d1a3_1977d8a0","line":516,"updated":"2015-07-23 14:33:11.000000000","message":"Thanks for the clarifying comment!","commit_id":"822a3da0f950b2be18ffc04b37a63256b0b45174"}]}