Reviewing Steps

[crschmidt]

This doesn't really belong here, but it's too simplistic for lj_dev, and we don't exactly have a dev-lounge that I can post in. So, I'm posting it here. It's steps (from my point of view) for testing and reviewing patches submitted to LiveJournal's bug tracking and enhancement system. It won't make sense to everyone, but what the hell, it's my journal, and I'm sure I can get some constructive feedback here.

Step 1. Get a LiveJournal Install.
Step 2. Update to CVS. This can just be LiveJournal or it can be livejournal, ljcom, bml, wcmtools... whatever kind of patch you're testing. But update
Step 3. Get the patch file you're going to review. wget, whatever. Get the file to where your install is.
Step 4. Apply the patch. This is an easy step to mark some patches need-improvement: if the patch doesn't apply cleanly (if any hunks failed) the patch needs to have some part of it rewritten. If you can't do it, you should just mark the bug need-improvement and let the author or someone else rewrite it. If a patch succeeds with fuzz, or shifts by some x number of lines, that's fine, but submit a new patch to zilla. That way other people can try out the clean patch as well.


Step 5. Test

This is the most difficult part of the process. (Not that this should come as a great shock to you.) Basically, you will need to understand what the bad behavior was, how it was changed, and what else has and hasn't changed. You need to understand all the logic pathways a page or function can take, and evaluate both sides of them.

For example:
I recently reviewed a patch that changed the behavior of the "You replied" filter to include requests closed within the past 24 hours (rather than just requests that were still "open"). To review, I had to test several different things.

1. Requests that are marked open show up in my youreplied.


2. Requests that are marked closed within the past 24 hours show up in my youreplied.


3. Requests that are marked closed and greater than 24 hours do NOT show up.

The last part is the most important. You need to test all possible outcomes, not just the expected ones. The point of reviewing is to find and demonstrate unexpected behavior.

You should test patches under all possible conditions. If a code section is supposed to behave differently for a deleted user than for a Suspended user than for a normal user, test all conditions under which the code might act differently.

Throw everything at it that can possibly break. Anything you don't think to do, users will, and users are much less forgiving when things fail. Unfortunately, there's nothing so wonderful at breaking things as a 1.4 million user userbase, and there are almost always some kinks that aren't seen until things get pushed live. As a tester, your job is to reduce these bugs to a minimum.

Never mark a patch reviewed by just looking at the code. You can not turn the entire LiveJournal server code around in your head - don't pretend you can. Test every part of every bug. Test every crack, hole and everything else you can find. If the patch will not break anything, and you are absolutely sure of it, mark it reviewed.

Comments

[littledownpour.livejournal.com]

testing is fun =) i like attempting to break things in all possible ways =)

this isn't a very constructive comment, but still. hi. and stuff.