Last updated 2 April 2015.
Start your post by thanking the contributor (be specific!) and identifying what was done correctly. Then, include specific critiques or corrections. Close your review by giving an actionable next step for the issue where appropriate.
Example of a supportive, constructive review:
Thanks @Druplicon! Looks like this test follows the steps to reproduce and the failure is exposed in the test-only patch.
- The test should not be in
NodeTestBase. This is a base class that provides setup and helper methods for Node module tests. Instead, let's move the test to a class that extends
NodeTestBase. See the class hierarchy on the NodeTestBase API page.
- Note that the assertion on line 1337 is still passing in the test-only patch. Instead of asserting that an error message is not found, add an assertion for a correct result. In this case, I'd
assertResponse(200)and assert that the node's title text is found.
So, the next step is to create an updated patch that moves the test into a child class and changes the last assertion. Post a test-only patch followed by a combined patch, just like in #42 above.
Example of what not to write:
Thanks but this is totally wrong. You can't put the test in that class. Also the last assertion is bogus.
Problems with this review:
- Following "thanks" with "but" makes it so that you're no longer actually thanking the person. Keep your thanks separate from criticisms.
- "Totally wrong" and "bogus" are discouraging, insulting, and needlessly emphatic (and also probably false in some degree).
- The contributor might not understand the role of base test classes in core modules. Always clarify concepts that might be unfamiliar (or link to documentation).
- "The last assertion is bogus" is unspecific and unhelpful. Say exactly what's wrong with it, and do so in a way that isn't insulting.
- Finally, it appears that not much time was put into this reply. The contributor probably spent a lot of time working on the patch, so take the time to provide better feedback.
- Criticism and Ineffective Feedback by Kate Heddleston
- Fantastic guide to giving super useful help to contributors, and managing your own ability to respond to the deluge. http://sarah.thesharps.us/2014/09/01/the-gentle-art-of-patch-review/ by Sarah Sharp via kattekrab