Bug 100780 – test-webkitpy: fix import of coverage so that it works in a clean install

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

[Dirk Pranke]

test-webkitpy: fix import of coverage so that it works in a clean install

[Dirk Pranke]

Created attachment 171499 [details]
Patch

[Dirk Pranke]

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?

[Dirk Pranke]

Comment on attachment 171499 [details]
Patch

reworking a bit ... hang on.

[Dirk Pranke]

Created attachment 171519 [details]
refactor tests a bit

[Eric Seidel (no email)]

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.

[Dirk Pranke]

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

[Tony Chang]

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.

[Dirk Pranke]

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

[Tony Chang]

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.

[Dirk Pranke]

Created attachment 171536 [details]
update w/ review feedback

[Tony Chang]

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.

[Dirk Pranke]

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

[Dirk Pranke]

Committed r132955: <http://trac.webkit.org/changeset/132955>