test-webkitpy: fix import of coverage so that it works in a clean install
Created attachment 171499 [details] Patch
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 on attachment 171499 [details] Patch reworking a bit ... hang on.
Created attachment 171519 [details] refactor tests a bit
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.
(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 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.
(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 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.
Created attachment 171536 [details] update w/ review feedback
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.
(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.
Committed r132955: <http://trac.webkit.org/changeset/132955>