)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"969bd5340ccdec1ab9f3d31185e12fede57118f9","unresolved":false,"context_lines":[{"line_number":14,"context_line":"and had not made the change to the new style."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"As the old aggregate handler had no exceptions imported, there was no"},{"line_number":17,"context_line":"merge conflict when the exceptions change merged. At the same time"},{"line_number":18,"context_line":"the exception in question is not covered by tests, because it only"},{"line_number":19,"context_line":"happens if there is a concurrent update, which we don\u0027t cover in the"},{"line_number":20,"context_line":"gabbi tests. We do cover a generation conflict on a supplied"},{"line_number":21,"context_line":"generation, but that\u0027s not the same thing."},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"So basically this went by without us noticing and I happened to notice"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"bf659307_ad4615c0","line":20,"range":{"start_line":17,"start_character":50,"end_line":20,"end_character":12},"updated":"2018-04-04 20:24:48.000000000","message":"So we don\u0027t even have a unit test of the concurrent update scenario?","commit_id":"554077070775722ffec153bb764cdad58f81e26a"}],"nova/tests/unit/api/openstack/placement/handlers/test_aggregate.py":[{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"d2c4f706b5c9f8bdd6ef5a5a3d8630274ddbccb8","unresolved":false,"context_lines":[{"line_number":23,"context_line":""},{"line_number":24,"context_line":"class TestAggregateHandlerErrors(test.NoDBTestCase):"},{"line_number":25,"context_line":"    \"\"\"Tests that makes sure errors hard to trigger by gabbi result"},{"line_number":26,"context_line":"    in except exceptions."},{"line_number":27,"context_line":"    \"\"\""},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"    def test_concurrent_exception_causes_409(self):"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf659307_71ff6534","line":26,"range":{"start_line":26,"start_character":7,"end_line":26,"end_character":13},"updated":"2018-04-05 14:22:37.000000000","message":"expected?","commit_id":"3fcaedfead0ed390c5bb38562903c61b417e6e53"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"e69fae8f049aecbcc0298e6838635b7ff0fcca8d","unresolved":false,"context_lines":[{"line_number":23,"context_line":""},{"line_number":24,"context_line":"class TestAggregateHandlerErrors(test.NoDBTestCase):"},{"line_number":25,"context_line":"    \"\"\"Tests that makes sure errors hard to trigger by gabbi result"},{"line_number":26,"context_line":"    in except exceptions."},{"line_number":27,"context_line":"    \"\"\""},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"    def test_concurrent_exception_causes_409(self):"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf659307_d4741f9f","line":26,"range":{"start_line":26,"start_character":7,"end_line":26,"end_character":13},"in_reply_to":"bf659307_71ff6534","updated":"2018-04-05 14:37:43.000000000","message":"yup thanks","commit_id":"3fcaedfead0ed390c5bb38562903c61b417e6e53"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"d2c4f706b5c9f8bdd6ef5a5a3d8630274ddbccb8","unresolved":false,"context_lines":[{"line_number":28,"context_line":""},{"line_number":29,"context_line":"    def test_concurrent_exception_causes_409(self):"},{"line_number":30,"context_line":"        rp \u003d resource_provider.ResourceProvider()"},{"line_number":31,"context_line":"        rp.set_aggregates \u003d mock.MagicMock()"},{"line_number":32,"context_line":"        rp.set_aggregates.side_effect \u003d exception.ConcurrentUpdateDetected"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"        expected_message \u003d (\u0027Update conflict: Another thread concurrently \u0027"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf659307_b1edfd8c","line":31,"range":{"start_line":31,"start_character":8,"end_line":31,"end_character":44},"updated":"2018-04-05 14:22:37.000000000","message":"This should be using a mock.patch(.object) rather than overwriting the method directly.","commit_id":"3fcaedfead0ed390c5bb38562903c61b417e6e53"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"f077719c4b2647ec23708ff997b2c6741a3529d5","unresolved":false,"context_lines":[{"line_number":28,"context_line":""},{"line_number":29,"context_line":"    def test_concurrent_exception_causes_409(self):"},{"line_number":30,"context_line":"        rp \u003d resource_provider.ResourceProvider()"},{"line_number":31,"context_line":"        rp.set_aggregates \u003d mock.MagicMock()"},{"line_number":32,"context_line":"        rp.set_aggregates.side_effect \u003d exception.ConcurrentUpdateDetected"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"        expected_message \u003d (\u0027Update conflict: Another thread concurrently \u0027"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf659307_85075319","line":31,"range":{"start_line":31,"start_character":8,"end_line":31,"end_character":44},"in_reply_to":"bf659307_14be17ff","updated":"2018-04-05 16:41:36.000000000","message":"In this (exact) situation, what you\u0027ve done is fine.  But directly replacing attributes with mocks isn\u0027t general best practice.  Slight differences in the setup could make this not work, sometimes in ways you might not notice until much later.  For example, if you had replaced ResourceProvider.set_aggregates (instead of ResourceProvider().set_aggregates), it could affect other tests who-knows-where.  If you had misspelled set_aggregates (or its name gets changed later), the statement would still work, but you won\u0027t have mocked anything.  (That\u0027s one you\u0027d catch right now, but only because the rest of the test happens to be well-designed.\n\npatch gives you advantages, including:\n\n- It fails (at \"compile\" time) if the symbol you\u0027re patching doesn\u0027t exist (obviating misspell/rename issue above).\n- It replaces the symbol just for the duration of the test, and restores its original value afterwards (meaning you *could* safely patch ResourceProvider.set_aggregates, for example).\n\nAgain, in this case it\u0027s okay, but it\u0027s just a good habit in general so you don\u0027t run afoul of those and other nasties.","commit_id":"3fcaedfead0ed390c5bb38562903c61b417e6e53"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"e69fae8f049aecbcc0298e6838635b7ff0fcca8d","unresolved":false,"context_lines":[{"line_number":28,"context_line":""},{"line_number":29,"context_line":"    def test_concurrent_exception_causes_409(self):"},{"line_number":30,"context_line":"        rp \u003d resource_provider.ResourceProvider()"},{"line_number":31,"context_line":"        rp.set_aggregates \u003d mock.MagicMock()"},{"line_number":32,"context_line":"        rp.set_aggregates.side_effect \u003d exception.ConcurrentUpdateDetected"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"        expected_message \u003d (\u0027Update conflict: Another thread concurrently \u0027"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf659307_14be17ff","line":31,"range":{"start_line":31,"start_character":8,"end_line":31,"end_character":44},"in_reply_to":"bf659307_b1edfd8c","updated":"2018-04-05 14:37:43.000000000","message":"I took it right from the (very start of the) docs: https://docs.python.org/3/library/unittest.mock.html#quick-guide\n\nIt was the cleanest and least ugly intrusive mock thing to do in this situation.","commit_id":"3fcaedfead0ed390c5bb38562903c61b417e6e53"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"d2c4f706b5c9f8bdd6ef5a5a3d8630274ddbccb8","unresolved":false,"context_lines":[{"line_number":36,"context_line":""},{"line_number":37,"context_line":"        exc \u003d self.assertRaises("},{"line_number":38,"context_line":"            webob.exc.HTTPConflict, aggregate._set_aggregates,"},{"line_number":39,"context_line":"            rp, mock.ANY)"},{"line_number":40,"context_line":"        self.assertIn(expected_message, str(exc))"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf659307_713a0535","line":39,"range":{"start_line":39,"start_character":16,"end_line":39,"end_character":24},"updated":"2018-04-05 14:22:37.000000000","message":"This is a weird use of mock.ANY.","commit_id":"3fcaedfead0ed390c5bb38562903c61b417e6e53"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"f077719c4b2647ec23708ff997b2c6741a3529d5","unresolved":false,"context_lines":[{"line_number":36,"context_line":""},{"line_number":37,"context_line":"        exc \u003d self.assertRaises("},{"line_number":38,"context_line":"            webob.exc.HTTPConflict, aggregate._set_aggregates,"},{"line_number":39,"context_line":"            rp, mock.ANY)"},{"line_number":40,"context_line":"        self.assertIn(expected_message, str(exc))"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf659307_c54b0b5d","line":39,"range":{"start_line":39,"start_character":16,"end_line":39,"end_character":24},"in_reply_to":"bf659307_34bb1b0f","updated":"2018-04-05 16:41:36.000000000","message":"I wasn\u0027t necessarily looking for a change here; just noting that I\u0027ve never seen anyone use mock.ANY as an input argument before.  In many circumstances it would break the thing you\u0027re calling; it just happens to work here because nothing accesses it between here and your mock.\n\nmock.ANY is intended to be used when asserting call args, when you don\u0027t want or care to check what was passed for a particular thing.  (Often it\u0027s used when that arg would be something really complex or hard to make a comparator for.)\n\nIn this case, yes, you can pass an empty list, or even a string like \"don\u0027t care\" if you like.  And as a bonus, you could then assert that your mock was called_once_with that value.","commit_id":"3fcaedfead0ed390c5bb38562903c61b417e6e53"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"e69fae8f049aecbcc0298e6838635b7ff0fcca8d","unresolved":false,"context_lines":[{"line_number":36,"context_line":""},{"line_number":37,"context_line":"        exc \u003d self.assertRaises("},{"line_number":38,"context_line":"            webob.exc.HTTPConflict, aggregate._set_aggregates,"},{"line_number":39,"context_line":"            rp, mock.ANY)"},{"line_number":40,"context_line":"        self.assertIn(expected_message, str(exc))"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf659307_34bb1b0f","line":39,"range":{"start_line":39,"start_character":16,"end_line":39,"end_character":24},"in_reply_to":"bf659307_713a0535","updated":"2018-04-05 14:37:43.000000000","message":"I could put an empty list there, but I chose this to mean \"we don\u0027t care what\u0027s here\".\n\nWhat\u0027s your preference? I\u0027m not wise in the ways of mocking","commit_id":"3fcaedfead0ed390c5bb38562903c61b417e6e53"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"108135166b50909a80c49b337309705d6121428b","unresolved":false,"context_lines":[{"line_number":21,"context_line":""},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"class TestAggregateHandlerErrors(test.NoDBTestCase):"},{"line_number":24,"context_line":"    \"\"\"Tests that make sure errors hard to trigger by gabbi result in excpected"},{"line_number":25,"context_line":"    exceptions."},{"line_number":26,"context_line":"    \"\"\""},{"line_number":27,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"bf659307_20ec35f2","line":24,"range":{"start_line":24,"start_character":70,"end_line":24,"end_character":79},"updated":"2018-04-05 17:53:29.000000000","message":"expected","commit_id":"ea48b5adacfd3d0780207388f9757b9d14447f03"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"1fb756765a941f6df9ce85f52e76b7b0f9609f7a","unresolved":false,"context_lines":[{"line_number":21,"context_line":""},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"class TestAggregateHandlerErrors(test.NoDBTestCase):"},{"line_number":24,"context_line":"    \"\"\"Tests that make sure errors hard to trigger by gabbi result in excpected"},{"line_number":25,"context_line":"    exceptions."},{"line_number":26,"context_line":"    \"\"\""},{"line_number":27,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"bf659307_a0b925ea","line":24,"range":{"start_line":24,"start_character":70,"end_line":24,"end_character":79},"updated":"2018-04-05 17:52:31.000000000","message":"expected","commit_id":"ea48b5adacfd3d0780207388f9757b9d14447f03"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"108135166b50909a80c49b337309705d6121428b","unresolved":false,"context_lines":[{"line_number":30,"context_line":"        expected_message \u003d (\u0027Update conflict: Another thread concurrently \u0027"},{"line_number":31,"context_line":"                            \u0027updated the data\u0027)"},{"line_number":32,"context_line":"        with mock.patch.object(rp, \"set_aggregates\","},{"line_number":33,"context_line":"                               side_effect\u003dexception.ConcurrentUpdateDetected):"},{"line_number":34,"context_line":"            exc \u003d self.assertRaises(webob.exc.HTTPConflict,"},{"line_number":35,"context_line":"                                    aggregate._set_aggregates, rp, [])"},{"line_number":36,"context_line":"        self.assertIn(expected_message, str(exc))"}],"source_content_type":"text/x-python","patch_set":3,"id":"bf659307_80b421f4","line":33,"range":{"start_line":33,"start_character":53,"end_line":33,"end_character":77},"updated":"2018-04-05 17:53:29.000000000","message":"This exception message doesn\u0027t have any kwargs so we\u0027re OK here.","commit_id":"ea48b5adacfd3d0780207388f9757b9d14447f03"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"108135166b50909a80c49b337309705d6121428b","unresolved":false,"context_lines":[{"line_number":33,"context_line":"                               side_effect\u003dexception.ConcurrentUpdateDetected):"},{"line_number":34,"context_line":"            exc \u003d self.assertRaises(webob.exc.HTTPConflict,"},{"line_number":35,"context_line":"                                    aggregate._set_aggregates, rp, [])"},{"line_number":36,"context_line":"        self.assertIn(expected_message, str(exc))"}],"source_content_type":"text/x-python","patch_set":3,"id":"bf659307_20c5155c","line":36,"range":{"start_line":36,"start_character":40,"end_line":36,"end_character":43},"updated":"2018-04-05 17:53:29.000000000","message":"Use six.text_type, str() casting can get funky if there is ever unicode involved.","commit_id":"ea48b5adacfd3d0780207388f9757b9d14447f03"}]}