Code review comment for lp:~gary/launchpad/bug744888

Revision history for this message
Aaron Bentley (abentley) wrote :

If StormStatementRecorder works for your test, I recommend using that instead.

Please change the SQL check as discussed on IRC:
<abentley> gary_poster: I wonder whether there's risk of your SQL check hitting false negatives? Would it be crazy to just look for the string 'message' in the entire statement?
<gary_poster> abentley: looking for message: well...there are also "bugmessage"s in theory...it's obviously a balance between possible false negatives and false positives.
<gary_poster> I guess we could try being a bit more aggressive. I'd be OK with simply splitting on whitespace and making sure there are no strings that start with "message" (which would include tables). If people find that too restrictive in the future they can adjust the test.

Aside from that, looks fine.

review: Approve

« Back to merge proposal