new-run-webkit-tests: refactor config, filesystem info out of port/base
Created attachment 71824 [details] Patch
Note: this patch assumes that bug 48144 and bug 48264 have already landed. Trying to review this patch won't make too much sense if you aren't familiar with the FileSystem and Config objects.
Note that this patch completes nearly all of the refactoring that was in the patch for bug 46776.
Once this lands, I can update the rest of the NRWT test code to use the mock filesystem and mock config objects and most if not all of the NRWT tests will no longer depend on the filesystem. This change by itself speeds test-webkitpy up from ~50seconds to ~14. It should be pretty easy to update the remaining tests and get it back down to under 5 seconds.
Created attachment 72077 [details] Patch
Comment on attachment 72077 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=72077&action=review > WebKitTools/Scripts/webkitpy/common/newstringio.py:43 > +class StringIO(StringIO.StringIO): > + def __enter__(self): > + return self > + > + def __exit__(self, type, value, traceback): > + pass It seems we could implement this a lot like from future import with_statement works. We could have from webkitpy.common import newstringio be required at the top of any file which uses StringIO and with_statement. However, the newstringio.py would just modify the existing StringIO (which I believe is possible) by adding these to StringIO. I think new.instancemethod() is the way one does it: http://docs.python.org/library/new.html newstringio.py would do nothing on 2.6 or later versions of python. I'm not sure if that's cleaner or not. The disadvantage to using a new class name is that it makes it harder to rip out once we do start requiring 2.6 or later. Also, if __enter__ or __exit__ ever change for the better in 2.6 or later versions of python our newstringio ones will be wron, no? I guess an alternate way to do this would be to do exactly what you've done, but only define your class in 2.5 or earlier. Otherwise to just import StringIO into the module as-is. All callers should do from newstringio import StringIO and then they don't need newstringio.StringIO() at the callsites. > WebKitTools/Scripts/webkitpy/common/system/executive_mock.py:31 > +# FIXME: Unify with tool/mocktool.MockExecute. I've since renamed that one, seems we should just move that one here instead. Possibly in a seprate first change. > WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:88 > + self._filesystem = kwargs.get('filesystem', filesystem.FileSystem()) We don't need to use kwargs like this. We can just have the __init__ declaration list the ones we need. Like this: __init__(self, filesystem=None, user=None, executive=None, **kwargs) Make sense?
(In reply to comment #6) > (From update of attachment 72077 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=72077&action=review > > > WebKitTools/Scripts/webkitpy/common/newstringio.py:43 > > +class StringIO(StringIO.StringIO): > > + def __enter__(self): > > + return self > > + > > + def __exit__(self, type, value, traceback): > > + pass > > It seems we could implement this a lot like from future import with_statement works. > > We could have from webkitpy.common import newstringio be required at the top of any file which uses StringIO and with_statement. > > However, the newstringio.py would just modify the existing StringIO (which I believe is possible) by adding these to StringIO. I think new.instancemethod() is the way one does it: http://docs.python.org/library/new.html > > newstringio.py would do nothing on 2.6 or later versions of python. > > I'm not sure if that's cleaner or not. The disadvantage to using a new class name is that it makes it harder to rip out once we do start requiring 2.6 or later. > Monkey-patching the existing class feels like more trouble than it is worth. It's just a search-and-replace to do the upgrade when the time comes (like moving from simplejson to json). > Also, if __enter__ or __exit__ ever change for the better in 2.6 or later versions of python our newstringio ones will be wron, no? > The code works under 2.6 (and 2.7) today. I suppose it's possible that it could change in the future, but I'm not sure I understand your concern? > I guess an alternate way to do this would be to do exactly what you've done, but only define your class in 2.5 or earlier. Otherwise to just import StringIO into the module as-is. All callers should do from newstringio import StringIO and then they don't need newstringio.StringIO() at the callsites. > Yes, we could do this as well, but it again seems like more trouble than it's worth. > > WebKitTools/Scripts/webkitpy/common/system/executive_mock.py:31 > > +# FIXME: Unify with tool/mocktool.MockExecute. > > I've since renamed that one, seems we should just move that one here instead. Possibly in a seprate first change. > The two classes are not identical. It will take some work to figure out what we want the functionality to be and update the tests accordingly, so I don't want to link that to this. > > WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:88 > > + self._filesystem = kwargs.get('filesystem', filesystem.FileSystem()) > > We don't need to use kwargs like this. We can just have the __init__ declaration list the ones we need. Like this: > > __init__(self, filesystem=None, user=None, executive=None, **kwargs) > > Make sense? We could do that, but we'd still need: if filesystem is None: filesystem = FileSystem() that doesn't seem like much of an improvement. (note that you can't use filesystem.FileSystem() as the default value, since then all Port objects would share a single FileSystem object reference). And, of course, using filesystem as the variable name would shadow the module, so we'd have to use a different name for the module (or import just the class name directly, as above, but I'm not a big fan of importing non-module names directly; I prefer to make the namespace explicit).
(In reply to comment #7) > We could do that, but we'd still need: > > if filesystem is None: > filesystem = FileSystem() > > that doesn't seem like much of an improvement. filesystem = filesystem or FileSystem() might be. Grabbing things manually out of kwargs just makes me cringe a bit. The interpreter doesn't help you at all, for typos, etc. > (note that you can't use filesystem.FileSystem() as the default value, since then all Port objects would share a single FileSystem object reference). I agree completely! I'm glad you're aware of that python got-cha. > And, of course, using filesystem as the variable name would shadow the module, so we'd have to use a different name for the module (or import just the class name directly, as above, but I'm not a big fan of importing non-module names directly; I prefer to make the namespace explicit). I see. Interesting point about shadowing. We seem to have mixed feelings in webkitpy about how to do imports. I'm not sure we ever came up with the one-true way. We started importing class names directly, but we've slowly moved towards importing modules instead. I'm not sure it really matters either way. For basic webkitpy support classes it seems to make sense to import them directly. For non-webkitpy classes we should probably import the module instead. Not sure.
(In reply to comment #8) > (In reply to comment #7) > > We could do that, but we'd still need: > > > > if filesystem is None: > > filesystem = FileSystem() > > > > that doesn't seem like much of an improvement. > > filesystem = filesystem or FileSystem() > might be. Grabbing things manually out of kwargs just makes me cringe a bit. The interpreter doesn't help you at all, for typos, etc. > Understood. I'm not sure what the best way to work around the module name renaming or stutter is. I'm inclined to leave things the way we are until we can decide on a consistent style guideline. > We seem to have mixed feelings in webkitpy about how to do imports. I'm not sure we ever came up with the one-true way. We started importing class names directly, but we've slowly moved towards importing modules instead. I'm not sure it really matters either way. For basic webkitpy support classes it seems to make sense to import them directly. For non-webkitpy classes we should probably import the module instead. Not sure. Yeah. I think you and Adam (and maybe Chris?) were more of the import class names directly school, whereas Ojan, Tony and myself have always (?) done modules. I think modules is the general pythonic way to do it (and I think there are things in the Google Python style guide to that effect). One thing that might improve stuff is if we made better use of packages and __init__.py, so we could have from webkitpy import common.system filesystem = system.FileSystem() or some such thing.
Dirk asked me to review this patch, but it seems like he's already got a dialog going with Eric.
(In reply to comment #9) > > We seem to have mixed feelings in webkitpy about how to do imports. I'm not sure we ever came up with the one-true way. We started importing class names directly, but we've slowly moved towards importing modules instead. I'm not sure it really matters either way. For basic webkitpy support classes it seems to make sense to import them directly. For non-webkitpy classes we should probably import the module instead. Not sure. > > Yeah. I think you and Adam (and maybe Chris?) were more of the import class names directly school, whereas Ojan, Tony and myself have always (?) done modules. I think modules is the general pythonic way to do it (and I think there are things in the Google Python style guide to that effect). I'm OK with moving to modules if that's the python way. I assume that would look something like this? from webkitpy.common.system import ospath ospath.relpath() > One thing that might improve stuff is if we made better use of packages and __init__.py, so we could have > > from webkitpy import common.system > > filesystem = system.FileSystem() > > or some such thing. Yes. We want to break bugzilla.py out into is own submodule with a nice __init__ like that. We named checkout/api.py the way we did because we didn't understand how to use checkout/__init__.py correctly. (I've since added an import into that __init__, but we should rename api.py to checkout.py now).
(In reply to comment #11) > (In reply to comment #9) > > > We seem to have mixed feelings in webkitpy about how to do imports. I'm not sure we ever came up with the one-true way. We started importing class names directly, but we've slowly moved towards importing modules instead. I'm not sure it really matters either way. For basic webkitpy support classes it seems to make sense to import them directly. For non-webkitpy classes we should probably import the module instead. Not sure. > > > > Yeah. I think you and Adam (and maybe Chris?) were more of the import class names directly school, whereas Ojan, Tony and myself have always (?) done modules. I think modules is the general pythonic way to do it (and I think there are things in the Google Python style guide to that effect). > > I'm OK with moving to modules if that's the python way. > > I assume that would look something like this? > > from webkitpy.common.system import ospath > > ospath.relpath() > Yes. See http://code.google.com/p/soc/wiki/PythonStyleGuide#Module_and_package_imports for more.
Created attachment 73034 [details] Patch
(In reply to comment #8) > > And, of course, using filesystem as the variable name would shadow the module, so we'd have to use a different name for the module (or import just the class name directly, as above, but I'm not a big fan of importing non-module names directly; I prefer to make the namespace explicit). > > I see. Interesting point about shadowing. > Turns out the Google Python style guide had a good suggestion for how to work around the 'parameter has same name as module' problem: """ Some module names might be the same as common local variable names. In those cases, to avoid confusion, it sometimes makes sense to only import the containing package, and then explicitly import the module. The result looks something like this: from soc import models import soc.models.user ... user = models.user.User() It probably best to avoid module names that lead to this confusion in the first place, but sometimes using the obvious module name (such as in the soc.models package example above) will result in the need for workarounds like the one above. """ I'll upload a patch with this for comparison.
(In reply to comment #6) > (From update of attachment 72077 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=72077&action=review > > > WebKitTools/Scripts/webkitpy/common/newstringio.py:43 > > +class StringIO(StringIO.StringIO): > > + def __enter__(self): > > + return self > > + > > + def __exit__(self, type, value, traceback): > > + pass > > It seems we could implement this a lot like from future import with_statement works. > > We could have from webkitpy.common import newstringio be required at the top of any file which uses StringIO and with_statement. > > However, the newstringio.py would just modify the existing StringIO (which I believe is possible) by adding these to StringIO. I think new.instancemethod() is the way one does it: http://docs.python.org/library/new.html > > newstringio.py would do nothing on 2.6 or later versions of python. > Turns out that StringIO still doesn't work with "with" on either 2.6 or 2.7. I'm not sure why I thought it did. Kind of makes the whole thing moot, so I've removed the FIXME and we can leave the code as is.
Comment on attachment 73034 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73034&action=review Thank you for the patch. Looks good. I appreciate you breaking this up into smaller pieces. Smaller still would be even better, but this was totally manageable. > WebKitTools/Scripts/webkitpy/common/system/filesystem_mock.py:55 > + def isdir(self, path): > + # FIXME: Implement :) > + raise NotImplementedError Seems we should just leave thse undefined, and we'd get the same behavior for 4 fewer lines in teh file. > WebKitTools/Scripts/webkitpy/common/system/filesystem_mock.py:62 > + def listdir(self, path): > + # FIXME: Implement :) > + raise NotImplementedError Same here. > WebKitTools/Scripts/webkitpy/common/system/filesystem_mock.py:74 > + raise OSError() Do we need to have code=EEXISTS or whatever it is? > WebKitTools/Scripts/webkitpy/layout_tests/port/config.py:87 > + ed)turns whether build was successful or is up-to-date. typo. > WebKitTools/Scripts/webkitpy/layout_tests/port/config_unittest.py:46 > + def make_config(self, output='', files={}, exit_code=0): > + e = executive_mock.MockExecutive(output=output, exit_code=exit_code) > + fs = filesystem_mock.MockFileSystem(files) > + return config.Config(e, fs) Thank you for the code sharing. > WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:356 > def _build_path(self, *comps): > - if not self._cached_build_root: > - self._cached_build_root = self._webkit_build_directory([ > - "--configuration", > - self.flag_from_configuration(self.get_option('configuration')), > - ]) > - return os.path.join(self._cached_build_root, *comps) > + return self._filesystem.join(self._config.build_directory( > + self.get_option('configuration')), *comps) Sigh. I'm about to post a patch to re-write this as part of bug 49051. I'll deal with the conflicts. :)
(In reply to comment #16) > (From update of attachment 73034 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=73034&action=review > > Thank you for the patch. Looks good. I appreciate you breaking this up into smaller pieces. Smaller still would be even better, but this was totally manageable. > > > WebKitTools/Scripts/webkitpy/common/system/filesystem_mock.py:55 > > + def isdir(self, path): > > + # FIXME: Implement :) > > + raise NotImplementedError > > Seems we should just leave thse undefined, and we'd get the same behavior for 4 fewer lines in teh file. > Yeah, but then I wouldn't have the reminder :) > > WebKitTools/Scripts/webkitpy/common/system/filesystem_mock.py:74 > > + raise OSError() > > Do we need to have code=EEXISTS or whatever it is? Good catch. Looks like this should actually be an IOError()
Created attachment 73183 [details] Patch merged to tip of tree ... this doesn't contain the memoize descriptors, because we cache the values already.
Comment on attachment 73183 [details] Patch merged to tip of tree ... this doesn't contain the memoize descriptors, because we cache the values already. Clearing flags on attachment: 73183 Committed r71474: <http://trac.webkit.org/changeset/71474>
All reviewed patches have been landed. Closing bug.
Reverted r71474 for reason: breaks chromium webkit tests Committed r71491: <http://trac.webkit.org/changeset/71491>
Okay, I fixed this again, in r71580.