Summary: | fix test-webkitpy, add easy way to find a checkout root | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dirk Pranke <dpranke> | ||||||
Component: | New Bugs | Assignee: | 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
Dirk Pranke
2010-08-13 19:03:12 PDT
Created attachment 64400 [details]
Patch
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 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 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.
(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. (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? (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. :) Created attachment 64536 [details]
Patch
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.
Committed r65572: <http://trac.webkit.org/changeset/65572> |