Bug 100780 - test-webkitpy: fix import of coverage so that it works in a clean install
Summary: test-webkitpy: fix import of coverage so that it works in a clean install
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks: 100787
  Show dependency treegraph
 
Reported: 2012-10-30 12:22 PDT by Dirk Pranke
Modified: 2012-10-30 16:17 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.78 KB, patch)
2012-10-30 12:23 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
refactor tests a bit (3.78 KB, patch)
2012-10-30 14:33 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
update w/ review feedback (9.01 KB, patch)
2012-10-30 15:40 PDT, Dirk Pranke
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2012-10-30 12:22:10 PDT
test-webkitpy: fix import of coverage so that it works in a clean install
Comment 1 Dirk Pranke 2012-10-30 12:23:08 PDT
Created attachment 171499 [details]
Patch
Comment 2 Dirk Pranke 2012-10-30 12:24:14 PDT
I'm not actually sure if this is the best way to fix this, or if it would be better to move coverage down a directory and put webkitpy/thirdparty/autoinstalled/coverage into sys.path, to minimize other potential conflicts. Thoughts?
Comment 3 Dirk Pranke 2012-10-30 14:21:14 PDT
Comment on attachment 171499 [details]
Patch

reworking a bit ... hang on.
Comment 4 Dirk Pranke 2012-10-30 14:33:10 PDT
Created attachment 171519 [details]
refactor tests a bit
Comment 5 Eric Seidel (no email) 2012-10-30 14:39:38 PDT
Comment on attachment 171519 [details]
refactor tests a bit

View in context: https://bugs.webkit.org/attachment.cgi?id=171519&action=review

> Tools/Scripts/webkitpy/thirdparty/__init__.py:130
> +        installed_something = installer.install(url="http://pypi.python.org/packages/source/c/coverage/coverage-3.5.1.tar.gz#md5=410d4c8155a4dab222f2bc51212d4a24", url_subpath="coverage-3.5.1/coverage")

installed_something?  Ah it's a bool?

> Tools/Scripts/webkitpy/thirdparty/__init__.py:132
> +        if not _AUTOINSTALLED_DIR in sys.path:
> +            sys.path.append(_AUTOINSTALLED_DIR)

Wow.  Nasty.  You might want to document this hack with a comment.
Comment 6 Dirk Pranke 2012-10-30 14:55:00 PDT
(In reply to comment #5)
> (From update of attachment 171519 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171519&action=review
> 
> > Tools/Scripts/webkitpy/thirdparty/__init__.py:130
> > +        installed_something = installer.install(url="http://pypi.python.org/packages/source/c/coverage/coverage-3.5.1.tar.gz#md5=410d4c8155a4dab222f2bc51212d4a24", url_subpath="coverage-3.5.1/coverage")
> 
> installed_something?  Ah it's a bool?
> 

Yes. This tracks whether installer.install() actually did work, and is used by autoinstall_everything() in test-webkitpy to see if we need to run the unit tests serial to get around some really weird bugs I hit a few months ago.

> > Tools/Scripts/webkitpy/thirdparty/__init__.py:132
> > +        if not _AUTOINSTALLED_DIR in sys.path:
> > +            sys.path.append(_AUTOINSTALLED_DIR)
> 
> Wow.  Nasty.  You might want to document this hack with a comment.

Sure, can do.
Comment 7 Tony Chang 2012-10-30 15:07:50 PDT
Comment on attachment 171519 [details]
refactor tests a bit

View in context: https://bugs.webkit.org/attachment.cgi?id=171519&action=review

>>> Tools/Scripts/webkitpy/thirdparty/__init__.py:130
>>> +        installed_something = installer.install(url="http://pypi.python.org/packages/source/c/coverage/coverage-3.5.1.tar.gz#md5=410d4c8155a4dab222f2bc51212d4a24", url_subpath="coverage-3.5.1/coverage")
>> 
>> installed_something?  Ah it's a bool?
> 
> Yes. This tracks whether installer.install() actually did work, and is used by autoinstall_everything() in test-webkitpy to see if we need to run the unit tests serial to get around some really weird bugs I hit a few months ago.

I think Eric is saying the naming is confusing.  Maybe call it successfully_installed or has_successfully_installed_coverage.
Comment 8 Dirk Pranke 2012-10-30 15:10:10 PDT
(In reply to comment #7)
> (From update of attachment 171519 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171519&action=review
> 
> >>> Tools/Scripts/webkitpy/thirdparty/__init__.py:130
> >>> +        installed_something = installer.install(url="http://pypi.python.org/packages/source/c/coverage/coverage-3.5.1.tar.gz#md5=410d4c8155a4dab222f2bc51212d4a24", url_subpath="coverage-3.5.1/coverage")
> >> 
> >> installed_something?  Ah it's a bool?
> > 
> > Yes. This tracks whether installer.install() actually did work, and is used by autoinstall_everything() in test-webkitpy to see if we need to run the unit tests serial to get around some really weird bugs I hit a few months ago.
> 
> I think Eric is saying the naming is confusing.  Maybe call it successfully_installed or has_successfully_installed_coverage.

Neither of those names is very good IMO because the routine returns False if the package is already installed (and also, we use the same variable name throughout the file for consistency, so I wouldn't want to necessarily make it specific to the package name).  I could rename it to has_installed_something or did_install_something ... WDYT of those?
Comment 9 Tony Chang 2012-10-30 15:17:13 PDT
Comment on attachment 171519 [details]
refactor tests a bit

View in context: https://bugs.webkit.org/attachment.cgi?id=171519&action=review

>>>>> Tools/Scripts/webkitpy/thirdparty/__init__.py:130
>>>>> +        installed_something = installer.install(url="http://pypi.python.org/packages/source/c/coverage/coverage-3.5.1.tar.gz#md5=410d4c8155a4dab222f2bc51212d4a24", url_subpath="coverage-3.5.1/coverage")
>>>> 
>>>> installed_something?  Ah it's a bool?
>>> 
>>> Yes. This tracks whether installer.install() actually did work, and is used by autoinstall_everything() in test-webkitpy to see if we need to run the unit tests serial to get around some really weird bugs I hit a few months ago.
>> 
>> I think Eric is saying the naming is confusing.  Maybe call it successfully_installed or has_successfully_installed_coverage.
> 
> Neither of those names is very good IMO because the routine returns False if the package is already installed (and also, we use the same variable name throughout the file for consistency, so I wouldn't want to necessarily make it specific to the package name).  I could rename it to has_installed_something or did_install_something ... WDYT of those?

has_installed_something or has_installed_coverage would be fine with me.
Comment 10 Dirk Pranke 2012-10-30 15:40:18 PDT
Created attachment 171536 [details]
update w/ review feedback
Comment 11 Tony Chang 2012-10-30 15:57:35 PDT
Comment on attachment 171536 [details]
update w/ review feedback

View in context: https://bugs.webkit.org/attachment.cgi?id=171536&action=review

> Tools/Scripts/webkitpy/test/main_unittest.py:81
> +        out, err = proc.communicate()

Nit: err -> _ since it's not used.
Comment 12 Dirk Pranke 2012-10-30 16:14:12 PDT
(In reply to comment #11)
> (From update of attachment 171536 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171536&action=review
> 
> > Tools/Scripts/webkitpy/test/main_unittest.py:81
> > +        out, err = proc.communicate()
> 
> Nit: err -> _ since it's not used.

Will do.
Comment 13 Dirk Pranke 2012-10-30 16:17:51 PDT
Committed r132955: <http://trac.webkit.org/changeset/132955>