Bug 44001 – fix test-webkitpy, add easy way to find a checkout root

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

fix test-webkitpy, add easy way to find a checkout root

[Dirk Pranke]

Created attachment 64400 [details]
Patch

[Dirk Pranke]

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?

[Adam Barth]

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

[Eric Seidel (no email)]

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.

[Dirk Pranke]

(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.

[Dirk Pranke]

(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?

[Eric Seidel (no email)]

(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. :)

[Dirk Pranke]

Created attachment 64536 [details]
Patch

[Eric Seidel (no email)]

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.

[Dirk Pranke]

Committed r65572: <http://trac.webkit.org/changeset/65572>