Launchpad itself

Code review comment for lp:~henninge/launchpad/bug-545354-enable-sharing

bug-545354-enable-sharing
Merge into recife

Revision history for this message

Jeroen T. Vermeulen (jtv) wrote on 2010-06-21:

I looked over the incremental diff at Henning's request. A few notes, mostly already discussed on IRC. I think having the separate unit test for the regex matching (which was a bit of a wildcard, ha ha) worked out really well.

These came up on IRC:
* Backquote identifiers in docstrings: "See `IPOTemplateSharingSubset`."""
* getSharingPOTemplatesRegex says you're getting a regex, not templates; add "By."
* No need to create a Product and ProductSeries; create a series and use its product.

The unit tests mostly follow the pattern "create templates with names [...]; create a subset; select from subset by regex; verify output." Most of these would be one-liners if you had a method for "create templates with names [...] (parameter 1); create a subset; select from subset by regex (parameter 2); return list of names of the selected templates." The caller could then assertContentsEqual the result against a list of expected strings. That would also help with:
* Making the output more obvious (the relevance of "templates[1:]" is less clear).
* Testing lots of other things that could go wrong; see below.
* Eliminating implicit state. You wouldn't need any setUp at all.

Other notes:
* You may want to test that you're really getting a match, not a search: "xyz" should not match "y.*" As long as we're not taking the regex from user input but from existing template names, this is a bigger risk than unescaped quotes. (Not that I disagree with your approach of testing for those right from the start!)
* "# Quotes in the pattern can be dangerous." To me that looks like you're specifying the appropriate behaviour! How about "quotes do not confuse the regex match"?
* I'd test single and double quotes separately, not in the same string.
* When you test for quoting robustness, you might as well set up a template to establish that you don't get any unexpected matches.
* You don't exercise the case where no templates match.

Apart from those notes, the incremental diff has my unreserved blessing.

Jeroen

I looked over the incremental diff at Henning's request.  A few notes, mostly already discussed on IRC.  I think having the separate unit test for the regex matching (which was a bit of a wildcard, ha ha) worked out really well.

These came up on IRC:
 * Backquote identifiers in docstrings: "See `IPOTemplateSharingSubset`."""
 * getSharingPOTemplatesRegex says you're getting a regex, not templates; add "By."
 * No need to create a Product and ProductSeries; create a series and use its product.

The unit tests mostly follow the pattern "create templates with names [...]; create a subset; select from subset by regex; verify output."  Most of these would be one-liners if you had a method for "create templates with names [...] (parameter 1); create a subset; select from subset by regex (parameter 2); return list of names of the selected templates."  The caller could then assertContentsEqual the result against a list of expected strings.  That would also help with:
 * Making the output more obvious (the relevance of "templates[1:]" is less clear).
 * Testing lots of other things that could go wrong; see below.
 * Eliminating implicit state.  You wouldn't need any setUp at all.

Other notes:
 * You may want to test that you're really getting a match, not a search: "xyz" should not match "y.*"  As long as we're not taking the regex from user input but from existing template names, this is a bigger risk than unescaped quotes.  (Not that I disagree with your approach of testing for those right from the start!)
 * "# Quotes in the pattern can be dangerous."  To me that looks like you're specifying the appropriate behaviour!  How about "quotes do not confuse the regex match"?
 * I'd test single and double quotes separately, not in the same string.
 * When you test for quoting robustness, you might as well set up a template to establish that you don't get any unexpected matches.
 * You don't exercise the case where no templates match.

Apart from those notes, the incremental diff has my unreserved blessing.

Jeroen

« Back to merge proposal