![[site community profile]](https://www.dreamwidth.org/img/comm_staff.png){kind=small}
Entry tags:
The Process of Reviewing
We've been seeing a lot of patches submitted lately, which is awesome (June hackathon, ILU ♥). So, now seems a perfect time to talk about reviews :D
Our workflow for filing and fixing bugs, assuming all goes well, is like this:
The dev makes sure that the basic functionality works okay. The patch should, in general, apply cleanly, and not have any syntax errors that would prevent the page contents from showing up, or the server from starting up at all. The patch should fix the issue mentioned in the bug!
The reviewer is there to confirm things that the dev did, and to catch things the dev missed. Reviewers:
* check for style
* check that the patch works to fix the basic issue mentioned
* check for any exceptional cases that might have been missed
* check that the patch doesn't break anything
* check for any security flaws
* possibly, make suggestions for things that might make the feature behave better (for instance, accessibility issues, or browser compatibility issues, or issues that crop up when JS is not enabled)
The committer is there as a final quality-checker. If you're new or not quite sure of your code, or worried that you'll break something, don't let that stop you from writing or speaking up. Your code / review can't break anything because we have your back. (And I hope that thought helps you as much as it helped me when I was starting out *g* Because I was scared that my code wouldn't be good enough, or would somehow break Dreamwidth, and knowing that![[staff profile]](https://www.dreamwidth.org/img/silk/identity/user_staff.png)
mark and ![[personal profile]](https://www.dreamwidth.org/img/silk/identity/user.png)
kareila were there to check what I was doing, so I couldn't break the site accidentally -- well that meant I had a safety net and could do things I wouldn't have otherwise dared to touch!)
If you see something that can be improved, the best time is to point it out while the patch is undergoing review and before it's been submitted. Sometimes your suggestion means the patch needs to be reworked completely. Sometimes, it means that the patch can be tweaked. And sometimes, the current solution is good enough, but your suggested improvement would be great, and can be moved to its own bug for future improvement.
So there are a couple of ways to go about this:
If you don't feel comfortable leaving a full review, you don't need to! However, if you're comfortable with the idea of applying the patch to your dreamhack, and confirming that it works as described, you can leave a comment saying so. If you're okay with comparing the code with the Programming Style Guidelines, you can politely point out if the code doesn't follow the style guidelines, or point to the style guidelines.
We don't generally reject for style. However, committers may tweak the patch to match style guidelines before commit, so having easy notes to refer to would be helpful. And we generally also try to point to specific style mistakes to raise awareness and build good habits ;-)
If you want to say something, but are afraid that it will mislead people into thinking the patch has been formally reviewed, you can qualify your comment with "Not a full review, but..." (not a full review, but the style looks good! Not a full review, but I noticed a syntax error that would prevent it from compiling. Not a full review, but this image won't work on journals with dark backgrounds -- like mine.)
Informal comments help the review work flow faster, because the person doing the formal review can then concentrate on the things that haven't been looked at, including exceptional cases.
When doing these, you don't need to change the review? flag; this makes sure that the patch still shows up for a formal review. If you see a major issue, you can mark it review-, so that the issue can be fixed.
In these, you'll want to change the review? flag to either review+ or review- after you're done. Instead of looking at just a specific part, you'll want to go over the patch as a whole. The checklist above for reviewers is a good starting point.
So, having said all that, you might be wondering what might be involved in testing or reviewing.
When testing, you'll want to first figure out how the site currently behaves. For bugs, look at the bug report -- what do you need to do to reproduce the bug? Do whatever you need to do to confirm that you can make the bug happen, when you do x, y, z. This will make sure that when you do apply the patch, you can confirm that the patch has changed existing behavior, because doing x, y, z no longer causes the bug.
Then you'll need to apply the patch to the code in your dreamhack. A patch is basically a set of changes that have been recorded in a file, which you can use to modify your own code by running a single command. This is good, because the alternative is to manually compare someone else's files with your own, manually picking out what has changed, and then modifying your own files to match.
First you need to download your patch. From the terminal on your dreamhack:
curl -o ~/patch http://bugs.dwscoalition.org... # this downloads the attachment from bugzilla into a file called patch in your home directory
There are two kinds of patches. If you see this line, or something like this line, in the patch:
then you'll need to apply the patch by doing the following:
cd $LJHOME
patch -p0 < ~/patch
If you see something like this instead:
then you can apply the patch by doing the following:
cd $LJHOME/cvs/dw-free # or cd $LJHOME/cvs/dw-nonfree if it's a file specific to Dreamwidth
patch -p1 < ~/patch
You'll get some messages -- if you see something about the patch being rejected, or hunks being rejected then that means that it wasn't successful. You'll have some files that were modified, and some additional files ending in .rej. Do not pass go, do not collect $200; since the patch program couldn't figure out how to integrate all their changes into your code, you won't be able to test properly. You can stop reviewing the patch, and instead go back to bugzilla and let the patch author know that their patch didn't apply cleanly. Also mention which files were rejected.
Then clean up your environment -- do what you usually do to remove any changes, since the patch may have managed to change some files.
(Note: the above is not strictly true. There are ways to fix things manually, but they're quite fiddly, and can be a subject of another post.)
Now, if you see something about applied but with a fuzz factor, ignore it. That means that the code didn't match exactly so the patch program had to guess a tiny bit, but it shouldn't matter. You'll have some modified files, and some new files ending in .orig. That's okay (you can delete the .orig files).
Else, everything applied successfully and cleanly, and you can now go on with actually reviewing the patch.
Easiest thing is to check for syntax errors in the code. Depending on what was touched, a syntax error could either cause the page to spew out a block of bolded error messages, or it could prevent the entire server from starting up. So, make sure that the changes are live on your site, then check that your server is up, and that the page is showing up properly. Syntax errors are grounds for rejection; they're pretty easy to find and fix.
Then read over the code, give it a once over take note of what it's trying to do -- and be mindful of the style guidelines. It might be useful to have the The programming guideline checklist open in a tab. As I mentioned above, we don't reject based on style alone -- but coding style can affect readability and maintainability of the code, so it's something we definitely want to take note of.
Even if you don't feel comfortable reviewing past this point, if you've checked that the patch applies, that there are no syntax errors, and that it follows the styles guidelines (or if it doesn't, then how), and left a comment on the bug saying so, that is already a major help. And one way to learn more about Dreamwidth, or about coding, is to read and analyze other people's code, so there's that, too.
Now it's time to test the functionality. Verify that the issue is fixed or that the new feature is working properly.
For bonus points, try to see if there's an edge case that the patch missed. Try to see if you can make things break. It's the most fun part of the process.
Beyond what you think might be immediately useful from the description of the bug/patch, look at the code for ideas for what to test. Does it touch CSS and JS? Try it out in different browsers. Does it require looking up a user setting (if ( $remote->...) )? What happens then if someone tries to access the page while not logged in. Is it meant to be a paid account feature? Try it with a free account, with a paid account, with a personal account, with community account. Does it accept and display user input? Try inputting foreign characters, or empty input, or invalid values, or HTML. Are you trying to do something that involves another user, and you're allowed to input their username? What happens then if you input an invalid user, or a suspended user, or a deleted user.
Think of what issues you might run into while trying to use it. Think of what issues you might run into if you were new to the site, and ran into this feature or page for the first time. Try anything you can think about that might be even remotely relevant. And have fun.
ETA: Also of interest isyvi's entry Review 101 which explains how to do reviews in more detail, and also gives examples (thanks cesy for reminding me!)
Our workflow for filing and fixing bugs, assuming all goes well, is like this:
- Someone sees a issue, reports it. A new bug is filed in Bugzilla
- A dev sees the bug ticket, decides to fix it, writes a patch and uploads it to Bugzilla.
- A reviewer looks at and tests the patch -- does it work? does it fix what it's meant to fix? does it break anything accidentally? The patch is marked as reviewed.
- A committer looks over the patch again briefly, makes sure that nothing got overlooked, before committing it.
The dev makes sure that the basic functionality works okay. The patch should, in general, apply cleanly, and not have any syntax errors that would prevent the page contents from showing up, or the server from starting up at all. The patch should fix the issue mentioned in the bug!
The reviewer is there to confirm things that the dev did, and to catch things the dev missed. Reviewers:
* check for style
* check that the patch works to fix the basic issue mentioned
* check for any exceptional cases that might have been missed
* check that the patch doesn't break anything
* check for any security flaws
* possibly, make suggestions for things that might make the feature behave better (for instance, accessibility issues, or browser compatibility issues, or issues that crop up when JS is not enabled)
The committer is there as a final quality-checker. If you're new or not quite sure of your code, or worried that you'll break something, don't let that stop you from writing or speaking up. Your code / review can't break anything because we have your back. (And I hope that thought helps you as much as it helped me when I was starting out *g* Because I was scared that my code wouldn't be good enough, or would somehow break Dreamwidth, and knowing that
mark and kareila were there to check what I was doing, so I couldn't break the site accidentally -- well that meant I had a safety net and could do things I wouldn't have otherwise dared to touch!)If you see something that can be improved, the best time is to point it out while the patch is undergoing review and before it's been submitted. Sometimes your suggestion means the patch needs to be reworked completely. Sometimes, it means that the patch can be tweaked. And sometimes, the current solution is good enough, but your suggested improvement would be great, and can be moved to its own bug for future improvement.
So there are a couple of ways to go about this:
Informal comments
If you don't feel comfortable leaving a full review, you don't need to! However, if you're comfortable with the idea of applying the patch to your dreamhack, and confirming that it works as described, you can leave a comment saying so. If you're okay with comparing the code with the Programming Style Guidelines, you can politely point out if the code doesn't follow the style guidelines, or point to the style guidelines.
We don't generally reject for style. However, committers may tweak the patch to match style guidelines before commit, so having easy notes to refer to would be helpful. And we generally also try to point to specific style mistakes to raise awareness and build good habits ;-)
If you want to say something, but are afraid that it will mislead people into thinking the patch has been formally reviewed, you can qualify your comment with "Not a full review, but..." (not a full review, but the style looks good! Not a full review, but I noticed a syntax error that would prevent it from compiling. Not a full review, but this image won't work on journals with dark backgrounds -- like mine.)
Informal comments help the review work flow faster, because the person doing the formal review can then concentrate on the things that haven't been looked at, including exceptional cases.
When doing these, you don't need to change the review? flag; this makes sure that the patch still shows up for a formal review. If you see a major issue, you can mark it review-, so that the issue can be fixed.
Formal review
In these, you'll want to change the review? flag to either review+ or review- after you're done. Instead of looking at just a specific part, you'll want to go over the patch as a whole. The checklist above for reviewers is a good starting point.
So, having said all that, you might be wondering what might be involved in testing or reviewing.
What to do, how to test
When testing, you'll want to first figure out how the site currently behaves. For bugs, look at the bug report -- what do you need to do to reproduce the bug? Do whatever you need to do to confirm that you can make the bug happen, when you do x, y, z. This will make sure that when you do apply the patch, you can confirm that the patch has changed existing behavior, because doing x, y, z no longer causes the bug.
Then you'll need to apply the patch to the code in your dreamhack. A patch is basically a set of changes that have been recorded in a file, which you can use to modify your own code by running a single command. This is good, because the alternative is to manually compare someone else's files with your own, manually picking out what has changed, and then modifying your own files to match.
First you need to download your patch. From the terminal on your dreamhack:
curl -o ~/patch http://bugs.dwscoalition.org... # this downloads the attachment from bugzilla into a file called patch in your home directory
There are two kinds of patches. If you see this line, or something like this line, in the patch:
main --> dw-free ...
--- cvs/dw-free/bin/...
+++ bin/...
then you'll need to apply the patch by doing the following:
cd $LJHOME
patch -p0 < ~/patch
If you see something like this instead:
--- a/bin/...
+++ b/bin/...then you can apply the patch by doing the following:
cd $LJHOME/cvs/dw-free # or cd $LJHOME/cvs/dw-nonfree if it's a file specific to Dreamwidth
patch -p1 < ~/patch
You'll get some messages -- if you see something about the patch being rejected, or hunks being rejected then that means that it wasn't successful. You'll have some files that were modified, and some additional files ending in .rej. Do not pass go, do not collect $200; since the patch program couldn't figure out how to integrate all their changes into your code, you won't be able to test properly. You can stop reviewing the patch, and instead go back to bugzilla and let the patch author know that their patch didn't apply cleanly. Also mention which files were rejected.
Then clean up your environment -- do what you usually do to remove any changes, since the patch may have managed to change some files.
(Note: the above is not strictly true. There are ways to fix things manually, but they're quite fiddly, and can be a subject of another post.)
Now, if you see something about applied but with a fuzz factor, ignore it. That means that the code didn't match exactly so the patch program had to guess a tiny bit, but it shouldn't matter. You'll have some modified files, and some new files ending in .orig. That's okay (you can delete the .orig files).
Else, everything applied successfully and cleanly, and you can now go on with actually reviewing the patch.
Easiest thing is to check for syntax errors in the code. Depending on what was touched, a syntax error could either cause the page to spew out a block of bolded error messages, or it could prevent the entire server from starting up. So, make sure that the changes are live on your site, then check that your server is up, and that the page is showing up properly. Syntax errors are grounds for rejection; they're pretty easy to find and fix.
Then read over the code, give it a once over take note of what it's trying to do -- and be mindful of the style guidelines. It might be useful to have the The programming guideline checklist open in a tab. As I mentioned above, we don't reject based on style alone -- but coding style can affect readability and maintainability of the code, so it's something we definitely want to take note of.
Even if you don't feel comfortable reviewing past this point, if you've checked that the patch applies, that there are no syntax errors, and that it follows the styles guidelines (or if it doesn't, then how), and left a comment on the bug saying so, that is already a major help. And one way to learn more about Dreamwidth, or about coding, is to read and analyze other people's code, so there's that, too.
Now it's time to test the functionality. Verify that the issue is fixed or that the new feature is working properly.
For bonus points, try to see if there's an edge case that the patch missed. Try to see if you can make things break. It's the most fun part of the process.
Beyond what you think might be immediately useful from the description of the bug/patch, look at the code for ideas for what to test. Does it touch CSS and JS? Try it out in different browsers. Does it require looking up a user setting (if ( $remote->...) )? What happens then if someone tries to access the page while not logged in. Is it meant to be a paid account feature? Try it with a free account, with a paid account, with a personal account, with community account. Does it accept and display user input? Try inputting foreign characters, or empty input, or invalid values, or HTML. Are you trying to do something that involves another user, and you're allowed to input their username? What happens then if you input an invalid user, or a suspended user, or a deleted user.
Think of what issues you might run into while trying to use it. Think of what issues you might run into if you were new to the site, and ran into this feature or page for the first time. Try anything you can think about that might be even remotely relevant. And have fun.
ETA: Also of interest is
yvi's entry Review 101 which explains how to do reviews in more detail, and also gives examples (thanks cesy for reminding me!)