)]}'
{"cinder/db/sqlalchemy/api.py":[{"author":{"_account_id":1736,"name":"Ivan Kolodyazhny","email":"e0ne@e0ne.info","username":"e0ne"},"change_message_id":"9362b7d7863a7ebceaf8cf41da27ac99326158e1","unresolved":false,"context_lines":[{"line_number":3585,"context_line":""},{"line_number":3586,"context_line":"    if filters:"},{"line_number":3587,"context_line":"        query \u003d query.filter_by(**filters)"},{"line_number":3588,"context_line":""},{"line_number":3589,"context_line":"    return query.filter_by(project_id\u003dproject_id).all()"},{"line_number":3590,"context_line":""},{"line_number":3591,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"ba3cc151_d0a52ca2","line":3588,"updated":"2015-07-01 10:50:19.000000000","message":"I\u0027m confused a bit. Why do you check for valid filter keys only in cgsnapshot_get_all method?","commit_id":"511cf311c9144839ec7688cf4ee6271916f41ee8"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"1a69e2abe0e84cda67a360fda3f6a6fa2f72cffd","unresolved":false,"context_lines":[{"line_number":3559,"context_line":"@require_admin_context"},{"line_number":3560,"context_line":"def cgsnapshot_get_all(context, filters\u003dNone):"},{"line_number":3561,"context_line":"    query \u003d model_query(context, models.Cgsnapshot)"},{"line_number":3562,"context_line":"    # Ensure that the filter value exists on the model"},{"line_number":3563,"context_line":"    if filters:"},{"line_number":3564,"context_line":"        for key in filters.keys():"},{"line_number":3565,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba3cc151_02c33139","line":3562,"updated":"2015-07-01 18:55:52.000000000","message":"s/value/values\ns/exists/exist","commit_id":"6bd9f54f87b55666f3b0ffbbe72018bd67e89ded"},{"author":{"_account_id":1736,"name":"Ivan Kolodyazhny","email":"e0ne@e0ne.info","username":"e0ne"},"change_message_id":"3317f772b03f202a47dd02a7f5fbd296e71cb430","unresolved":false,"context_lines":[{"line_number":3592,"context_line":"                LOG.debug(\"\u0027%s\u0027 filter key is not valid.\", key)"},{"line_number":3593,"context_line":"                return []"},{"line_number":3594,"context_line":"        query \u003d query.filter_by(**filters)"},{"line_number":3595,"context_line":""},{"line_number":3596,"context_line":"    return query.filter_by(project_id\u003dproject_id).all()"},{"line_number":3597,"context_line":""},{"line_number":3598,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"ba3cc151_6ee09c26","line":3595,"updated":"2015-07-09 15:20:30.000000000","message":"Lines #3585-3594 are the same as #3562-3571. Please, remove duplicated code","commit_id":"ca904e980648a95c718f11b3bf3fb431f9339150"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"35b8162e4f59818745aef0216e5931b6d0d94590","unresolved":false,"context_lines":[{"line_number":3589,"context_line":"@require_context"},{"line_number":3590,"context_line":"def cgsnapshot_get_all_by_project(context, project_id, filters\u003dNone):"},{"line_number":3591,"context_line":"    authorize_project_context(context, project_id)"},{"line_number":3592,"context_line":"    query \u003d model_query(context, models.Cgsnapshot)"},{"line_number":3593,"context_line":""},{"line_number":3594,"context_line":"    if filters:"},{"line_number":3595,"context_line":"        if not is_valid_filter_keys(filters):"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_303c2992","line":3592,"updated":"2015-07-09 16:58:43.000000000","message":"If you are going to get rid of redundant code I think you should take it a little bit farther.\n\nCrate a common method that accepts context, filters and project_id (all except context with default to None) and if it is not None adds it to the filters dictionary.\n\nThen these 2 methods can just do:\n\n return cgsnapshot_get_all_common(context, filters, project_id)\n\nAnd the other one will do:\n\n return cgsnapshot_get_all_common(context, filters)","commit_id":"4f1d25d25b3adf73cabe86b9688dacb2fddd9516"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"5d79746c4f38c8bad2c8848fc9928f3d417975bd","unresolved":false,"context_lines":[{"line_number":3568,"context_line":"    return True"},{"line_number":3569,"context_line":""},{"line_number":3570,"context_line":""},{"line_number":3571,"context_line":"def _cgsnapshot_get_all(context, project_id\u003dNone, group_id\u003dNone, filters\u003dNone):"},{"line_number":3572,"context_line":"    query \u003d model_query(context, models.Cgsnapshot)"},{"line_number":3573,"context_line":""},{"line_number":3574,"context_line":"    if filters:"}],"source_content_type":"text/x-python","patch_set":8,"id":"ba3cc151_472eaefb","line":3571,"updated":"2015-07-10 11:53:39.000000000","message":"We could use **kwargs instead of project_id and group_id and then update filters inside, but I prefer your solution for being explicit about the arguments.","commit_id":"56f5e9e1be9e00f81c3126f2997140e1c9345589"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"5d79746c4f38c8bad2c8848fc9928f3d417975bd","unresolved":false,"context_lines":[{"line_number":3591,"context_line":""},{"line_number":3592,"context_line":""},{"line_number":3593,"context_line":"@require_admin_context"},{"line_number":3594,"context_line":"def cgsnapshot_get_all_by_group(context, group_id):"},{"line_number":3595,"context_line":"    return _cgsnapshot_get_all(context, group_id\u003dgroup_id)"},{"line_number":3596,"context_line":""},{"line_number":3597,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"ba3cc151_67e5f236","line":3594,"updated":"2015-07-10 11:53:39.000000000","message":"Sorry for realizing this so late, but I think you should include filters here as well","commit_id":"56f5e9e1be9e00f81c3126f2997140e1c9345589"},{"author":{"_account_id":9171,"name":"Vipin Balachandran","email":"vipin.bl@gmail.com","username":"vbala"},"change_message_id":"a4edc18d70d463ca7de27bfa3d33fbb22fd7e6e9","unresolved":false,"context_lines":[{"line_number":3568,"context_line":"    for key in filters.keys():"},{"line_number":3569,"context_line":"        try:"},{"line_number":3570,"context_line":"            getattr(model, key)"},{"line_number":3571,"context_line":"        except AttributeError:"},{"line_number":3572,"context_line":"            LOG.debug(\"\u0027%s\u0027 filter key is not valid.\", key)"},{"line_number":3573,"context_line":"            return False"},{"line_number":3574,"context_line":"    return True"}],"source_content_type":"text/x-python","patch_set":12,"id":"9a41bdd9_3a17ec52","line":3571,"updated":"2015-07-13 03:48:19.000000000","message":"It is better to use hasattr(model, key) instead of exception handling.","commit_id":"10a4047d20a26c5c09ba7d80b52db80efd54f41c"},{"author":{"_account_id":1736,"name":"Ivan Kolodyazhny","email":"e0ne@e0ne.info","username":"e0ne"},"change_message_id":"5a3d8a3ab097e2ffb720426f3f92cf891c5f1a93","unresolved":false,"context_lines":[{"line_number":3568,"context_line":"    for key in filters.keys():"},{"line_number":3569,"context_line":"        try:"},{"line_number":3570,"context_line":"            getattr(model, key)"},{"line_number":3571,"context_line":"        except AttributeError:"},{"line_number":3572,"context_line":"            LOG.debug(\"\u0027%s\u0027 filter key is not valid.\", key)"},{"line_number":3573,"context_line":"            return False"},{"line_number":3574,"context_line":"    return True"}],"source_content_type":"text/x-python","patch_set":12,"id":"9a41bdd9_6dda9b7a","line":3571,"in_reply_to":"9a41bdd9_3a17ec52","updated":"2015-07-17 10:23:34.000000000","message":"Vipin, all proposed solutions are good, but try-except will be faster in Python","commit_id":"10a4047d20a26c5c09ba7d80b52db80efd54f41c"}],"cinder/tests/unit/test_db_api.py":[{"author":{"_account_id":1736,"name":"Ivan Kolodyazhny","email":"e0ne@e0ne.info","username":"e0ne"},"change_message_id":"9362b7d7863a7ebceaf8cf41da27ac99326158e1","unresolved":false,"context_lines":[{"line_number":1104,"context_line":"                                           \u0027consistencygroup_id\u0027: \u0027g1\u0027})"},{"line_number":1105,"context_line":"        cgsnapshot3 \u003d db.cgsnapshot_create(self.ctxt, {\u0027id\u0027: 3,"},{"line_number":1106,"context_line":"                                           \u0027consistencygroup_id\u0027: \u0027g2\u0027})"},{"line_number":1107,"context_line":"        # no filter"},{"line_number":1108,"context_line":"        filters \u003d {}"},{"line_number":1109,"context_line":"        cgsnapshots \u003d db.cgsnapshot_get_all(self.ctxt, filters\u003dfilters)"},{"line_number":1110,"context_line":"        self.assertEqual(3, len(cgsnapshots))"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba3cc151_708f802a","line":1107,"updated":"2015-07-01 10:50:19.000000000","message":"Actually, it is test with empty filters. You should test with filters\u003dNone too.","commit_id":"511cf311c9144839ec7688cf4ee6271916f41ee8"},{"author":{"_account_id":1736,"name":"Ivan Kolodyazhny","email":"e0ne@e0ne.info","username":"e0ne"},"change_message_id":"9362b7d7863a7ebceaf8cf41da27ac99326158e1","unresolved":false,"context_lines":[{"line_number":1124,"context_line":"                                        db.cgsnapshot_get_all("},{"line_number":1125,"context_line":"                                            self.ctxt,"},{"line_number":1126,"context_line":"                                            filters))"},{"line_number":1127,"context_line":""},{"line_number":1128,"context_line":""},{"line_number":1129,"context_line":"class DBAPIVolumeTypeTestCase(BaseTest):"},{"line_number":1130,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"ba3cc151_50a35cb2","line":1127,"updated":"2015-07-01 10:50:19.000000000","message":"Please, add test for db.cgsnapshot_get_all_by_project method","commit_id":"511cf311c9144839ec7688cf4ee6271916f41ee8"},{"author":{"_account_id":9003,"name":"Tom Barron","email":"tpb@dyncloud.net","username":"tbarron"},"change_message_id":"c4719f34196cd14dc031c792acb41c930ad5e0ed","unresolved":false,"context_lines":[{"line_number":1125,"context_line":"                                            self.ctxt,"},{"line_number":1126,"context_line":"                                            filters))"},{"line_number":1127,"context_line":""},{"line_number":1128,"context_line":"    def test_cgsnapshot_get_all_by_project(self):"},{"line_number":1129,"context_line":"        cgsnapshot1 \u003d db.cgsnapshot_create(self.ctxt,"},{"line_number":1130,"context_line":"                                           {\u0027id\u0027: 1,"},{"line_number":1131,"context_line":"                                            \u0027consistencygroup_id\u0027: \u0027g1\u0027,"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba3cc151_107709d4","line":1128,"updated":"2015-07-01 14:12:18.000000000","message":"I think you can use the ddt module to pass the filters into each of these tests and as a result get (1) one test-case per filter, (2) no code duplication.\n\nSee e.g.\n\ncinder/tests/unit/test_dellscapi.py\n\nand\n\ncinder/tests/unit/volume/drivers/netapp/eseries/test_library.py","commit_id":"6bd9f54f87b55666f3b0ffbbe72018bd67e89ded"},{"author":{"_account_id":14305,"name":"Yuriy Nesenenko","email":"ynesenenko@mirantis.com","username":"yuriy_n"},"change_message_id":"65f4c3c37be7f2980709f23e44ce65ca6ddd5438","unresolved":false,"context_lines":[{"line_number":1125,"context_line":"                                            self.ctxt,"},{"line_number":1126,"context_line":"                                            filters))"},{"line_number":1127,"context_line":""},{"line_number":1128,"context_line":"    def test_cgsnapshot_get_all_by_project(self):"},{"line_number":1129,"context_line":"        cgsnapshot1 \u003d db.cgsnapshot_create(self.ctxt,"},{"line_number":1130,"context_line":"                                           {\u0027id\u0027: 1,"},{"line_number":1131,"context_line":"                                            \u0027consistencygroup_id\u0027: \u0027g1\u0027,"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba3cc151_c17555fa","line":1128,"in_reply_to":"ba3cc151_107709d4","updated":"2015-07-01 14:55:53.000000000","message":"The ddt module is used only in cinder/tests/unit/test_dellscapi.py and cinder/tests/unit/volume/drivers/netapp/eseries/test_library.py. So I am not sure that it\u0027s a good idea to mix the style applied in this case (here we use  self._assertEqualListsOfObjects(...)) with that.","commit_id":"6bd9f54f87b55666f3b0ffbbe72018bd67e89ded"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"1a69e2abe0e84cda67a360fda3f6a6fa2f72cffd","unresolved":false,"context_lines":[{"line_number":1125,"context_line":"                                            self.ctxt,"},{"line_number":1126,"context_line":"                                            filters))"},{"line_number":1127,"context_line":""},{"line_number":1128,"context_line":"    def test_cgsnapshot_get_all_by_project(self):"},{"line_number":1129,"context_line":"        cgsnapshot1 \u003d db.cgsnapshot_create(self.ctxt,"},{"line_number":1130,"context_line":"                                           {\u0027id\u0027: 1,"},{"line_number":1131,"context_line":"                                            \u0027consistencygroup_id\u0027: \u0027g1\u0027,"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba3cc151_a2a845dc","line":1128,"in_reply_to":"ba3cc151_3cf1c601","updated":"2015-07-01 18:55:52.000000000","message":"Any module in requirements or test_requirements can be used in tests, so I agree that should not be a limitation.  But maybe using ddt would make this code harder to read for people not familiar with ddt module.\n\nBut we could always use a list of tuples with (filter, expected) and loop through it:\n\n tests \u003d [\n     ({\u0027project_id\u0027: 2}, [cgsnapshot2]),\n     ...\n ]\n\n for filters, expected in tests:\n     self._assertEqualListsOfObjects(expected,\n                                     db.cgsnapshot_get_all(\n                                            self.ctxt,\n                                            filters))\n\nAnyway, I don\u0027t think such nit deserves a -1... ;-)","commit_id":"6bd9f54f87b55666f3b0ffbbe72018bd67e89ded"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"91ccbba3876d065c6be84e098c0e2d350667d661","unresolved":false,"context_lines":[{"line_number":1125,"context_line":"                                            self.ctxt,"},{"line_number":1126,"context_line":"                                            filters))"},{"line_number":1127,"context_line":""},{"line_number":1128,"context_line":"    def test_cgsnapshot_get_all_by_project(self):"},{"line_number":1129,"context_line":"        cgsnapshot1 \u003d db.cgsnapshot_create(self.ctxt,"},{"line_number":1130,"context_line":"                                           {\u0027id\u0027: 1,"},{"line_number":1131,"context_line":"                                            \u0027consistencygroup_id\u0027: \u0027g1\u0027,"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba3cc151_e2a6e9bf","line":1128,"in_reply_to":"ba3cc151_6ce5f206","updated":"2015-07-03 15:52:11.000000000","message":"I disagree with your opinion of the -1.  As it says in the description, it means \"the patch needs further work before merging\", in general there\u0027s something you actually want changed before this is merged or there is something fishy that needs discussion or is cause for concern.\n\nWhile it is not a grade per se, it is a statement of your opinion of the patch status, and I don\u0027t mean its quality.\n\nIn general people reply to comments regardless of the -1, just because it is the right thing to do after someone has taken the time to review your code and make, like in your case, good suggestions for improvement.\n\nThe fact that you have now changed your vote to +1 means that you were not against merging it in the first place.  Therefore the -1 was not \"deserved\".","commit_id":"6bd9f54f87b55666f3b0ffbbe72018bd67e89ded"},{"author":{"_account_id":9003,"name":"Tom Barron","email":"tpb@dyncloud.net","username":"tbarron"},"change_message_id":"3ac92c343f1da3456a8586aaa2bb29b6d6781a7c","unresolved":false,"context_lines":[{"line_number":1125,"context_line":"                                            self.ctxt,"},{"line_number":1126,"context_line":"                                            filters))"},{"line_number":1127,"context_line":""},{"line_number":1128,"context_line":"    def test_cgsnapshot_get_all_by_project(self):"},{"line_number":1129,"context_line":"        cgsnapshot1 \u003d db.cgsnapshot_create(self.ctxt,"},{"line_number":1130,"context_line":"                                           {\u0027id\u0027: 1,"},{"line_number":1131,"context_line":"                                            \u0027consistencygroup_id\u0027: \u0027g1\u0027,"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba3cc151_6ce5f206","line":1128,"in_reply_to":"ba3cc151_a2a845dc","updated":"2015-07-03 15:00:22.000000000","message":"The -1 is not a matter of what is \"deserved\", nor is it a \"grade.\"\n\nIt means that there is an issue to be addressed pre-merge and is a way to make sure that it is seen and discussed at least.\n\nThat has now happened, and I am changing the vote to +1\nsince, as I indicated earlier, the overall change seems fine.\n\nFWIW I think the tests cited that use ddt are quite easy\nto read - indeed easier since you don\u0027t have to read carefully\nto see that they are identical except for the values in\nquestion.\n\nHere is another good example of cinder tests using ddt:\n\nhttps://review.openstack.org/#/c/195695/5/cinder/tests/unit/test_utils.py","commit_id":"6bd9f54f87b55666f3b0ffbbe72018bd67e89ded"},{"author":{"_account_id":11904,"name":"Sean McGinnis","email":"sean.mcginnis@gmail.com","username":"SeanM"},"change_message_id":"d017db6a653b67f4e3fc2f80b34008be81c938b0","unresolved":false,"context_lines":[{"line_number":1125,"context_line":"                                            self.ctxt,"},{"line_number":1126,"context_line":"                                            filters))"},{"line_number":1127,"context_line":""},{"line_number":1128,"context_line":"    def test_cgsnapshot_get_all_by_project(self):"},{"line_number":1129,"context_line":"        cgsnapshot1 \u003d db.cgsnapshot_create(self.ctxt,"},{"line_number":1130,"context_line":"                                           {\u0027id\u0027: 1,"},{"line_number":1131,"context_line":"                                            \u0027consistencygroup_id\u0027: \u0027g1\u0027,"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba3cc151_3cf1c601","line":1128,"in_reply_to":"ba3cc151_c17555fa","updated":"2015-07-01 15:00:58.000000000","message":"You\u0027re repeating the same thing repeatedly with just different input filters and expected output. This seems perfect for ddt.\n\nI don\u0027t really see how where it is used elsewhere in the existing code is relevant.","commit_id":"6bd9f54f87b55666f3b0ffbbe72018bd67e89ded"},{"author":{"_account_id":9003,"name":"Tom Barron","email":"tpb@dyncloud.net","username":"tbarron"},"change_message_id":"db6127bcc9e2a2ff21f4a9dc87a87087f4e3a59c","unresolved":false,"context_lines":[{"line_number":1125,"context_line":"                                            self.ctxt,"},{"line_number":1126,"context_line":"                                            filters))"},{"line_number":1127,"context_line":""},{"line_number":1128,"context_line":"    def test_cgsnapshot_get_all_by_project(self):"},{"line_number":1129,"context_line":"        cgsnapshot1 \u003d db.cgsnapshot_create(self.ctxt,"},{"line_number":1130,"context_line":"                                           {\u0027id\u0027: 1,"},{"line_number":1131,"context_line":"                                            \u0027consistencygroup_id\u0027: \u0027g1\u0027,"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba3cc151_63ae5ce9","line":1128,"in_reply_to":"ba3cc151_e2a6e9bf","updated":"2015-07-03 16:35:14.000000000","message":"@Gorka: You make a fair point.","commit_id":"6bd9f54f87b55666f3b0ffbbe72018bd67e89ded"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"1a69e2abe0e84cda67a360fda3f6a6fa2f72cffd","unresolved":false,"context_lines":[{"line_number":1148,"context_line":"        self._assertEqualListsOfObjects([cgsnapshot1],"},{"line_number":1149,"context_line":"                                        db.cgsnapshot_get_all("},{"line_number":1150,"context_line":"                                            self.ctxt,"},{"line_number":1151,"context_line":"                                            filters))"},{"line_number":1152,"context_line":""},{"line_number":1153,"context_line":""},{"line_number":1154,"context_line":"class DBAPIVolumeTypeTestCase(BaseTest):"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba3cc151_c2bea964","line":1151,"updated":"2015-07-01 18:55:52.000000000","message":"Why don\u0027t you tests fake_key here as well?","commit_id":"6bd9f54f87b55666f3b0ffbbe72018bd67e89ded"},{"author":{"_account_id":9003,"name":"Tom Barron","email":"tpb@dyncloud.net","username":"tbarron"},"change_message_id":"15d75e22787aeeb670326d74623939b25302b72a","unresolved":false,"context_lines":[{"line_number":1120,"context_line":"                                                self.ctxt,"},{"line_number":1121,"context_line":"                                                filters))"},{"line_number":1122,"context_line":""},{"line_number":1123,"context_line":"    def test_cgsnapshot_get_all_by_project(self):"},{"line_number":1124,"context_line":"        cgsnapshot1 \u003d db.cgsnapshot_create(self.ctxt,"},{"line_number":1125,"context_line":"                                           {\u0027id\u0027: 1,"},{"line_number":1126,"context_line":"                                            \u0027consistencygroup_id\u0027: \u0027g1\u0027,"}],"source_content_type":"text/x-python","patch_set":5,"id":"ba3cc151_65ec0090","line":1123,"updated":"2015-07-06 11:27:06.000000000","message":"Using ddt would be better here than the for loop because\nthat would yield one critiical assert per test and make dealing\nwith failed tests easier.","commit_id":"ca904e980648a95c718f11b3bf3fb431f9339150"},{"author":{"_account_id":1736,"name":"Ivan Kolodyazhny","email":"e0ne@e0ne.info","username":"e0ne"},"change_message_id":"3317f772b03f202a47dd02a7f5fbd296e71cb430","unresolved":false,"context_lines":[{"line_number":1120,"context_line":"                                                self.ctxt,"},{"line_number":1121,"context_line":"                                                filters))"},{"line_number":1122,"context_line":""},{"line_number":1123,"context_line":"    def test_cgsnapshot_get_all_by_project(self):"},{"line_number":1124,"context_line":"        cgsnapshot1 \u003d db.cgsnapshot_create(self.ctxt,"},{"line_number":1125,"context_line":"                                           {\u0027id\u0027: 1,"},{"line_number":1126,"context_line":"                                            \u0027consistencygroup_id\u0027: \u0027g1\u0027,"}],"source_content_type":"text/x-python","patch_set":5,"id":"ba3cc151_6e4e5c39","line":1123,"in_reply_to":"ba3cc151_65ec0090","updated":"2015-07-09 15:20:30.000000000","message":"Good catch, Tom!\n\nIMO, using ddt library will make our tests cleaner but I\u0027m OK to make it in a separate patch/blueprint.","commit_id":"ca904e980648a95c718f11b3bf3fb431f9339150"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"f9b7fd406947ffd071aef287ec84b57a9b547cf5","unresolved":false,"context_lines":[{"line_number":1120,"context_line":"                                                self.ctxt,"},{"line_number":1121,"context_line":"                                                filters))"},{"line_number":1122,"context_line":""},{"line_number":1123,"context_line":"    def test_cgsnapshot_get_all_by_project(self):"},{"line_number":1124,"context_line":"        cgsnapshot1 \u003d db.cgsnapshot_create(self.ctxt,"},{"line_number":1125,"context_line":"                                           {\u0027id\u0027: 1,"},{"line_number":1126,"context_line":"                                            \u0027consistencygroup_id\u0027: \u0027g1\u0027,"}],"source_content_type":"text/x-python","patch_set":10,"id":"ba3cc151_b2e2532e","line":1123,"updated":"2015-07-10 13:05:32.000000000","message":"It would be nice to have a test for get_all_by_group as well, but since the code is basically calling the same method as this one I won\u0027t be too picky about it.","commit_id":"7433bd232865a4449032b117a550fafcfc8404b6"},{"author":{"_account_id":9171,"name":"Vipin Balachandran","email":"vipin.bl@gmail.com","username":"vbala"},"change_message_id":"a4edc18d70d463ca7de27bfa3d33fbb22fd7e6e9","unresolved":false,"context_lines":[{"line_number":1146,"context_line":"                                                self.ctxt,"},{"line_number":1147,"context_line":"                                                filters))"},{"line_number":1148,"context_line":""},{"line_number":1149,"context_line":"    def test_cgsnapshot_get_all_by_group(self):"},{"line_number":1150,"context_line":"        cgsnapshot1 \u003d db.cgsnapshot_create(self.ctxt,"},{"line_number":1151,"context_line":"                                           {\u0027id\u0027: 1,"},{"line_number":1152,"context_line":"                                            \u0027consistencygroup_id\u0027: \u0027g1\u0027})"}],"source_content_type":"text/x-python","patch_set":12,"id":"9a41bdd9_fd3e5eb7","line":1149,"updated":"2015-07-13 03:48:19.000000000","message":"Already covered by test_cgsnapshot_get_all_by_filter","commit_id":"10a4047d20a26c5c09ba7d80b52db80efd54f41c"},{"author":{"_account_id":1736,"name":"Ivan Kolodyazhny","email":"e0ne@e0ne.info","username":"e0ne"},"change_message_id":"5a3d8a3ab097e2ffb720426f3f92cf891c5f1a93","unresolved":false,"context_lines":[{"line_number":1146,"context_line":"                                                self.ctxt,"},{"line_number":1147,"context_line":"                                                filters))"},{"line_number":1148,"context_line":""},{"line_number":1149,"context_line":"    def test_cgsnapshot_get_all_by_group(self):"},{"line_number":1150,"context_line":"        cgsnapshot1 \u003d db.cgsnapshot_create(self.ctxt,"},{"line_number":1151,"context_line":"                                           {\u0027id\u0027: 1,"},{"line_number":1152,"context_line":"                                            \u0027consistencygroup_id\u0027: \u0027g1\u0027})"}],"source_content_type":"text/x-python","patch_set":12,"id":"9a41bdd9_8d7b5f7b","line":1149,"in_reply_to":"9a41bdd9_fd3e5eb7","updated":"2015-07-17 10:23:34.000000000","message":"Agree","commit_id":"10a4047d20a26c5c09ba7d80b52db80efd54f41c"},{"author":{"_account_id":11904,"name":"Sean McGinnis","email":"sean.mcginnis@gmail.com","username":"SeanM"},"change_message_id":"a2e7b79a8969d824dd505cf40495a2165f739467","unresolved":false,"context_lines":[{"line_number":1213,"context_line":"                                                self.ctxt,"},{"line_number":1214,"context_line":"                                                filters))"},{"line_number":1215,"context_line":""},{"line_number":1216,"context_line":"    def test_cgsnapshot_get_all_by_project(self):"},{"line_number":1217,"context_line":"        cgsnapshot1 \u003d db.cgsnapshot_create(self.ctxt,"},{"line_number":1218,"context_line":"                                           {\u0027id\u0027: 1,"},{"line_number":1219,"context_line":"                                            \u0027consistencygroup_id\u0027: \u0027g1\u0027,"}],"source_content_type":"text/x-python","patch_set":13,"id":"3a50d1a3_233fd0b5","line":1216,"updated":"2015-07-20 12:16:32.000000000","message":"Should also have tests for filtering by group and filtering with invalid input.","commit_id":"044a359f149352e39d220a77664955a926200d89"},{"author":{"_account_id":14305,"name":"Yuriy Nesenenko","email":"ynesenenko@mirantis.com","username":"yuriy_n"},"change_message_id":"c6da6597d86d1471bbbd9e1c9eb5bcc5b1f5ec60","unresolved":false,"context_lines":[{"line_number":1213,"context_line":"                                                self.ctxt,"},{"line_number":1214,"context_line":"                                                filters))"},{"line_number":1215,"context_line":""},{"line_number":1216,"context_line":"    def test_cgsnapshot_get_all_by_project(self):"},{"line_number":1217,"context_line":"        cgsnapshot1 \u003d db.cgsnapshot_create(self.ctxt,"},{"line_number":1218,"context_line":"                                           {\u0027id\u0027: 1,"},{"line_number":1219,"context_line":"                                            \u0027consistencygroup_id\u0027: \u0027g1\u0027,"}],"source_content_type":"text/x-python","patch_set":13,"id":"3a50d1a3_a3a680bf","line":1216,"in_reply_to":"3a50d1a3_233fd0b5","updated":"2015-07-20 12:24:31.000000000","message":"Already covered by test_cgsnapshot_get_all_by_filter.","commit_id":"044a359f149352e39d220a77664955a926200d89"},{"author":{"_account_id":11904,"name":"Sean McGinnis","email":"sean.mcginnis@gmail.com","username":"SeanM"},"change_message_id":"94b6e09537b25738c2802bfb575852e9462cfe6b","unresolved":false,"context_lines":[{"line_number":1213,"context_line":"                                                self.ctxt,"},{"line_number":1214,"context_line":"                                                filters))"},{"line_number":1215,"context_line":""},{"line_number":1216,"context_line":"    def test_cgsnapshot_get_all_by_project(self):"},{"line_number":1217,"context_line":"        cgsnapshot1 \u003d db.cgsnapshot_create(self.ctxt,"},{"line_number":1218,"context_line":"                                           {\u0027id\u0027: 1,"},{"line_number":1219,"context_line":"                                            \u0027consistencygroup_id\u0027: \u0027g1\u0027,"}],"source_content_type":"text/x-python","patch_set":13,"id":"3a50d1a3_43b0dcdd","line":1216,"in_reply_to":"3a50d1a3_a3a680bf","updated":"2015-07-20 12:28:24.000000000","message":"Ah, missed that part. Why is project ID separate then.\n\nI would recommend moving all of these into one test and using ddt to have multiple runs that each test a specific path through the code.","commit_id":"044a359f149352e39d220a77664955a926200d89"},{"author":{"_account_id":1736,"name":"Ivan Kolodyazhny","email":"e0ne@e0ne.info","username":"e0ne"},"change_message_id":"31c91663cda0deb527ec3b23f3aab4356b70563d","unresolved":false,"context_lines":[{"line_number":1213,"context_line":"                                                self.ctxt,"},{"line_number":1214,"context_line":"                                                filters))"},{"line_number":1215,"context_line":""},{"line_number":1216,"context_line":"    def test_cgsnapshot_get_all_by_project(self):"},{"line_number":1217,"context_line":"        cgsnapshot1 \u003d db.cgsnapshot_create(self.ctxt,"},{"line_number":1218,"context_line":"                                           {\u0027id\u0027: 1,"},{"line_number":1219,"context_line":"                                            \u0027consistencygroup_id\u0027: \u0027g1\u0027,"}],"source_content_type":"text/x-python","patch_set":13,"id":"3a50d1a3_c3494cd2","line":1216,"in_reply_to":"3a50d1a3_a3a680bf","updated":"2015-07-20 12:29:25.000000000","message":"I\u0027m agree with Yuriy for these cases","commit_id":"044a359f149352e39d220a77664955a926200d89"},{"author":{"_account_id":1736,"name":"Ivan Kolodyazhny","email":"e0ne@e0ne.info","username":"e0ne"},"change_message_id":"31c91663cda0deb527ec3b23f3aab4356b70563d","unresolved":false,"context_lines":[{"line_number":1231,"context_line":""},{"line_number":1232,"context_line":"        for filters, expected in tests:"},{"line_number":1233,"context_line":"            self._assertEqualListsOfObjects(expected,"},{"line_number":1234,"context_line":"                                            db.cgsnapshot_get_all("},{"line_number":1235,"context_line":"                                                self.ctxt,"},{"line_number":1236,"context_line":"                                                filters))"},{"line_number":1237,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"3a50d1a3_a31760a2","line":1234,"updated":"2015-07-20 12:29:25.000000000","message":"Should be db.cgsnapshot_get_all_by_project","commit_id":"044a359f149352e39d220a77664955a926200d89"}]}