Bug 44001

Summary: fix test-webkitpy, add easy way to find a checkout root
Product: WebKit Reporter: Dirk Pranke <dpranke>
Component: New BugsAssignee: Dirk Pranke <dpranke>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, bulach, cjerdonek, eric
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch eric: review+, eric: commit-queue-

Description Dirk Pranke 2010-08-13 19:03:12 PDT
fix test-webkitpy, add easy way to find a checkout root
Comment 1 Dirk Pranke 2010-08-13 19:06:04 PDT
Created attachment 64400 [details]
Patch
Comment 2 Dirk Pranke 2010-08-13 19:09:24 PDT
split off from bug 37630 to discuss cleaning up test-webkitpy and adding a generic method to find the root of the checkout.

I think this patch is basically sound, but it needs some unit tests for the new routines added to scm.py. 

Eric, Adam, Chris, what do you think?
Comment 3 Adam Barth 2010-08-13 19:13:51 PDT
Comment on attachment 64400 [details]
Patch

This looks good.  I bet if i was more in python mode I'd want some more tests.  :)


WebKitTools/ChangeLog:9
 +          the deduplicate_tests tests contaminate the test environment.
Contaminates
Comment 4 Eric Seidel (no email) 2010-08-13 19:15:27 PDT
Comment on attachment 64400 [details]
Patch

I think this is a great idea.  I'm glad you wrote this patch.  Thanks!

WebKitTools/Scripts/deduplicate-tests:66
 +      logutils.configure_logging()
Why move this to run instead of just at the top of the file?

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:54
 +  def default_scm():
Seems we should just make this a static method on SCM.

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:41
 +  def find_checkout_root():
Nice.  A bunch of scripts ported from chromium want this.

WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:242
 +          self._scm = scm.default_scm_system()
This call doesn't exist.  Are we missing a unit test?

Looks good except for the typo.
Comment 5 Dirk Pranke 2010-08-16 15:39:30 PDT
(In reply to comment #4)
> (From update of attachment 64400 [details])
> I think this is a great idea.  I'm glad you wrote this patch.  Thanks!
> 
> WebKitTools/Scripts/deduplicate-tests:66
>  +      logutils.configure_logging()
> Why move this to run instead of just at the top of the file?
>

No strong reason, I just am not a big fan of having mixing top-level statements and function definitions.
 
> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:54
>  +  def default_scm():
> Seems we should just make this a static method on SCM.
>

We certainly could, but I'm not sure that it buys us much of anything. I personally tend to avoid static/class methods, and prefer standalone functions because it (to me) more clearly implies that the function is stateless. It also seems that using static methods enforces two layers of namespace when you only need one, i.e. scm.scm.default_scm() instead of scm.default_scm() (or import scm.scm instead of import scm).
 
> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:41
>  +  def find_checkout_root():
> Nice.  A bunch of scripts ported from chromium want this.
> 

Indeed ;)

> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:242
>  +          self._scm = scm.default_scm_system()
> This call doesn't exist.  Are we missing a unit test?
>

Yes. The unit tests for rebaseline_chromium_webkit_tests don't really cover much. I'll add something at least slightly better.
Comment 6 Dirk Pranke 2010-08-16 15:53:11 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 64400 [details] [details])
> > I think this is a great idea.  I'm glad you wrote this patch.  Thanks!
> > 
> > WebKitTools/Scripts/deduplicate-tests:66
> >  +      logutils.configure_logging()
> > Why move this to run instead of just at the top of the file?
> >
> 
> No strong reason, I just am not a big fan of having mixing top-level statements and function definitions.
> 
> > WebKitTools/Scripts/webkitpy/common/checkout/scm.py:54
> >  +  def default_scm():
> > Seems we should just make this a static method on SCM.
> >
> 
> We certainly could, but I'm not sure that it buys us much of anything. I personally tend to avoid static/class methods, and prefer standalone functions because it (to me) more clearly implies that the function is stateless. It also seems that using static methods enforces two layers of namespace when you only need one, i.e. scm.scm.default_scm() instead of scm.default_scm() (or import scm.scm instead of import scm).

Sorry, meant to add, when do you prefer static/class methods?
Comment 7 Eric Seidel (no email) 2010-08-16 15:57:37 PDT
(In reply to comment #6)
> Sorry, meant to add, when do you prefer static/class methods?

I have no strong opinion. :) Thanks for asking.  I've definitely used both in the past.  Static/class methods have some mild advantage in importing ease, but using free functions makes imports more explicit.  Feel free to flip a coin, follow your gut, or whatever you like. :)
Comment 8 Dirk Pranke 2010-08-16 16:48:11 PDT
Created attachment 64536 [details]
Patch
Comment 9 Eric Seidel (no email) 2010-08-16 22:43:37 PDT
Comment on attachment 64536 [details]
Patch

WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py:199
 +          # Mock out stderr to avoid unnecessary error logging.
We often use our OutputCapture class for this.

WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py:210
 +          sys.stderr = open(os.devnull, 'a')
Yeah, OutputCapture would allow you to assert that it printed the expected strings here.

Looks good.  OutputCapture would be better in the tests.
Comment 10 Dirk Pranke 2010-08-17 16:11:45 PDT
Committed r65572: <http://trac.webkit.org/changeset/65572>