)]}'
{"REVIEWING.rst":[{"author":{"_account_id":1192,"name":"David Kranz","email":"dkranz@redhat.com","username":"david-kranz"},"change_message_id":"f5bac8c10f48118d302f3dd2b645671631979077","unresolved":false,"context_lines":[{"line_number":11,"context_line":"For any new or change to a test it has to be verified in the gate. This means"},{"line_number":12,"context_line":"that the first thing to check with any change is that a gate job actually runs"},{"line_number":13,"context_line":"it. Tests which aren\u0027t executed either because of configuration or skips should"},{"line_number":14,"context_line":"not be accepted."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Unit Tests"}],"source_content_type":"text/x-rst","patch_set":1,"id":"1ae5cdf2_70031f42","line":14,"updated":"2014-06-11 19:39:47.000000000","message":"I know it is controversial but IMO this is too harsh. We run a very limited set of configurations in the gate.  For example, tempest does not work with vlan nova-networking. If some one proposed a fix for that, should we reject it? I believe there are also tests for various consoles, migration and other things that are always skipped. People run tempest outside the gate and if they find bugs in configs we don\u0027t run, we should consider a fix useful since we might actually use a different config in the future. I understand the desire to avoid bit rot in tests that never run but I think this should be a judgement call of the core review team rather than a strict prohibition.","commit_id":"c4c6f18e7603bcc89bf97f8633414fbefd8d3f33"},{"author":{"_account_id":5196,"name":"Matthew Treinish","email":"mtreinish@kortar.org","username":"treinish"},"change_message_id":"4b57d610d0b6a2b556a9c0f9c3f1500060fea7f4","unresolved":false,"context_lines":[{"line_number":11,"context_line":"For any new or change to a test it has to be verified in the gate. This means"},{"line_number":12,"context_line":"that the first thing to check with any change is that a gate job actually runs"},{"line_number":13,"context_line":"it. Tests which aren\u0027t executed either because of configuration or skips should"},{"line_number":14,"context_line":"not be accepted."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Unit Tests"}],"source_content_type":"text/x-rst","patch_set":1,"id":"1ae5cdf2_4f64c1b8","line":14,"in_reply_to":"1ae5cdf2_70031f42","updated":"2014-06-11 20:30:54.000000000","message":"We\u0027re not going to be able to catch everything in the gate or tempest in general, it\u0027s always been a best effort thing. (limited people, resources, etc..) It\u0027s more than just bit-rot though, the problem is that tempest largely is still only self validating instead of being externally tested with unit tests. The only way we can be assured that the code is working as intended is if it\u0027s run. As a reviewer it\u0027s actually difficult to catch subtle bugs most of the time which is why we have automated testing. Approving code that has never been run is just a bad idea, the bugs we used to have in tree because things were never run were pretty egregious. (just think of after I fixed the testr skipping tests with syntax errors bug how many broken things we had in tree) Everytime this restriction gets loose, either because of a bug or not, we just merge things that don\u0027t work or bit rot withing a week or 2 which I think warrants this provision.\n\nWe definitely have tests in tree that we don\u0027t gate on, most of those are grandfathered in from before we made things more strict. (Like the live migration tests, which as I understand it has tons of bugs in nova because we can\u0027t gate one it) I get that people do run tempest outside of the gate but the amount of bugs and fixes we actually get in tempest around this is very minimal. For example to date I don\u0027t believe we\u0027ve ever gotten a bug or a fix for something like the live migration tests. I think they\u0027re so rare that we can handle them as special cases because it\u0027s in tree but not run. Honestly though, most of what I see from people who run tempest outside of the gate are fixes to hard coded devstack assumptions we have in tree which are always valid fixes. We almost never see any reported issues for code paths we don\u0027t exercise in the gate, with the exception of no tenant-isolation which we cover poorly with periodic jobs.","commit_id":"c4c6f18e7603bcc89bf97f8633414fbefd8d3f33"},{"author":{"_account_id":1192,"name":"David Kranz","email":"dkranz@redhat.com","username":"david-kranz"},"change_message_id":"f5bac8c10f48118d302f3dd2b645671631979077","unresolved":false,"context_lines":[{"line_number":28,"context_line":"-------------"},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"Tests that are being added for APIs which don\u0027t conform to the API stability"},{"line_number":31,"context_line":"guidelines should not be approved."},{"line_number":32,"context_line":""},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"When to approve"}],"source_content_type":"text/x-rst","patch_set":1,"id":"1ae5cdf2_106adbed","line":31,"updated":"2014-06-11 19:39:47.000000000","message":"I\u0027m not sure what this means. The stability guidelines are about what changes to an existing api are allowed. I think you mean that a project must have officially declared an api to be stable before tempest tests can be created. That is obviously going to be true for all integrated apis but projects seeking integration need to state when they are ready.","commit_id":"c4c6f18e7603bcc89bf97f8633414fbefd8d3f33"},{"author":{"_account_id":5196,"name":"Matthew Treinish","email":"mtreinish@kortar.org","username":"treinish"},"change_message_id":"4b57d610d0b6a2b556a9c0f9c3f1500060fea7f4","unresolved":false,"context_lines":[{"line_number":28,"context_line":"-------------"},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"Tests that are being added for APIs which don\u0027t conform to the API stability"},{"line_number":31,"context_line":"guidelines should not be approved."},{"line_number":32,"context_line":""},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"When to approve"}],"source_content_type":"text/x-rst","patch_set":1,"id":"1ae5cdf2_4b8676dc","line":31,"in_reply_to":"1ae5cdf2_106adbed","updated":"2014-06-11 20:30:54.000000000","message":"The problem is a stable API means different things to different people. I was trying to explicitly point to a definition of what a stable api is. Just calling an api stable isn\u0027t consistent between projects unfortunately, do you have a suggestion on how to word this better?","commit_id":"c4c6f18e7603bcc89bf97f8633414fbefd8d3f33"},{"author":{"_account_id":1192,"name":"David Kranz","email":"dkranz@redhat.com","username":"david-kranz"},"change_message_id":"f5bac8c10f48118d302f3dd2b645671631979077","unresolved":false,"context_lines":[{"line_number":30,"context_line":"Tests that are being added for APIs which don\u0027t conform to the API stability"},{"line_number":31,"context_line":"guidelines should not be approved."},{"line_number":32,"context_line":""},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"When to approve"},{"line_number":35,"context_line":"---------------"},{"line_number":36,"context_line":" * Every patch needs two +2s before being approved."}],"source_content_type":"text/x-rst","patch_set":1,"id":"1ae5cdf2_8b3f9eb3","line":33,"updated":"2014-06-11 19:39:47.000000000","message":"I would add:\n\nReject copy and paste test code\n---------------------------------------------\n\nWhen creating new tests that are similar to existing tests it is tempting to simply copy the code and make a few modifications. This increases code size and the maintenance burden. Such changes should not be approved if it is easy to abstract the duplicated code into a function or method.","commit_id":"c4c6f18e7603bcc89bf97f8633414fbefd8d3f33"},{"author":{"_account_id":5196,"name":"Matthew Treinish","email":"mtreinish@kortar.org","username":"treinish"},"change_message_id":"4b57d610d0b6a2b556a9c0f9c3f1500060fea7f4","unresolved":false,"context_lines":[{"line_number":30,"context_line":"Tests that are being added for APIs which don\u0027t conform to the API stability"},{"line_number":31,"context_line":"guidelines should not be approved."},{"line_number":32,"context_line":""},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"When to approve"},{"line_number":35,"context_line":"---------------"},{"line_number":36,"context_line":" * Every patch needs two +2s before being approved."}],"source_content_type":"text/x-rst","patch_set":1,"id":"1ae5cdf2_0b0b8e8e","line":33,"in_reply_to":"1ae5cdf2_8b3f9eb3","updated":"2014-06-11 20:30:54.000000000","message":"ok I\u0027ll add that in the next revision.","commit_id":"c4c6f18e7603bcc89bf97f8633414fbefd8d3f33"},{"author":{"_account_id":5803,"name":"Attila Fazekas","email":"afazekas@redhat.com","username":"afazekas"},"change_message_id":"dd65a26e9316e21cfc086fcdb779602d85691205","unresolved":false,"context_lines":[{"line_number":55,"context_line":"---------------"},{"line_number":56,"context_line":" * Every patch needs two +2s before being approved."},{"line_number":57,"context_line":" * Its ok to hold off on an approval until a subject matter expert reviews it"},{"line_number":58,"context_line":" * If a patch has already been approved but requires a trivial rebase to merge, you do not have to wait for a second +2, since the patch has already had two +2s."}],"source_content_type":"text/x-rst","patch_set":3,"id":"1abeadc6_6ee38a33","line":58,"updated":"2014-08-19 12:38:49.000000000","message":"Too long line.","commit_id":"4b3bccbcf7f82eb3dd24ed3e2541d2e1184d3e58"},{"author":{"_account_id":5196,"name":"Matthew Treinish","email":"mtreinish@kortar.org","username":"treinish"},"change_message_id":"61cb49a68634b5abc807d32080ffe34616ff40de","unresolved":false,"context_lines":[{"line_number":55,"context_line":"---------------"},{"line_number":56,"context_line":" * Every patch needs two +2s before being approved."},{"line_number":57,"context_line":" * Its ok to hold off on an approval until a subject matter expert reviews it"},{"line_number":58,"context_line":" * If a patch has already been approved but requires a trivial rebase to merge, you do not have to wait for a second +2, since the patch has already had two +2s."}],"source_content_type":"text/x-rst","patch_set":3,"id":"1abeadc6_3fea27ac","line":58,"in_reply_to":"1abeadc6_6ee38a33","updated":"2014-08-22 00:54:21.000000000","message":"Done","commit_id":"4b3bccbcf7f82eb3dd24ed3e2541d2e1184d3e58"},{"author":{"_account_id":7872,"name":"Marc Koderer","email":"marc@koderer.com","username":"mkoderer"},"change_message_id":"20d65aeac9706c1e207f35ca1705f4825112c761","unresolved":false,"context_lines":[{"line_number":34,"context_line":""},{"line_number":35,"context_line":""},{"line_number":36,"context_line":"Reject Copy and Paste Test Code"},{"line_number":37,"context_line":"------------------------"},{"line_number":38,"context_line":"When creating new tests that are similar to existing tests it is tempting to"},{"line_number":39,"context_line":"simply copy the code and make a few modifications. This increases code size and"},{"line_number":40,"context_line":"the maintenance burden. Such changes should not be approved if it is easy to"}],"source_content_type":"text/x-rst","patch_set":4,"id":"1abeadc6_dd1f7159","line":37,"updated":"2014-08-22 09:33:29.000000000","message":"There are some\u0027-\u0027 missing","commit_id":"16dd51b50f9c44d757f9ea6a64cc6746d33c30a2"},{"author":{"_account_id":7872,"name":"Marc Koderer","email":"marc@koderer.com","username":"mkoderer"},"change_message_id":"20d65aeac9706c1e207f35ca1705f4825112c761","unresolved":false,"context_lines":[{"line_number":51,"context_line":"whether to skip or not."},{"line_number":52,"context_line":""},{"line_number":53,"context_line":""},{"line_number":54,"context_line":"When to approve"},{"line_number":55,"context_line":"---------------"},{"line_number":56,"context_line":" * Every patch needs two +2s before being approved."},{"line_number":57,"context_line":" * Its ok to hold off on an approval until a subject matter expert reviews it"}],"source_content_type":"text/x-rst","patch_set":4,"id":"1abeadc6_dd5771c8","line":54,"updated":"2014-08-22 09:33:29.000000000","message":"What would be also nice to cover:\n - When to give a \"-1\" and \"-2\" vote\n   - A -1 vote should always consist of a solution how to get the fix merged\n - It should appeal to people to communicate directly as much as possible. So a link to IRC channels and the QA Meeting would be nice.","commit_id":"16dd51b50f9c44d757f9ea6a64cc6746d33c30a2"}]}