)]}'
{"ironic/tests/unit/common/test_rpc.py":[{"author":{"_account_id":10697,"name":"Thiago Paiva Brito","email":"thiagop@lsd.ufcg.edu.br","username":"thiagop","inactive":true},"change_message_id":"92b340e1655ab37980eb03b3196cead677f5229d","unresolved":false,"context_lines":[{"line_number":32,"context_line":"        rpc.TRANSPORT \u003d None"},{"line_number":33,"context_line":"        rpc.NOTIFICATION_TRANSPORT \u003d None"},{"line_number":34,"context_line":"        rpc.NOTIFIER \u003d None"},{"line_number":35,"context_line":"        mock_get_transport.return_value \u003d mock.MagicMock()"},{"line_number":36,"context_line":"        mock_get_notification.return_value \u003d mock.MagicMock()"},{"line_number":37,"context_line":"        mock_notifier.return_value \u003d mock.MagicMock()"},{"line_number":38,"context_line":"        rpc.init(CONF)"},{"line_number":39,"context_line":"        self.assertEqual(mock_get_transport.return_value, rpc.TRANSPORT)"},{"line_number":40,"context_line":"        self.assertEqual(mock_get_notification.return_value,"}],"source_content_type":"text/x-python","patch_set":1,"id":"9abb7d3a_1ac3788a","line":37,"range":{"start_line":35,"start_character":8,"end_line":37,"end_character":53},"updated":"2016-05-31 20:20:09.000000000","message":"You don\u0027t have to initialize these guys with MagicMock() when using patch() or patch.object(). The tests passes, but I\u0027d see it removed just not to keep unused code.","commit_id":"3ff28077741e4f880b9d5a5d3751903ba654dbad"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"dc09c5fac23a0b66793213a50372e4f3d2db649a","unresolved":false,"context_lines":[{"line_number":32,"context_line":"        rpc.TRANSPORT \u003d None"},{"line_number":33,"context_line":"        rpc.NOTIFICATION_TRANSPORT \u003d None"},{"line_number":34,"context_line":"        rpc.NOTIFIER \u003d None"},{"line_number":35,"context_line":"        mock_get_transport.return_value \u003d mock.MagicMock()"},{"line_number":36,"context_line":"        mock_get_notification.return_value \u003d mock.MagicMock()"},{"line_number":37,"context_line":"        mock_notifier.return_value \u003d mock.MagicMock()"},{"line_number":38,"context_line":"        rpc.init(CONF)"},{"line_number":39,"context_line":"        self.assertEqual(mock_get_transport.return_value, rpc.TRANSPORT)"},{"line_number":40,"context_line":"        self.assertEqual(mock_get_notification.return_value,"}],"source_content_type":"text/x-python","patch_set":1,"id":"9abb7d3a_b8ea0bd9","line":37,"range":{"start_line":35,"start_character":8,"end_line":37,"end_character":53},"in_reply_to":"9abb7d3a_1ac3788a","updated":"2016-06-01 14:27:25.000000000","message":"Done","commit_id":"3ff28077741e4f880b9d5a5d3751903ba654dbad"},{"author":{"_account_id":10697,"name":"Thiago Paiva Brito","email":"thiagop@lsd.ufcg.edu.br","username":"thiagop","inactive":true},"change_message_id":"92b340e1655ab37980eb03b3196cead677f5229d","unresolved":false,"context_lines":[{"line_number":36,"context_line":"        mock_get_notification.return_value \u003d mock.MagicMock()"},{"line_number":37,"context_line":"        mock_notifier.return_value \u003d mock.MagicMock()"},{"line_number":38,"context_line":"        rpc.init(CONF)"},{"line_number":39,"context_line":"        self.assertEqual(mock_get_transport.return_value, rpc.TRANSPORT)"},{"line_number":40,"context_line":"        self.assertEqual(mock_get_notification.return_value,"},{"line_number":41,"context_line":"                         rpc.NOTIFICATION_TRANSPORT)"},{"line_number":42,"context_line":"        self.assertTrue(mock_serializer.called)"},{"line_number":43,"context_line":"        self.assertEqual(mock_notifier.return_value, rpc.NOTIFIER)"},{"line_number":44,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"9abb7d3a_ba898c28","line":41,"range":{"start_line":39,"start_character":8,"end_line":41,"end_character":52},"updated":"2016-05-31 20:20:09.000000000","message":"I\u0027d use \"assertNotNone\" to check this since check \"MagicMock \u003d\u003d MagicMock\" sounds strange...","commit_id":"3ff28077741e4f880b9d5a5d3751903ba654dbad"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"dc09c5fac23a0b66793213a50372e4f3d2db649a","unresolved":false,"context_lines":[{"line_number":36,"context_line":"        mock_get_notification.return_value \u003d mock.MagicMock()"},{"line_number":37,"context_line":"        mock_notifier.return_value \u003d mock.MagicMock()"},{"line_number":38,"context_line":"        rpc.init(CONF)"},{"line_number":39,"context_line":"        self.assertEqual(mock_get_transport.return_value, rpc.TRANSPORT)"},{"line_number":40,"context_line":"        self.assertEqual(mock_get_notification.return_value,"},{"line_number":41,"context_line":"                         rpc.NOTIFICATION_TRANSPORT)"},{"line_number":42,"context_line":"        self.assertTrue(mock_serializer.called)"},{"line_number":43,"context_line":"        self.assertEqual(mock_notifier.return_value, rpc.NOTIFIER)"},{"line_number":44,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"9abb7d3a_38a45bb8","line":41,"range":{"start_line":39,"start_character":8,"end_line":41,"end_character":52},"in_reply_to":"9abb7d3a_ba898c28","updated":"2016-06-01 14:27:25.000000000","message":"Either I don\u0027t agree or I don\u0027t understand. I want the check to be this. If I check that rpc.NOTIFICATION_TRANSPORT is not None, it will pass even if there was a bug in the code that set it to \u0027foobar\u0027.","commit_id":"3ff28077741e4f880b9d5a5d3751903ba654dbad"},{"author":{"_account_id":10697,"name":"Thiago Paiva Brito","email":"thiagop@lsd.ufcg.edu.br","username":"thiagop","inactive":true},"change_message_id":"9c0bf762c2a8c409e1c0585382ea1d7b194098b0","unresolved":false,"context_lines":[{"line_number":36,"context_line":"        mock_get_notification.return_value \u003d mock.MagicMock()"},{"line_number":37,"context_line":"        mock_notifier.return_value \u003d mock.MagicMock()"},{"line_number":38,"context_line":"        rpc.init(CONF)"},{"line_number":39,"context_line":"        self.assertEqual(mock_get_transport.return_value, rpc.TRANSPORT)"},{"line_number":40,"context_line":"        self.assertEqual(mock_get_notification.return_value,"},{"line_number":41,"context_line":"                         rpc.NOTIFICATION_TRANSPORT)"},{"line_number":42,"context_line":"        self.assertTrue(mock_serializer.called)"},{"line_number":43,"context_line":"        self.assertEqual(mock_notifier.return_value, rpc.NOTIFIER)"},{"line_number":44,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"9abb7d3a_5a44e0b5","line":41,"range":{"start_line":39,"start_character":8,"end_line":41,"end_character":52},"in_reply_to":"9abb7d3a_ba898c28","updated":"2016-05-31 20:22:11.000000000","message":"Sorry, \"assertIsNotNone\"","commit_id":"3ff28077741e4f880b9d5a5d3751903ba654dbad"},{"author":{"_account_id":12356,"name":"Vladyslav Drok","email":"vdrok@mirantis.com","username":"vdrok"},"change_message_id":"77d99f26f574f9efad0341fb5b0e026834ab3e08","unresolved":false,"context_lines":[{"line_number":29,"context_line":"    @mock.patch.object(messaging, \u0027get_transport\u0027, autospec\u003dTrue)"},{"line_number":30,"context_line":"    def test_init_globals(self, mock_get_transport, mock_get_notification,"},{"line_number":31,"context_line":"                          mock_serializer, mock_notifier):"},{"line_number":32,"context_line":"        rpc.TRANSPORT \u003d None"},{"line_number":33,"context_line":"        rpc.NOTIFICATION_TRANSPORT \u003d None"},{"line_number":34,"context_line":"        rpc.NOTIFIER \u003d None"},{"line_number":35,"context_line":"        rpc.init(CONF)"}],"source_content_type":"text/x-python","patch_set":2,"id":"9abb7d3a_cb49f632","line":32,"updated":"2016-06-01 15:49:23.000000000","message":"As unit tests are run in parallel, setting global variables may cause troubles in other tests (no unit tests that rely on these values exist right now but can be added in future). What about mocking (rpc, \u0027TRANSPORT\u0027) and other globals instead?","commit_id":"4405e6d412a089732c53b42dbd475824d8b8befc"},{"author":{"_account_id":13362,"name":"Tan Lin","email":"lin.tan@intel.com","username":"zero"},"change_message_id":"37a609bfd6f51c8017b8341d2561cc2c1ac15f8c","unresolved":false,"context_lines":[{"line_number":29,"context_line":"    @mock.patch.object(messaging, \u0027get_transport\u0027, autospec\u003dTrue)"},{"line_number":30,"context_line":"    def test_init_globals(self, mock_get_transport, mock_get_notification,"},{"line_number":31,"context_line":"                          mock_serializer, mock_notifier):"},{"line_number":32,"context_line":"        rpc.TRANSPORT \u003d None"},{"line_number":33,"context_line":"        rpc.NOTIFICATION_TRANSPORT \u003d None"},{"line_number":34,"context_line":"        rpc.NOTIFIER \u003d None"},{"line_number":35,"context_line":"        rpc.init(CONF)"}],"source_content_type":"text/x-python","patch_set":2,"id":"9abb7d3a_9a4d5bc6","line":32,"in_reply_to":"9abb7d3a_3cf561ec","updated":"2016-06-02 03:17:29.000000000","message":"Agree, it\u0027s not a good idea to setting global variable in each test case. But as there is only one for now, we can leave it. So once there are more test cases, let\u0027s move it to setup() like above examples.","commit_id":"4405e6d412a089732c53b42dbd475824d8b8befc"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"8d8cf62f404553dbd41e8e62765b88764f4d6851","unresolved":false,"context_lines":[{"line_number":29,"context_line":"    @mock.patch.object(messaging, \u0027get_transport\u0027, autospec\u003dTrue)"},{"line_number":30,"context_line":"    def test_init_globals(self, mock_get_transport, mock_get_notification,"},{"line_number":31,"context_line":"                          mock_serializer, mock_notifier):"},{"line_number":32,"context_line":"        rpc.TRANSPORT \u003d None"},{"line_number":33,"context_line":"        rpc.NOTIFICATION_TRANSPORT \u003d None"},{"line_number":34,"context_line":"        rpc.NOTIFIER \u003d None"},{"line_number":35,"context_line":"        rpc.init(CONF)"}],"source_content_type":"text/x-python","patch_set":2,"id":"9abb7d3a_64c4505f","line":32,"in_reply_to":"9abb7d3a_49016539","updated":"2016-06-02 09:47:28.000000000","message":"Here\u0027s a patch for it: https://review.openstack.org/#/c/324332/","commit_id":"4405e6d412a089732c53b42dbd475824d8b8befc"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"257b7f01004c97b8b04fac97f4bbe0b143d094b9","unresolved":false,"context_lines":[{"line_number":29,"context_line":"    @mock.patch.object(messaging, \u0027get_transport\u0027, autospec\u003dTrue)"},{"line_number":30,"context_line":"    def test_init_globals(self, mock_get_transport, mock_get_notification,"},{"line_number":31,"context_line":"                          mock_serializer, mock_notifier):"},{"line_number":32,"context_line":"        rpc.TRANSPORT \u003d None"},{"line_number":33,"context_line":"        rpc.NOTIFICATION_TRANSPORT \u003d None"},{"line_number":34,"context_line":"        rpc.NOTIFIER \u003d None"},{"line_number":35,"context_line":"        rpc.init(CONF)"}],"source_content_type":"text/x-python","patch_set":2,"id":"9abb7d3a_a7ed90f1","line":32,"in_reply_to":"9abb7d3a_64c4505f","updated":"2016-06-02 12:58:30.000000000","message":"Thanks Lucas! I was going to open a bug and putting low-hanging-fruit but you beat me to doing something about it ;)","commit_id":"4405e6d412a089732c53b42dbd475824d8b8befc"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"ba97204126d39889794dd35b1156f57df43bdca9","unresolved":false,"context_lines":[{"line_number":29,"context_line":"    @mock.patch.object(messaging, \u0027get_transport\u0027, autospec\u003dTrue)"},{"line_number":30,"context_line":"    def test_init_globals(self, mock_get_transport, mock_get_notification,"},{"line_number":31,"context_line":"                          mock_serializer, mock_notifier):"},{"line_number":32,"context_line":"        rpc.TRANSPORT \u003d None"},{"line_number":33,"context_line":"        rpc.NOTIFICATION_TRANSPORT \u003d None"},{"line_number":34,"context_line":"        rpc.NOTIFIER \u003d None"},{"line_number":35,"context_line":"        rpc.init(CONF)"}],"source_content_type":"text/x-python","patch_set":2,"id":"9abb7d3a_49016539","line":32,"in_reply_to":"9abb7d3a_9a4d5bc6","updated":"2016-06-02 09:38:22.000000000","message":"I think I\u0027m late, but +1 to what vdrok said. We should avoid overwriting the global values like that because they can affect other tests. Even setting it in the setUp() should be avoided because we could have 2 separated test classes accessing the globals. Let\u0027s always mock it.\n\nI will put a patch up fixing up these occurrences in our tests.","commit_id":"4405e6d412a089732c53b42dbd475824d8b8befc"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"7d93a6cdfcfc0d187d6664441d72fd079bab2432","unresolved":false,"context_lines":[{"line_number":29,"context_line":"    @mock.patch.object(messaging, \u0027get_transport\u0027, autospec\u003dTrue)"},{"line_number":30,"context_line":"    def test_init_globals(self, mock_get_transport, mock_get_notification,"},{"line_number":31,"context_line":"                          mock_serializer, mock_notifier):"},{"line_number":32,"context_line":"        rpc.TRANSPORT \u003d None"},{"line_number":33,"context_line":"        rpc.NOTIFICATION_TRANSPORT \u003d None"},{"line_number":34,"context_line":"        rpc.NOTIFIER \u003d None"},{"line_number":35,"context_line":"        rpc.init(CONF)"}],"source_content_type":"text/x-python","patch_set":2,"id":"9abb7d3a_3cf561ec","line":32,"in_reply_to":"9abb7d3a_cb49f632","updated":"2016-06-01 18:05:46.000000000","message":"That\u0027s an interesting point. I guess it makes sense but does that mean one should never access an actual global variable in a unit test?\n\nDoing a quick search, I see two examples where we do that [1] [2]. I guess we should decide on some approach and be consistent with it throughout. (So whatever we do, should be in a separate patch.)\n\n[1] https://github.com/openstack/ironic/blob/d1cb7180d5e89cdaa6e9f2b81fef0186947f37b1/ironic/tests/unit/common/test_keystone.py#L51\n\n[2] https://github.com/openstack/ironic/blob/0607226fc4b4bc3c9e1738dc3f78ed99e5d4f13d/ironic/tests/unit/common/test_glance_service.py#L1165","commit_id":"4405e6d412a089732c53b42dbd475824d8b8befc"}]}