)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6676,"name":"zqfan","email":"aji.zqfan@gmail.com","username":"aji-zqfan"},"change_message_id":"31be59b25f6bd6ed7bc8c65742094753f2cbd585","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Clean unrelated error of two tests"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When running test test_check_alarm_query_constraints and"},{"line_number":10,"context_line":"test_start_singleton, a NoneType error will be raised, that is because the"},{"line_number":11,"context_line":"tests invoked the real aodh.storage.get_connection_from_config method."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Change-Id: I00932eda921d15bb9e59f1f82631e5ad7bb4f93d"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"dab17558_7316d3f2","line":11,"range":{"start_line":9,"start_character":0,"end_line":11,"end_character":70},"updated":"2016-05-12 10:10:45.000000000","message":"what are you talking about? this function has already been mocked","commit_id":"364d751a2d9776287e472a51eef2e1ee9bea76c1"}],"aodh/tests/unit/test_evaluator.py":[{"author":{"_account_id":6676,"name":"zqfan","email":"aji.zqfan@gmail.com","username":"aji-zqfan"},"change_message_id":"f3fe4dcdd758a0e39f63cd3ec540355edb5da52a","unresolved":false,"context_lines":[{"line_number":59,"context_line":"                               group\u003d\u0027coordination\u0027,"},{"line_number":60,"context_line":"                               enforce_type\u003dTrue)"},{"line_number":61,"context_line":""},{"line_number":62,"context_line":"        with mock.patch(\u0027aodh.coordination.PartitionCoordinator\u0027) as m_pc:"},{"line_number":63,"context_line":"            m_pc.return_value.is_active.return_value \u003d coordination_active"},{"line_number":64,"context_line":""},{"line_number":65,"context_line":"            self.addCleanup(self.svc.stop)"}],"source_content_type":"text/x-python","patch_set":1,"id":"dab17558_55b097dd","line":62,"range":{"start_line":62,"start_character":8,"end_line":62,"end_character":74},"updated":"2016-05-13 10:48:20.000000000","message":"add the following line after it, it seems no error:\n\nwith mock.patch.object(self.svc, \u0027_evaluate_assigned_alarms\u0027, mock.MagicMock):","commit_id":"364d751a2d9776287e472a51eef2e1ee9bea76c1"},{"author":{"_account_id":1669,"name":"Julien Danjou","display_name":"jd","email":"julien@danjou.info","username":"jdanjou"},"change_message_id":"de116be25782dff96703c8eafbfcdb46474a40f5","unresolved":false,"context_lines":[{"line_number":64,"context_line":""},{"line_number":65,"context_line":"            self.addCleanup(self.svc.stop)"},{"line_number":66,"context_line":"            self.svc.start()"},{"line_number":67,"context_line":"            time.sleep(1)"},{"line_number":68,"context_line":""},{"line_number":69,"context_line":"        self.svc.partition_coordinator.start.assert_called_once_with()"},{"line_number":70,"context_line":"        self.svc.partition_coordinator.join_group.assert_called_once_with("}],"source_content_type":"text/x-python","patch_set":1,"id":"dab17558_251c0613","line":67,"updated":"2016-05-12 08:34:20.000000000","message":"I don\u0027t get why there is that.","commit_id":"364d751a2d9776287e472a51eef2e1ee9bea76c1"},{"author":{"_account_id":8290,"name":"liusheng","email":"liusheng2048@gmail.com","username":"LiuSheng"},"change_message_id":"44192df2865933b5ac4a3b901cf077a6f91aa798","unresolved":false,"context_lines":[{"line_number":64,"context_line":""},{"line_number":65,"context_line":"            self.addCleanup(self.svc.stop)"},{"line_number":66,"context_line":"            self.svc.start()"},{"line_number":67,"context_line":"            time.sleep(1)"},{"line_number":68,"context_line":""},{"line_number":69,"context_line":"        self.svc.partition_coordinator.start.assert_called_once_with()"},{"line_number":70,"context_line":"        self.svc.partition_coordinator.join_group.assert_called_once_with("}],"source_content_type":"text/x-python","patch_set":1,"id":"dab17558_32208e9b","line":67,"in_reply_to":"dab17558_18ee52ec","updated":"2016-05-13 09:22:43.000000000","message":"yes, that is reason I explained. there is a similar example in SO[1].\n\nBut I think this problem is unrelated with CleanUp method. the, you can find the @mock.patch using a context manager (try and finally) to mock and unmock the target method. yes, if this \"sleep(1)\" make us uncomfortable, let\u0027s mock _evaluate_assigned_alarms()!\n\n[1] http://stackoverflow.com/questions/23757473/how-to-unittest-that-a-thread-is-spawned","commit_id":"364d751a2d9776287e472a51eef2e1ee9bea76c1"},{"author":{"_account_id":6676,"name":"zqfan","email":"aji.zqfan@gmail.com","username":"aji-zqfan"},"change_message_id":"31be59b25f6bd6ed7bc8c65742094753f2cbd585","unresolved":false,"context_lines":[{"line_number":64,"context_line":""},{"line_number":65,"context_line":"            self.addCleanup(self.svc.stop)"},{"line_number":66,"context_line":"            self.svc.start()"},{"line_number":67,"context_line":"            time.sleep(1)"},{"line_number":68,"context_line":""},{"line_number":69,"context_line":"        self.svc.partition_coordinator.start.assert_called_once_with()"},{"line_number":70,"context_line":"        self.svc.partition_coordinator.join_group.assert_called_once_with("}],"source_content_type":"text/x-python","patch_set":1,"id":"dab17558_33ca8b6f","line":67,"in_reply_to":"dab17558_251c0613","updated":"2016-05-12 10:10:45.000000000","message":"+1","commit_id":"364d751a2d9776287e472a51eef2e1ee9bea76c1"},{"author":{"_account_id":8290,"name":"liusheng","email":"liusheng2048@gmail.com","username":"LiuSheng"},"change_message_id":"05f7886ed4d33e900e4bdec754f58401ed74ba56","unresolved":false,"context_lines":[{"line_number":64,"context_line":""},{"line_number":65,"context_line":"            self.addCleanup(self.svc.stop)"},{"line_number":66,"context_line":"            self.svc.start()"},{"line_number":67,"context_line":"            time.sleep(1)"},{"line_number":68,"context_line":""},{"line_number":69,"context_line":"        self.svc.partition_coordinator.start.assert_called_once_with()"},{"line_number":70,"context_line":"        self.svc.partition_coordinator.join_group.assert_called_once_with("}],"source_content_type":"text/x-python","patch_set":1,"id":"dab17558_f0291939","line":67,"in_reply_to":"dab17558_251c0613","updated":"2016-05-12 11:38:04.000000000","message":"Let me try to explain and welcome to correct me ：）\n\n\nThis test started a evaluation service with \u0027aodh.storage.get_connection_from_config\u0027 mocked as a MagicMock, but it will invoke the real get_connection_from_config method and apparently it failed, the error is in aodh.evaluator.AlarmEvaluationService#_evaluate_assigned_alarms() method, I have added a log in the method to print the storage.get_connection_from_config method to confirm this, the log show it was \u003cfunction get_connection_from_config at 0x7fe7c4c22500\u003e than \u003cMagicMock id\u003d\u0027140633411617424\u0027\u003e. so the mocked object didn\u0027t be applied here! this is the reason of the NoneType error, but why ? obviously we have mocked this method.\n\nI found the _evaluate_assigned_alarms() is called in a periodic task, and it run in a thread, not in the main thread. We have mocked the _evaluate_assigned_alarms() with  @mock.patch(), the method was mocked at the entrance of this _do_test_start() method and was unmocked at the exit of this _do_test_start() method, but the thread may be still running and didn\u0027t reach the code location of invoking get_connection_from_config, the MagicMock boject has been reset firstly. so the real get_connection_from_config method will be invoked. but the thread may also finish before the exit of this method, that\u0027s why the NoneType error didn\u0027t occur occasionally.\n\nTo verify this speculation, I added a sleep(1) here and added a sleep(2) in the _evaluate_assigned_alarms() method, this problem occurred inevitably.","commit_id":"364d751a2d9776287e472a51eef2e1ee9bea76c1"},{"author":{"_account_id":8290,"name":"liusheng","email":"liusheng2048@gmail.com","username":"LiuSheng"},"change_message_id":"8714481d0a9eb0cb960b6d58ece45e621e85cf9e","unresolved":false,"context_lines":[{"line_number":64,"context_line":""},{"line_number":65,"context_line":"            self.addCleanup(self.svc.stop)"},{"line_number":66,"context_line":"            self.svc.start()"},{"line_number":67,"context_line":"            time.sleep(1)"},{"line_number":68,"context_line":""},{"line_number":69,"context_line":"        self.svc.partition_coordinator.start.assert_called_once_with()"},{"line_number":70,"context_line":"        self.svc.partition_coordinator.join_group.assert_called_once_with("}],"source_content_type":"text/x-python","patch_set":1,"id":"dab17558_f249e68f","line":67,"in_reply_to":"dab17558_32208e9b","updated":"2016-05-13 09:24:45.000000000","message":"sorry for typo again.","commit_id":"364d751a2d9776287e472a51eef2e1ee9bea76c1"},{"author":{"_account_id":8290,"name":"liusheng","email":"liusheng2048@gmail.com","username":"LiuSheng"},"change_message_id":"f73808b1c5f7e48e425e5b088408c6176fed53ef","unresolved":false,"context_lines":[{"line_number":64,"context_line":""},{"line_number":65,"context_line":"            self.addCleanup(self.svc.stop)"},{"line_number":66,"context_line":"            self.svc.start()"},{"line_number":67,"context_line":"            time.sleep(1)"},{"line_number":68,"context_line":""},{"line_number":69,"context_line":"        self.svc.partition_coordinator.start.assert_called_once_with()"},{"line_number":70,"context_line":"        self.svc.partition_coordinator.join_group.assert_called_once_with("}],"source_content_type":"text/x-python","patch_set":1,"id":"dab17558_d09e6806","line":67,"in_reply_to":"dab17558_423ad2e8","updated":"2016-05-13 03:01:27.000000000","message":"... mybad, s/mocked the _evaluate_assigned_alarms() with/mocked the get_connection_from_config() with/\n\nTo prove more, I have added \"print \u0027get_conn time is %s\u0027 % time.clock()\" statement in get_connection_from_config and added \"print \"unmock time is %s\" % time.clock()\" statement at the end of mock.mock._patch#decorate_callable() method. the testing result is [1], you can see the unmock time is before than the get_connection time. I think I have explained the \"root cause\", so if you have another intelligent \"root cause\", please teach me.\n\n[1] http://paste.openstack.org/show/497004/","commit_id":"364d751a2d9776287e472a51eef2e1ee9bea76c1"},{"author":{"_account_id":6676,"name":"zqfan","email":"aji.zqfan@gmail.com","username":"aji-zqfan"},"change_message_id":"ebbf136876e77b2edef3fdcc411d1976ee9ae0d4","unresolved":false,"context_lines":[{"line_number":64,"context_line":""},{"line_number":65,"context_line":"            self.addCleanup(self.svc.stop)"},{"line_number":66,"context_line":"            self.svc.start()"},{"line_number":67,"context_line":"            time.sleep(1)"},{"line_number":68,"context_line":""},{"line_number":69,"context_line":"        self.svc.partition_coordinator.start.assert_called_once_with()"},{"line_number":70,"context_line":"        self.svc.partition_coordinator.join_group.assert_called_once_with("}],"source_content_type":"text/x-python","patch_set":1,"id":"dab17558_f528ab87","line":67,"in_reply_to":"dab17558_7d8947ca","updated":"2016-05-13 10:43:55.000000000","message":"Sorry, typo is ok as long as it isn\u0027t misleading to me, I\u0027ve done it many times as well.\n\nthe ref is a good example, it shows that we have no full control of thread scheduling, in our case, it is a daemon thread, makes it no way to join it IIUC.\n\nI tested that replace addCleanup() with stop(), it seems work properly\n\nI think mock.patch can only cover the life time of the thread who calls it, but I could be wrong because I don\u0027t experiment it","commit_id":"364d751a2d9776287e472a51eef2e1ee9bea76c1"},{"author":{"_account_id":6676,"name":"zqfan","email":"aji.zqfan@gmail.com","username":"aji-zqfan"},"change_message_id":"81c0785c1f3699ec1b563b1ef1d5ab8f5409ba7d","unresolved":false,"context_lines":[{"line_number":64,"context_line":""},{"line_number":65,"context_line":"            self.addCleanup(self.svc.stop)"},{"line_number":66,"context_line":"            self.svc.start()"},{"line_number":67,"context_line":"            time.sleep(1)"},{"line_number":68,"context_line":""},{"line_number":69,"context_line":"        self.svc.partition_coordinator.start.assert_called_once_with()"},{"line_number":70,"context_line":"        self.svc.partition_coordinator.join_group.assert_called_once_with("}],"source_content_type":"text/x-python","patch_set":1,"id":"dab17558_18ee52ec","line":67,"in_reply_to":"dab17558_d09e6806","updated":"2016-05-13 06:22:32.000000000","message":"IIUC, the trick is when the cleanup is called, it is when the test case is over, spawn a thread is costly than main thread do some checks, so when the test case is over, the _evaluate_assigned_alarm is still running, we need to wait it to stop at least in this cycle, and the mock is reverted, so the real get_connection_From_config is called, here is the error you observed\n\nin the start test cases, we only test if the start() is executed correctly, we don\u0027t care about the spawned threads, so the solutions can be: we mock _evaluate_assigned_alarms as well, or just call stop() right after the start() instead of using addCleanup() to ensure the mock cover the whole life time\n\nso for the several tests, we can save a few seconds when we run test and in gate, no need to wait for something happen while we really don\u0027t care about....\n\nfor example, in test_evaluation_cycle, we really need to wait for the spawned thread to do something, so that wait is necessary, even it maybe a bit short..","commit_id":"364d751a2d9776287e472a51eef2e1ee9bea76c1"},{"author":{"_account_id":6676,"name":"zqfan","email":"aji.zqfan@gmail.com","username":"aji-zqfan"},"change_message_id":"40d8c7e3ed6277b21ccde7134407c0d04611a283","unresolved":false,"context_lines":[{"line_number":64,"context_line":""},{"line_number":65,"context_line":"            self.addCleanup(self.svc.stop)"},{"line_number":66,"context_line":"            self.svc.start()"},{"line_number":67,"context_line":"            time.sleep(1)"},{"line_number":68,"context_line":""},{"line_number":69,"context_line":"        self.svc.partition_coordinator.start.assert_called_once_with()"},{"line_number":70,"context_line":"        self.svc.partition_coordinator.join_group.assert_called_once_with("}],"source_content_type":"text/x-python","patch_set":1,"id":"dab17558_423ad2e8","line":67,"in_reply_to":"dab17558_f0291939","updated":"2016-05-12 13:44:02.000000000","message":"I some kind of remember I have a local patch to deal with same scenario, it is strange why that patch doesn\u0027t show up\n\nanyway, it will be better if you add these explanation to commit message\n\nMaybe I am blind but I don\u0027t think the _evaluate_assgined_alarms is mocked in this method\n\ntime sleep is needed only when we really need to wait for something, to resolve the error you find, we need to find the root cause and resolve it, this sleep is not so reasonable to me as a workaround","commit_id":"364d751a2d9776287e472a51eef2e1ee9bea76c1"},{"author":{"_account_id":8290,"name":"liusheng","email":"liusheng2048@gmail.com","username":"LiuSheng"},"change_message_id":"b7819f9673be9036df9971bd89d12701be83a9eb","unresolved":false,"context_lines":[{"line_number":64,"context_line":""},{"line_number":65,"context_line":"            self.addCleanup(self.svc.stop)"},{"line_number":66,"context_line":"            self.svc.start()"},{"line_number":67,"context_line":"            time.sleep(1)"},{"line_number":68,"context_line":""},{"line_number":69,"context_line":"        self.svc.partition_coordinator.start.assert_called_once_with()"},{"line_number":70,"context_line":"        self.svc.partition_coordinator.join_group.assert_called_once_with("}],"source_content_type":"text/x-python","patch_set":1,"id":"dab17558_7d8947ca","line":67,"in_reply_to":"dab17558_f249e68f","updated":"2016-05-13 09:37:54.000000000","message":"mocking _evaluate_assigned_alarms has the same issue, will find another way.","commit_id":"364d751a2d9776287e472a51eef2e1ee9bea76c1"}]}