Bug 36958

Summary: webkitpy: unit test the networktransaction log messages
Product: WebKit Reporter: Chris Jerdonek <cjerdonek>
Component: Tools / TestsAssignee: Chris Jerdonek <cjerdonek>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cjerdonek, eric, hamaji
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Proposed patch
abarth: review-
Proposed patch 2 none

Description Chris Jerdonek 2010-04-01 07:23:55 PDT
The networktransaction.py log messages need to be unit-tested.

The purpose of this report is to be a mini-tutorial on how to unit-test log messages -- now that Python logging has been enabled for webkit-patch.
Comment 1 Chris Jerdonek 2010-04-01 07:39:00 PDT
Created attachment 52293 [details]
Proposed patch

Hi guys, this is all there is to it!

Note that the tearDown() method also asserts that the array of remaining log messages is empty.  This checks that there are no unanticipated log messages you *didn't* want to be present.  We get this extra check for free since it's in the tearDown(). 

(Shinichiro, you've already seen this pattern a number of times, so let's let Adam or Eric review it so we know at least one of them gets a chance to see it.  Thanks!)
Comment 2 Adam Barth 2010-04-01 09:18:34 PDT
Comment on attachment 52293 [details]
Proposed patch

Nice, but can we put the setUp and tearDown code into a base class so we don't have to copy/paste it all the time?
Comment 3 Eric Seidel (no email) 2010-04-01 11:20:15 PDT
I think it looks nice. :)
Comment 4 Chris Jerdonek 2010-04-01 15:01:18 PDT
(In reply to comment #2)
> (From update of attachment 52293 [details])
> Nice, but can we put the setUp and tearDown code into a base class so we don't
> have to copy/paste it all the time?

Yeah, certainly.  It crossed my mind, too.  It might be a bit heavy for just a couple lines of code, but since Python supports multiple inheritance, I guess we don't risk it presenting a problem down the road should we ever need another base class.  (I've never actually needed to use multiple inheritance in Python before.)
Comment 5 Chris Jerdonek 2010-04-02 00:35:01 PDT
Created attachment 52397 [details]
Proposed patch 2

Changed the patch to use a base class.

It seems possible that we could run into multiple inheritance issues down the road with this change after all.

This is because it seems that the base class I created needs to itself inherit from unittest.TestCase (as opposed to the calling class inheriting from both this base class and unittest.TestCase).  Otherwise, the setUp() and tearDown() methods of the base class don't seem to get fired.  If we ever needed to inherit from unittest.TestCase twice in this way, I'm not sure whether unittest will know to call the setUp() and tearDown() methods of both of the unittest.TestCase super classes.
Comment 6 Adam Barth 2010-04-02 00:38:39 PDT
Comment on attachment 52397 [details]
Proposed patch 2

Yeah, we can deal with the inheritance issues if they come up.  We've used this pattern before and been happy with it.  Thanks for the patch.
Comment 7 Chris Jerdonek 2010-04-02 01:31:48 PDT
Comment on attachment 52397 [details]
Proposed patch 2

Clearing flags on attachment: 52397

Committed r56988: <http://trac.webkit.org/changeset/56988>
Comment 8 Chris Jerdonek 2010-04-02 01:31:55 PDT
All reviewed patches have been landed.  Closing bug.