[webkit-reviews] review denied: [Bug 33119] svn-apply breaks on patch files with leading white space : [Attachment 52855] proposed patch with unittests
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Apr 30 05:54:06 PDT 2010
Chris Jerdonek <cjerdonek at webkit.org> has denied Zoltan Horvath
<zoltan at webkit.org>'s request for review:
Bug 33119: svn-apply breaks on patch files with leading white space
https://bugs.webkit.org/show_bug.cgi?id=33119
Attachment 52855: proposed patch with unittests
https://bugs.webkit.org/attachment.cgi?id=52855&action=review
------- Additional Comments from Chris Jerdonek <cjerdonek at webkit.org>
Marking this patch r- because it no longer applies.
Also, some high-level comments regarding the directory being created here:
> +++ WebKitTools/Scripts/webkitperl/tools_unittest/svn-apply.pl
(revision 0)
Might it make more sense to continue the existing pattern of one *_unittest/
directory per file to be unit-tested? For example, svn-apply_unittest/ and
svn-unapply_unittest/?
If code can be shared between tests of code in svn-apply and svn-unapply,
maybe that can be handled by adding a test-support file in webkitperl/.
Finally, in cases where svn-apply and svn-unapply define the same or nearly
the same methods (because of cut-and-paste), it may also make sense to
refactor those areas to use the same code. Then for each refactored
function, one function can be unit-testted instead of two. That's not
something you necessarily need to be the one to do, but it's something
to consider.
More information about the webkit-reviews
mailing list