<div dir="ltr">+1This looks really great and well thought out. I appreciate the effort that went in to outlining this. I think this will be extremely helpful.Thanks,Wayne <div class="gmail_quote">On Jul 17, 2014 10:28 PM, "Tim Penhey" <<a href="mailto:tim.penhey@canonical.com" target="_blank">tim.penhey@canonical.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Hi all, Last night in the team meeting we discussed the review process. Initially because there have been a number of things sneaking through reviews where ideally we'd like to catch them earlier. In order to help the newer folks on the team learn how we review, it has been suggested that we formalise the mentored reviewers process. Mentored Reviewers ================== All juju core members are expected to review code. It is not reasonable to expect the newer members of the team to feel comfortable being the sole reviewer on work. I have created a new sheet for people to register themselves on if they feel they would like to have a review mentor [1]. You should discuss this with your team lead if you are unsure. A mentor will be allocated to you, or if you have discussed this with someone in particular, you can have a say in your own mentor. Reviews done by the mentored developer are not considered enough to land code until they have been approved by their mentor. It is up the mentor and mentee how they choose to do the reviews. They could do this over a hang out together, or the mentor could review the review after the mentee has done the first pass. Once the mentor feels that the mentee is consistently doing good reviews, they will announce the graduation, and that developer no longer needs a second review pass of their reviews. Guidelines ========== Prior to starting any significant piece of work or refactoring, the general approach should first be checked with the team lead and technical architect. Smaller more obvious pieces of work don't necessarily need this, but it is often good to talk to someone about the problem and the proposed fix as an initial sanity check. The aim here is to avoid getting into design discussions at review time as this is clearly the wrong time to do it. As the developer, it is strongly suggested that you work to the boy scout rule - "leave the code cleaner than you found it". We don't expect perfection (or we shouldn't), but don't leave a mess. It is ok to push back on a reviewer if you feel that the reviewer is being unreasonable in their request for perfection. If you feel that this is the case, talk to your team lead. As a reviewer, if you don't understand the code or what is being done there are a number of options: * it could be that the code is confusing, getting a second opinion can help here * it could be that you don't understand the area of code being changed, and it isn't always feasible to grok everything completely. If the change isn't obviously correct (to you), get a second opinion A review that says The code looks good, test coverage seems to be complete, but I can't tell if this will actually fix problem X is still a useful review, and worth doing. The developer should seek someone to understands that area, or work with the reviewer to help them understand. In any situation where you need to stop and explain it can seem like the process is too slow, but sharing and explaining is short term delay for long term gain as the knowledge is shared through the team. Checklist ========= I think it makes sense to have a check list to cover the basics of what we look for in reviews. Probably even get this into the docs directory in trunk. There is a document already which covers some basics of writing good tests: doc/how-to-write-tests.txt Initial things that I look for (off the top of my head): * firstly work out what the code is trying to fix (feature or bug) * is it obvious that the code does this? * for new functions, do the names make sense? do they say what the function does? * are all the public constants, variables, functions and methods documented? * how are the new functions tested? * can they be tested in isolation? * are all the new code paths tested? * do the names of the tests make sense? * could similar tests be combined into a table based test? * should a table based test be broken out in more obvious tests? (this sometimes happens when there is too much logic or too many conditionals in the actual test block of the table based test) * is this code valid for all operating systems? * are there any race conditions? * are the tests isolated from the environment? Cheers, Tim [1] <a href="https://docs.google.com/a/canonical.com/spreadsheets/d/1v9KB6Y4r1bMLOyB1JEs-wj_jBAvsq6EglWssa4cOx9c/edit#gid=0" target="_blank">https://docs.google.com/a/canonical.com/spreadsheets/d/1v9KB6Y4r1bMLOyB1JEs-wj_jBAvsq6EglWssa4cOx9c/edit#gid=0</a> -- Juju-dev mailing list <a href="mailto:Juju-dev@lists.ubuntu.com" target="_blank">Juju-dev@lists.ubuntu.com</a> Modify settings or unsubscribe at: <a href="https://lists.ubuntu.com/mailman/listinfo/juju-dev" target="_blank">https://lists.ubuntu.com/mailman/listinfo/juju-dev</a> </blockquote></div> </div>