)]}'
{"nova/tests/unit/compute/test_tracker.py":[{"author":{"_account_id":8688,"name":"Alexis Lee","email":"openstack@lxsli.co.uk","username":"lxsli"},"change_message_id":"44a9a58abb12629af0e7c75308cddfc06643a53a","unresolved":false,"context_lines":[{"line_number":1146,"context_line":"        self.assertEqual(disk_used, self.rt.compute_node.local_gb_used)"},{"line_number":1147,"context_line":"        self.assertEqual(self.instance.memory_mb,"},{"line_number":1148,"context_line":"                         self.rt.compute_node.memory_mb_used)"},{"line_number":1149,"context_line":"        self.assertEqual(1, self.rt.compute_node.running_vms)"},{"line_number":1150,"context_line":""},{"line_number":1151,"context_line":"    @mock.patch(\u0027nova.objects.InstancePCIRequests.get_by_instance_uuid\u0027)"},{"line_number":1152,"context_line":"    @mock.patch(\u0027nova.objects.MigrationList.get_in_progress_by_host_and_node\u0027)"}],"source_content_type":"text/x-python","patch_set":2,"id":"fa1b9901_b91229b3","line":1149,"updated":"2015-08-18 10:11:36.000000000","message":"where did the NUMA check go?","commit_id":"f7f58a34b42bb32d3c544dbd782925a6eb5121fb"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"69cbed8c4214b82f2abbd53505f5b27fbfc9f3b1","unresolved":false,"context_lines":[{"line_number":1146,"context_line":"        self.assertEqual(disk_used, self.rt.compute_node.local_gb_used)"},{"line_number":1147,"context_line":"        self.assertEqual(self.instance.memory_mb,"},{"line_number":1148,"context_line":"                         self.rt.compute_node.memory_mb_used)"},{"line_number":1149,"context_line":"        self.assertEqual(1, self.rt.compute_node.running_vms)"},{"line_number":1150,"context_line":""},{"line_number":1151,"context_line":"    @mock.patch(\u0027nova.objects.InstancePCIRequests.get_by_instance_uuid\u0027)"},{"line_number":1152,"context_line":"    @mock.patch(\u0027nova.objects.MigrationList.get_in_progress_by_host_and_node\u0027)"}],"source_content_type":"text/x-python","patch_set":2,"id":"fa1b9901_3986cbdb","line":1149,"in_reply_to":"fa1b9901_b91229b3","updated":"2015-08-19 16:30:02.000000000","message":"It\u0027s not germane to this test, which is that the context manager\u0027s __exit__ functions the same as if one instantiated the claim object normally without the with block. We test the NUMA stuff elsewhere, we don\u0027t need to repeat a NUMA test on every variation of the claim call.","commit_id":"f7f58a34b42bb32d3c544dbd782925a6eb5121fb"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"3ed83a0f00f2929b49cb0eff55c28b9a41cd255c","unresolved":false,"context_lines":[{"line_number":1019,"context_line":""},{"line_number":1020,"context_line":"    def setUp(self):"},{"line_number":1021,"context_line":"        super(TestInstanceClaim, self).setUp()"},{"line_number":1022,"context_line":"        self.flags(reserved_host_disk_mb\u003d0, reserved_host_memory_mb\u003d0)"},{"line_number":1023,"context_line":""},{"line_number":1024,"context_line":"        self._setup_rt()"},{"line_number":1025,"context_line":"        self.rt.compute_node \u003d copy.deepcopy(_COMPUTE_NODE_FIXTURES[0])"}],"source_content_type":"text/x-python","patch_set":6,"id":"fa1b9901_db78b280","line":1022,"updated":"2015-08-26 12:35:03.000000000","message":"I have a few worries here: \n- #1 reserved_host_disk_mb is already defaulted to 0, no need to flag it again\n- #2 (the main one) you provide a opts change in a setUp() method, I definitely prefer to have it per unittest because it could create some side effects for the other unittests\n- #3 I understand that you\u0027re adding it because it was done that way in test_resource_tracker, but that\u0027s a very bad idea IMHO\n- #4 a comment on why we need this flag seems pretty useful since those flags are only checked in a southbound private method called by update_available_resources(). Why not mocking update_available_resources like we did for the other stuff ?","commit_id":"3b524fb88ea0f249bb8ac8ea35d39fd0649d34a1"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"f41c2f7b5997af467cadcdc5bba56e9bcd4d2ff8","unresolved":false,"context_lines":[{"line_number":1019,"context_line":""},{"line_number":1020,"context_line":"    def setUp(self):"},{"line_number":1021,"context_line":"        super(TestInstanceClaim, self).setUp()"},{"line_number":1022,"context_line":"        self.flags(reserved_host_disk_mb\u003d0, reserved_host_memory_mb\u003d0)"},{"line_number":1023,"context_line":""},{"line_number":1024,"context_line":"        self._setup_rt()"},{"line_number":1025,"context_line":"        self.rt.compute_node \u003d copy.deepcopy(_COMPUTE_NODE_FIXTURES[0])"}],"source_content_type":"text/x-python","patch_set":6,"id":"da20952f_c49c5fc2","line":1022,"in_reply_to":"da20952f_5d628b3f","updated":"2015-08-30 17:28:49.000000000","message":"Yes, that is correct, Dan. The calculations would fail if the flag values were different.","commit_id":"3b524fb88ea0f249bb8ac8ea35d39fd0649d34a1"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"7c44e6521aca27b059d4f79986a88e94476233dc","unresolved":false,"context_lines":[{"line_number":1019,"context_line":""},{"line_number":1020,"context_line":"    def setUp(self):"},{"line_number":1021,"context_line":"        super(TestInstanceClaim, self).setUp()"},{"line_number":1022,"context_line":"        self.flags(reserved_host_disk_mb\u003d0, reserved_host_memory_mb\u003d0)"},{"line_number":1023,"context_line":""},{"line_number":1024,"context_line":"        self._setup_rt()"},{"line_number":1025,"context_line":"        self.rt.compute_node \u003d copy.deepcopy(_COMPUTE_NODE_FIXTURES[0])"}],"source_content_type":"text/x-python","patch_set":6,"id":"da20952f_34945641","line":1022,"in_reply_to":"da20952f_c49c5fc2","updated":"2015-08-31 15:19:16.000000000","message":"I understood why you placed that here, my question was rather : what was the scope of that ? I mean, the other tests were there before adding it, right ? So I\u0027m wondering why they would still need that, hence my thoughts \u003d\u003e if that\u0027s only for the test you\u0027re adding, then it should be in the test itself.\n\nMaybe I\u0027m wrong, I just need to understand if that\u0027s also needed for the other existing tests.\n\n\nAlso, like I said, I wonder why you\u0027re not mocking update_available_resources() in the test and returning what you want. If so, that would prevent to modify the flags, right ?","commit_id":"3b524fb88ea0f249bb8ac8ea35d39fd0649d34a1"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"93e5160d23dea33391cd3da08768a15645927e62","unresolved":false,"context_lines":[{"line_number":1019,"context_line":""},{"line_number":1020,"context_line":"    def setUp(self):"},{"line_number":1021,"context_line":"        super(TestInstanceClaim, self).setUp()"},{"line_number":1022,"context_line":"        self.flags(reserved_host_disk_mb\u003d0, reserved_host_memory_mb\u003d0)"},{"line_number":1023,"context_line":""},{"line_number":1024,"context_line":"        self._setup_rt()"},{"line_number":1025,"context_line":"        self.rt.compute_node \u003d copy.deepcopy(_COMPUTE_NODE_FIXTURES[0])"}],"source_content_type":"text/x-python","patch_set":6,"id":"da20952f_5d628b3f","line":1022,"in_reply_to":"fa1b9901_db78b280","updated":"2015-08-28 18:35:02.000000000","message":"If the execution of the test depends on the value, then setting it is goodness, IMHO. If we change the default later and this test fails in a hard-to-discover way, then that\u0027s not ideal, IMHO.\n\nPresumably all the tests in this class require it set \u003d0? If so, then setting it in setUp() is correct. If it\u0027s only the new one below, then I\u0027d ask... can we not set it there? Is it getting clobbered and not restored somewhere else?","commit_id":"3b524fb88ea0f249bb8ac8ea35d39fd0649d34a1"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"3ed83a0f00f2929b49cb0eff55c28b9a41cd255c","unresolved":false,"context_lines":[{"line_number":1141,"context_line":"        with mock.patch(\u0027nova.objects.InstanceList\u0027"},{"line_number":1142,"context_line":"                        \u0027.get_by_host_and_node\u0027) as mock_get_all:"},{"line_number":1143,"context_line":"            mock_get_all.return_value \u003d [self.instance]"},{"line_number":1144,"context_line":"            self.rt.update_available_resource(self.ctx)"},{"line_number":1145,"context_line":""},{"line_number":1146,"context_line":"        self.assertEqual(disk_used, self.rt.compute_node.local_gb_used)"},{"line_number":1147,"context_line":"        self.assertEqual(self.instance.memory_mb,"}],"source_content_type":"text/x-python","patch_set":6,"id":"fa1b9901_5b0022ab","line":1144,"updated":"2015-08-26 12:35:03.000000000","message":"here you\u0027re testing another stuff right ? I mean, the scope of test_claim_context_manager is to test the context manager, not really another call to update_avail_resource(), right?\n\nI know that those 2 calls were imbricated in the existing unittest, I just wonder why it\u0027s necessary to provide that here too","commit_id":"3b524fb88ea0f249bb8ac8ea35d39fd0649d34a1"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"7c44e6521aca27b059d4f79986a88e94476233dc","unresolved":false,"context_lines":[{"line_number":1141,"context_line":"        with mock.patch(\u0027nova.objects.InstanceList\u0027"},{"line_number":1142,"context_line":"                        \u0027.get_by_host_and_node\u0027) as mock_get_all:"},{"line_number":1143,"context_line":"            mock_get_all.return_value \u003d [self.instance]"},{"line_number":1144,"context_line":"            self.rt.update_available_resource(self.ctx)"},{"line_number":1145,"context_line":""},{"line_number":1146,"context_line":"        self.assertEqual(disk_used, self.rt.compute_node.local_gb_used)"},{"line_number":1147,"context_line":"        self.assertEqual(self.instance.memory_mb,"}],"source_content_type":"text/x-python","patch_set":6,"id":"da20952f_102fd588","line":1144,"in_reply_to":"da20952f_1dcf332a","updated":"2015-08-31 15:19:16.000000000","message":"Well, the main problem is that the unittest is calling update_available_resource() within a context manager, without mocking it, which means that any failure related to that method would also hit that unittest.\n\nI just wonder here why we couldn\u0027t just mock update_available_resource() so we would just verify the context manager by itself","commit_id":"3b524fb88ea0f249bb8ac8ea35d39fd0649d34a1"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"93e5160d23dea33391cd3da08768a15645927e62","unresolved":false,"context_lines":[{"line_number":1141,"context_line":"        with mock.patch(\u0027nova.objects.InstanceList\u0027"},{"line_number":1142,"context_line":"                        \u0027.get_by_host_and_node\u0027) as mock_get_all:"},{"line_number":1143,"context_line":"            mock_get_all.return_value \u003d [self.instance]"},{"line_number":1144,"context_line":"            self.rt.update_available_resource(self.ctx)"},{"line_number":1145,"context_line":""},{"line_number":1146,"context_line":"        self.assertEqual(disk_used, self.rt.compute_node.local_gb_used)"},{"line_number":1147,"context_line":"        self.assertEqual(self.instance.memory_mb,"}],"source_content_type":"text/x-python","patch_set":6,"id":"da20952f_1dcf332a","line":1144,"in_reply_to":"fa1b9901_5b0022ab","updated":"2015-08-28 18:35:02.000000000","message":"I don\u0027t think I follow this... He\u0027s checking that the proper things happened during the exit path of the above context manager-wrapped call right? This seems okay to me...","commit_id":"3b524fb88ea0f249bb8ac8ea35d39fd0649d34a1"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"01635ba475cd1354e00d0ab24b0228d3151def9c","unresolved":false,"context_lines":[{"line_number":966,"context_line":""},{"line_number":967,"context_line":"    def setUp(self):"},{"line_number":968,"context_line":"        super(TestInstanceClaim, self).setUp()"},{"line_number":969,"context_line":"        self.flags(reserved_host_disk_mb\u003d0, reserved_host_memory_mb\u003d0)"},{"line_number":970,"context_line":""},{"line_number":971,"context_line":"        self._setup_rt()"},{"line_number":972,"context_line":"        self.rt.compute_node \u003d copy.deepcopy(_COMPUTE_NODE_FIXTURES[0])"}],"source_content_type":"text/x-python","patch_set":9,"id":"da20952f_50954d84","line":969,"updated":"2015-08-31 15:21:39.000000000","message":"see my comment on PS6","commit_id":"a60569a18dca336f8feaf229ab077bbb698944ed"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"01635ba475cd1354e00d0ab24b0228d3151def9c","unresolved":false,"context_lines":[{"line_number":1092,"context_line":"        with mock.patch(\u0027nova.objects.InstanceList\u0027"},{"line_number":1093,"context_line":"                        \u0027.get_by_host_and_node\u0027) as mock_get_all:"},{"line_number":1094,"context_line":"            mock_get_all.return_value \u003d [self.instance]"},{"line_number":1095,"context_line":"            self.rt.update_available_resource(self.ctx)"},{"line_number":1096,"context_line":""},{"line_number":1097,"context_line":"        self.assertEqual(disk_used, self.rt.compute_node.local_gb_used)"},{"line_number":1098,"context_line":"        self.assertEqual(self.instance.memory_mb,"}],"source_content_type":"text/x-python","patch_set":9,"id":"da20952f_50dc6d72","line":1095,"updated":"2015-08-31 15:21:39.000000000","message":"follow-up on PS6","commit_id":"a60569a18dca336f8feaf229ab077bbb698944ed"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"649cff19a816e0e16de00fbe2e9f28a86bfe9c35","unresolved":false,"context_lines":[{"line_number":1143,"context_line":"            try:"},{"line_number":1144,"context_line":"                with self.rt.instance_claim(self.ctx, self.instance, None):"},{"line_number":1145,"context_line":"                    raise Exception"},{"line_number":1146,"context_line":"            except:  # noqa"},{"line_number":1147,"context_line":"                # Ignore the exception. Just make sure below that the abort()"},{"line_number":1148,"context_line":"                # method of the claim object was called (and the resulting"},{"line_number":1149,"context_line":"                # resources reset to the pre-claimed amounts)"}],"source_content_type":"text/x-python","patch_set":11,"id":"ba15a1d1_744ee197","line":1146,"updated":"2015-09-21 14:25:14.000000000","message":"I usually dislike catching any base Exception like that but rather make sure that we correctly catch the real one, but given your comment below, I think it\u0027s acceptable","commit_id":"980f7178a7066ea51af2e885009a51b93eee69b9"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"c6d0199240e66632c7657a323d837de6dc403bcf","unresolved":false,"context_lines":[{"line_number":1143,"context_line":"            try:"},{"line_number":1144,"context_line":"                with self.rt.instance_claim(self.ctx, self.instance, None):"},{"line_number":1145,"context_line":"                    raise Exception"},{"line_number":1146,"context_line":"            except:  # noqa"},{"line_number":1147,"context_line":"                # Ignore the exception. Just make sure below that the abort()"},{"line_number":1148,"context_line":"                # method of the claim object was called (and the resulting"},{"line_number":1149,"context_line":"                # resources reset to the pre-claimed amounts)"}],"source_content_type":"text/x-python","patch_set":11,"id":"ba15a1d1_a4e92b81","line":1146,"in_reply_to":"ba15a1d1_744ee197","updated":"2015-09-23 20:26:51.000000000","message":"This would be better if you raised test.TestingException and caught that specific exception.","commit_id":"980f7178a7066ea51af2e885009a51b93eee69b9"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"eb3b9d412798070477f567f2a1a56fc9f1094f56","unresolved":false,"context_lines":[{"line_number":1143,"context_line":"            try:"},{"line_number":1144,"context_line":"                with self.rt.instance_claim(self.ctx, self.instance, None):"},{"line_number":1145,"context_line":"                    raise Exception"},{"line_number":1146,"context_line":"            except:  # noqa"},{"line_number":1147,"context_line":"                # Ignore the exception. Just make sure below that the abort()"},{"line_number":1148,"context_line":"                # method of the claim object was called (and the resulting"},{"line_number":1149,"context_line":"                # resources reset to the pre-claimed amounts)"}],"source_content_type":"text/x-python","patch_set":11,"id":"7a2fa921_6a722daf","line":1146,"in_reply_to":"ba15a1d1_a4e92b81","updated":"2015-10-07 18:21:54.000000000","message":"Agreed.","commit_id":"980f7178a7066ea51af2e885009a51b93eee69b9"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"0d86d2ec43a64dffc60fcf070cfd63193761c0a5","unresolved":false,"context_lines":[{"line_number":1240,"context_line":"        self.assertEqual(0, self.rt.compute_node.running_vms)"},{"line_number":1241,"context_line":""},{"line_number":1242,"context_line":"        @mock.patch.object(self.instance, \u0027save\u0027)"},{"line_number":1243,"context_line":"        def _doit(mock_save):"},{"line_number":1244,"context_line":"            with self.rt.instance_claim(self.ctx, self.instance, None):"},{"line_number":1245,"context_line":"                # Raise an exception. Just make sure below that the abort()"},{"line_number":1246,"context_line":"                # method of the claim object was called (and the resulting"}],"source_content_type":"text/x-python","patch_set":12,"id":"7a2fa921_aa30459f","line":1243,"updated":"2015-10-07 18:30:10.000000000","message":"thanks, it\u0027s better than just a nested contextmanager","commit_id":"7e546c50cb13335570be551071601a81eb5829cf"}]}