)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"f5595d35ebb751dd6dff25120e13b11f1ceae6c9","unresolved":false,"context_lines":[{"line_number":14,"context_line":"However in order to actually remove the router we also need admin"},{"line_number":15,"context_line":"context."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"This was not caught by unit tests and one reason for that is so called"},{"line_number":18,"context_line":"\u0027mock everything\u0027 approach which is evil and generally useless."},{"line_number":19,"context_line":"This patch replaces unit tests with functional tests that we able"},{"line_number":20,"context_line":"to catch the bug."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":9,"id":"da6ed579_8b792432","line":17,"updated":"2016-01-12 05:00:17.000000000","message":"This was, no doubt, a test escape.  But, on the other hand, we have different levels of testing because we need multiple levels.  Unit tests are not meant to catch everything.\n\nSure, use mocks sparingly, but they are still a very useful tool if used wisely.  I just hesitate to make such broad statements like \"evil\" and \"generally useless\".  Every test should be validating some important behavior of the system.  It isn\u0027t required to validate every important behavior of the system.  In fact, I think tests that try to validate too much at once can be difficult to understand, write, and maintain.  The fact that testing missed an important behavior isn\u0027t proof to me that the whole strategy is flawed.","commit_id":"d82ab1eeb1c740af00c4f3d8ad5decb056de9361"},{"author":{"_account_id":5948,"name":"Oleg Bondarev","email":"obondarev@mirantis.com","username":"obondarev"},"change_message_id":"c1d4d7177e6b6f274a312feec6eb35a550dcc68d","unresolved":false,"context_lines":[{"line_number":14,"context_line":"However in order to actually remove the router we also need admin"},{"line_number":15,"context_line":"context."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"This was not caught by unit tests and one reason for that is so called"},{"line_number":18,"context_line":"\u0027mock everything\u0027 approach which is evil and generally useless."},{"line_number":19,"context_line":"This patch replaces unit tests with functional tests that we able"},{"line_number":20,"context_line":"to catch the bug."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":9,"id":"da6ed579_3d871963","line":17,"in_reply_to":"da6ed579_8b792432","updated":"2016-01-12 08:38:07.000000000","message":"I completely agree that mocks are very useful, but they should be used with care. If we mock each call that method does and then check that mocks were called - such tests are useless IMO. Please see tests that were removed in this patch - I was trying to update them to actually catch the bug till I understood that they don\u0027t bring any value, are hard to understand and maintain. There was a unit test for this exact case: test_dvr_deletens_if_no_ports_delete_admin_routers which was supposed to catch the regression but it didn\u0027t - that\u0027s why I call it evil.","commit_id":"d82ab1eeb1c740af00c4f3d8ad5decb056de9361"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"f5595d35ebb751dd6dff25120e13b11f1ceae6c9","unresolved":false,"context_lines":[{"line_number":16,"context_line":""},{"line_number":17,"context_line":"This was not caught by unit tests and one reason for that is so called"},{"line_number":18,"context_line":"\u0027mock everything\u0027 approach which is evil and generally useless."},{"line_number":19,"context_line":"This patch replaces unit tests with functional tests that we able"},{"line_number":20,"context_line":"to catch the bug."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"Closes-Bug: #1424096"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":9,"id":"da6ed579_8bc7e444","line":19,"range":{"start_line":19,"start_character":11,"end_line":19,"end_character":19},"updated":"2016-01-12 05:00:17.000000000","message":"We don\u0027t need to move everything to functional tests because unit tests miss something.  We have different levels of testing for doing different kinds of tests.  Moving everything to functional tests comes at a price.  Why not test what we can at the unit level *and* test what we can\u0027t at the functional level and not argue about which level of testing is \"better\"?  I\u0027ll continue to distrust any code that is only tested at one level, whatever that level is.","commit_id":"d82ab1eeb1c740af00c4f3d8ad5decb056de9361"},{"author":{"_account_id":7787,"name":"Kevin Benton","email":"kevin@benton.pub","username":"blak111"},"change_message_id":"5f6076e180edd178e8aa58639e836747fe0c7d67","unresolved":false,"context_lines":[{"line_number":16,"context_line":""},{"line_number":17,"context_line":"This was not caught by unit tests and one reason for that is so called"},{"line_number":18,"context_line":"\u0027mock everything\u0027 approach which is evil and generally useless."},{"line_number":19,"context_line":"This patch replaces unit tests with functional tests that we able"},{"line_number":20,"context_line":"to catch the bug."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"Closes-Bug: #1424096"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":9,"id":"da6ed579_c6c953ae","line":19,"updated":"2016-01-12 05:11:12.000000000","message":"Well it\u0027s not that they were unit tests that was the issue. It was that that the unit tests were written in such a way that they were basically not testing anything.","commit_id":"d82ab1eeb1c740af00c4f3d8ad5decb056de9361"},{"author":{"_account_id":5948,"name":"Oleg Bondarev","email":"obondarev@mirantis.com","username":"obondarev"},"change_message_id":"c1d4d7177e6b6f274a312feec6eb35a550dcc68d","unresolved":false,"context_lines":[{"line_number":16,"context_line":""},{"line_number":17,"context_line":"This was not caught by unit tests and one reason for that is so called"},{"line_number":18,"context_line":"\u0027mock everything\u0027 approach which is evil and generally useless."},{"line_number":19,"context_line":"This patch replaces unit tests with functional tests that we able"},{"line_number":20,"context_line":"to catch the bug."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"Closes-Bug: #1424096"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":9,"id":"da6ed579_9d7725d4","line":19,"range":{"start_line":19,"start_character":11,"end_line":19,"end_character":19},"in_reply_to":"da6ed579_8bc7e444","updated":"2016-01-12 08:38:07.000000000","message":"Sorry for the confusion, I didn\u0027t want to say that unit tests are bad, I understand the need of testing at all levels. My point is that for this rather complex case we actually need a functional test. Unit tests were removed for the reasons stated in my previous comment.","commit_id":"d82ab1eeb1c740af00c4f3d8ad5decb056de9361"}],"neutron/tests/functional/services/l3_router/test_l3_dvr_router_plugin.py":[{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"596c5dfd2b6571bad7134991d8f5ffc55455da66","unresolved":false,"context_lines":[{"line_number":527,"context_line":"                          set_context\u003dnon_admin_port) as port:"},{"line_number":528,"context_line":"            self.core_plugin.update_port("},{"line_number":529,"context_line":"                    self.context, port[\u0027port\u0027][\u0027id\u0027],"},{"line_number":530,"context_line":"                    {\u0027port\u0027: {\u0027binding:host_id\u0027: HOST}})"},{"line_number":531,"context_line":"            self.l3_plugin.add_router_interface("},{"line_number":532,"context_line":"                self.context, router[\u0027id\u0027],"},{"line_number":533,"context_line":"                {\u0027subnet_id\u0027: subnet[\u0027subnet\u0027][\u0027id\u0027]})"}],"source_content_type":"text/x-python","patch_set":4,"id":"5a710552_5ed87def","line":530,"updated":"2015-12-16 09:02:39.000000000","message":"please use the define here. please see https://review.openstack.org/257036","commit_id":"2abc044843b1c8d53bf9e5e9ffc6b186a7dd7885"},{"author":{"_account_id":5948,"name":"Oleg Bondarev","email":"obondarev@mirantis.com","username":"obondarev"},"change_message_id":"efd853fc4fc67e5ea5504fa30b5db4588797d315","unresolved":false,"context_lines":[{"line_number":527,"context_line":"                          set_context\u003dnon_admin_port) as port:"},{"line_number":528,"context_line":"            self.core_plugin.update_port("},{"line_number":529,"context_line":"                    self.context, port[\u0027port\u0027][\u0027id\u0027],"},{"line_number":530,"context_line":"                    {\u0027port\u0027: {\u0027binding:host_id\u0027: HOST}})"},{"line_number":531,"context_line":"            self.l3_plugin.add_router_interface("},{"line_number":532,"context_line":"                self.context, router[\u0027id\u0027],"},{"line_number":533,"context_line":"                {\u0027subnet_id\u0027: subnet[\u0027subnet\u0027][\u0027id\u0027]})"}],"source_content_type":"text/x-python","patch_set":4,"id":"5a710552_f2195d7a","line":530,"in_reply_to":"5a710552_5ed87def","updated":"2015-12-16 09:08:03.000000000","message":"blocker? :)","commit_id":"2abc044843b1c8d53bf9e5e9ffc6b186a7dd7885"}]}