Bug 63501

+dpranke. (Dirk, let me know if you'd rather not get this bugmail.)

Created attachment 98842 [details] Patch

Created attachment 98847 [details] Patch

It's now possible to successfully the tests with WebKitTestRunner with this patch. About 20 or so fail for unknown reasons, so we still have some debugging to do.

Comment on attachment 98847 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98847&action=review This is some sort of karmic payback for all those patches I've submitted that had fixes for umpteen different things in a single patch, isn't it? :) > Tools/Scripts/webkitpy/common/system/filesystem.py:64 > + I'm not sure this really belongs in filesystem, but then I'm not sure of a better place for it, either. Maybe add a FIXME to consider put it someplace else if there is something appropriate? > Tools/Scripts/webkitpy/layout_tests/port/base.py:546 > + # FIXME: This should be replaced by "return "%s-%s" (self.port_name(), self.version())" Sadly, %s-%s doesn't really cover it, since the fully-qualified name can also include the architecture and the graphics type, but there isn't a 1:1 mapping between classes and any particular combination of those attributes that is true across all of the ports (and I'm not sure there should be, because that might cause an explosion of subclasses). I would just delete the FIXME. > Tools/Scripts/webkitpy/layout_tests/port/base.py:550 > + # FIXME: Seems this is only used for MockDRT and should be removed. Adding a comment or a docstring that this is used only for MockDRT is great, but I'm not sure why you think this should be removed. Are you suggesting that MockDRT doesn't need this, or that MockDRT shouldn't exist, or simply that there's a better way to provide MockDRT with this info? > Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:70 > + # FIXME: version, name, etc. should be passed separately and we should not have to parse port_name. This is a noble goal, but I'm not sure how you plan to accomplish it. Specifying a substring of the fully-qualified name is terribly useful from the users' point of view and can be flat-out necessary if you don't know the right combinations to use. Given that, if you move the logic out of the class (which is what I'm assuming you're suggesting), I don't know where else you would put it (factory.py being the obvious candidate) that wouldn't simply create a mirror of the class hierarchy or one terribly complex series of tests and branches. It's an interesting topic, and I definitely agree that the mechanisms that exist now are crufty. I look forward to your ideas. > Tools/Scripts/webkitpy/layout_tests/port/mac.py:54 > + # FIXME: There is no mac-future directory, I don't understand why this code is here. I had a long discussion with someone (mark rowe?) in #webkit that led me to create this in order to clear up some mutual confusion, but at least at the time there seemed to be a real need for something like this in order to handle pre-released versions of software that might exist inside Apple (like your comment about Lion, above). I will attempt to dig up the notes I have on it and/or reconstruct the thinking tomorrow and send it to you. It is unfortunate that I didn't comment this better. I think it had to do with people inside of Apple thinking of "platform/mac" as representing the "future" version of the o/s, while people outside of Apple thinking of "platform/mac" as the "currently shipping" version. Since just saying "--platform mac" is equivalent to "--platform mac-<version I'm running on>", we needed some way to indicate that there might be unreleased, unnamed versions that could still be referred to. At least, I think that was the conclusion. I will try to dig up more tomorrow. > Tools/Scripts/webkitpy/layout_tests/port/mac.py:70 > + # FIXME: This isn't quite right for wk2, it should include mac-VERSION as well. I never did come up with a good strategy for handling the wk2 versus wk1 versions of the ports. My half-formed thoughts were that it probably needed to be treated as a different configuration attribute, like chromium has for architecture and cpu/gpu type. > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:79 > + return self.wk2_port_name() if self.get_option('webkit_test_runner') else self.port_name() I've been waiting through the whole patch to see what you were going to use port_name() for ... if this is the thinking, then the chromium implementations of port_name should probably be returning "chromium", not "chromium-<os>". Are you using port_name() for other things as well, that I may have missed? It's not clear to me that wk2- variants and wk1-variants should have separate expectations files. In Chrome, we've found that multiple expectations files are best avoided, but maybe it'll be better if the files are nearly always empty? Also, if I specify mac-wk2 as the port name, does that automatically set webkit_test_runner to True (i.e., as if we had also passed --2)? A general design feature of NRWT has been that you should be able to use the --platform / port name to uniquely (in a single argument) capture all of the attributes you would need (although debug/release doesn't use this feature at the moment, there's no reason it couldn't). This has been very useful in the chromium bots, among other places, where you don't have to figure out how to translate a single bot config into a potential set of different arguments.

(In reply to comment #5) > > Tools/Scripts/webkitpy/common/system/filesystem.py:64 > > I'm not sure this really belongs in filesystem, but then I'm not sure of a better place for it, either. Maybe add a FIXME to consider put it someplace else if there is something appropriate? I figure filesystem is a good place to ask where things are on disk. But I'm happy to move it elsewhere. > > Tools/Scripts/webkitpy/layout_tests/port/base.py:546 > > + # FIXME: This should be replaced by "return "%s-%s" (self.port_name(), self.version())" > > Sadly, %s-%s doesn't really cover it, since the fully-qualified name can also include the architecture and the graphics type, but there isn't a 1:1 mapping between classes and any particular combination of those attributes that is true across all of the ports (and I'm not sure there should be, because that might cause an explosion of subclasses). I would just delete the FIXME. Yeah, so this is the problem. We don't know what "name()" actually means (or all the ways its used). But we do know what each of the components mean (I think). In webkit we use the following two formats (as far as I know): PORT (mac, win, qt) PORT-VERSION (mac-leopard, win-xp, etc.) Chromium has a bunch more though: PORT-OS (chromium-mac) PORT-OS-VERSION (chromium-win-xp) PORT-OS-ARCH (chromium-linux-x86_64) PORT-GPU-OS (chromium-gpu-linux) PORT-GPU-OS-ARCH (chromium-gpu-linux-x86_64) I think the parsing logic should be separate from the Port storage logic. Port needs to know how to construct various names from those components (for layout test directories, bot names, etc.) but parsing arbitrary user input should be the responsibility of some other class IMO. > > Tools/Scripts/webkitpy/layout_tests/port/base.py:550 > > + # FIXME: Seems this is only used for MockDRT and should be removed. > > Adding a comment or a docstring that this is used only for MockDRT is great, but I'm not sure why you think this should be removed. Are you suggesting that MockDRT doesn't need this, or that MockDRT shouldn't exist, or simply that there's a better way to provide MockDRT with this info? Mocks very rarely have to change the original classes. There is no reason why MockDRT can't just add that method itself to some Port subclass. > > Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:70 > > + # FIXME: version, name, etc. should be passed separately and we should not have to parse port_name. > > This is a noble goal, but I'm not sure how you plan to accomplish it. Specifying a substring of the fully-qualified name is terribly useful from the users' point of view and can be flat-out necessary if you don't know the right combinations to use. Given that, if you move the logic out of the class (which is what I'm assuming you're suggesting), I don't know where else you would put it (factory.py being the obvious candidate) that wouldn't simply create a mirror of the class hierarchy or one terribly complex series of tests and branches. I dont' think we'd use a mirror of the class hierarchy. I think we'd have some sort of factory class (like we already do). It's not clear to me where all these combined strings come from. Since we have various types of strings we could have separate parsers for each. It's unclear what the generic "name" is supposed to represent or the port_name argument to the constructor. The fact that we currently use port_name in the constructor in differrent ways for different sections of code is where I think the confusion stems. > > Tools/Scripts/webkitpy/layout_tests/port/mac.py:54 > > + # FIXME: There is no mac-future directory, I don't understand why this code is here. > > I had a long discussion with someone (mark rowe?) in #webkit that led me to create this in order to clear up some mutual confusion, but at least at the time there seemed to be a real need for something like this in order to handle pre-released versions of software that might exist inside Apple (like your comment about Lion, above). I suspect it was a interesting idea from Apple, but it seems they never went forward with a "future" port. We can eventually remove it. Not urgent. > I will attempt to dig up the notes I have on it and/or reconstruct the thinking tomorrow and send it to you. It is unfortunate that I didn't comment this better. I think it had to do with people inside of Apple thinking of "platform/mac" as representing the "future" version of the o/s, while people outside of Apple thinking of "platform/mac" as the "currently shipping" version. Since just saying "--platform mac" is equivalent to "--platform mac-<version I'm running on>", we needed some way to indicate that there might be unreleased, unnamed versions that could still be referred to. Yes. My understanding of the thinking is that platform/mac always represents the future. platform/mac-VERSION represents any specific shipping OS. > > Tools/Scripts/webkitpy/layout_tests/port/mac.py:70 > > + # FIXME: This isn't quite right for wk2, it should include mac-VERSION as well. > > I never did come up with a good strategy for handling the wk2 versus wk1 versions of the ports. My half-formed thoughts were that it probably needed to be treated as a different configuration attribute, like chromium has for architecture and cpu/gpu type. Handeling it as an option has worked OK. I'm not sure I understand the ARCH or GPU flavors of the chomium ports. Like the "name" confusion, it's not clear what name variants want to include the ARCH vs. GPU sub-types and which don't. Bot names likely do, but do fallback paths? Who knows. Part of the confusion is what I mentioned above that webkit only really has two flavors of names, and Chromium has 4 factorial. :) > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:79 > > + return self.wk2_port_name() if self.get_option('webkit_test_runner') else self.port_name() > > I've been waiting through the whole patch to see what you were going to use port_name() for ... if this is the thinking, then the chromium implementations of port_name should probably be returning "chromium", not "chromium-<os>". Are you using port_name() for other things as well, that I may have missed? Oh, this was an easy correction. Yes, I could change all chromium ports to parse out "chroimum" as port_name() instead, that's probably better. In this specific case, there are 5 test_expectation files in the repository: ./chromium/test_expectations.txt ./gtk/test_expectations.txt ./mac/test_expectations.txt ./qt/test_expectations.txt ./win/test_expectations.txt and none of them use the "name()" version of the port name, so I changed them to use port_name() which matched expectations at least for the 4 webkit-ports. This is more examples of confusion on what the various "name"s mean. > It's not clear to me that wk2- variants and wk1-variants should have separate expectations files. In Chrome, we've found that multiple expectations files are best avoided, but maybe it'll be better if the files are nearly always empty? Mostly I wanted to stop reading in the mac/test_expectations.txt for wk2, since wk2 skips several tests which are listed in mac/test_expectations.txt and thus test_expectations got mad for having duplicate entries. Initially I just disabled test_expectations for wk2, but then decided I should leave the option open, and changed it to look for test_expectations in PORTNAME-wk2 instead. > Also, if I specify mac-wk2 as the port name, does that automatically set webkit_test_runner to True (i.e., as if we had also passed --2)? No. > A general design feature of NRWT has been that you should be able to use the --platform / port name to uniquely (in a single argument) capture all of the attributes you would need (although debug/release doesn't use this feature at the moment, there's no reason it couldn't). This has been very useful in the chromium bots, among other places, where you don't have to figure out how to translate a single bot config into a potential set of different arguments. I guess I wasn't aware of that design feature. I'm not sure I agree with it, given the current architecture. Here again a factory/parser would be useful to make explicit what are acceptable user input, and separate that from how the actual Port objects store configuration parameters and synthesize the needed names for on-disk storage and bot lookup.

(In reply to comment #6) > > > Tools/Scripts/webkitpy/layout_tests/port/mac.py:54 > > > + # FIXME: There is no mac-future directory, I don't understand why this code is here. > > > > I had a long discussion with someone (mark rowe?) in #webkit that led me to create this in order to clear up some mutual confusion, but at least at the time there seemed to be a real need for something like this in order to handle pre-released versions of software that might exist inside Apple (like your comment about Lion, above). > > I suspect it was a interesting idea from Apple, but it seems they never went forward with a "future" port. We can eventually remove it. Not urgent. > Okay, I dug up the notes on this, and you can look at bug 56730 for more context. The gist of it is that there needs to be a stable way to refer to the future or currently-in-development version of the o/s and the currently shipping version. Note that this is not necessarily the same thing as the actual names of the baseline directories. E.g., for people outside of Apple, as of this writing if you are running on 10.6 / Snow Leopard, you should still put new baselines into platform/mac. However, inside Apple, where you might be able to distinguish between tests that will pass on 10.7/Lion and fail on SL, you need to be able to say whether you want the Lion behavior or the SL behavior. We agreed to use "-future" as the version indicator for this. Note that the tools do not really fully support this concept yet. Also, there is nothing Apple/Mac-specific about this. We could just as easily reuse this concept for Windows 8, Ubuntu Natty, etc. So, we might want to refine this further, but I don't think we can remove it.

Yeah, looking at the platform directory, chromium has created a ton of new names, none of which are supported by old-run-webkit-tests or any other webkit port: Chromium names: chromium chromium-gpu chromium-gpu-linux chromium-gpu-mac chromium-gpu-win chromium-linux chromium-linux-x86 chromium-mac chromium-mac-leopard chromium-win chromium-win-vista chromium-win-xp google-chrome-linux32 google-chrome-linux64 Everybody else: android android-v8 gtk mac mac-leopard mac-snowleopard mac-wk2 qt qt-4.8 qt-arm qt-linux qt-mac qt-win qt-wk2 win win-7sp0 win-wk2 win-xp The only names in that list which don't follow PORT/PORT-VERSION are Qt, which adds PORT-OS (qt-win, qt-mac) and PORT-ARCH (qt-arm). (I guess andriod vs. andriod-v8 also doesn't follow, but the android port should be deleted at this point, so I'm not sure its' a very useful example.) In any case in order to maintain any level of sanity, we need to decide what the various possible name uses are (fallback directories, test-expecation locations, skipped file locations, and bot names are the only things which come to mind) and create functions which can produce those given the right components. We also then need to understand what set of names users are expected to be allowed to provide, and then design a parser for those.

(In reply to comment #6) > (In reply to comment #5) > > > > Tools/Scripts/webkitpy/common/system/filesystem.py:64 > > > > I'm not sure this really belongs in filesystem, but then I'm not sure of a better place for it, either. Maybe add a FIXME to consider put it someplace else if there is something appropriate? > > I figure filesystem is a good place to ask where things are on disk. But I'm happy to move it elsewhere. > Mostly, it feels weird to me to have filesystem.py looking at sys.modules. But, like I said, I couldn't think of a better place for it, either.

(In reply to comment #6) > > > Tools/Scripts/webkitpy/layout_tests/port/base.py:550 > > > + # FIXME: Seems this is only used for MockDRT and should be removed. > > > > Adding a comment or a docstring that this is used only for MockDRT is great, but I'm not sure why you think this should be removed. Are you suggesting that MockDRT doesn't need this, or that MockDRT shouldn't exist, or simply that there's a better way to provide MockDRT with this info? > > Mocks very rarely have to change the original classes. There is no reason why MockDRT can't just add that method itself to some Port subclass. Okay, I had to go back and remember why I added this in the first place ... Generally speaking, calling code should not be aware of the difference between the "real" port and the "mock" port (this is true, to a lesser extent, of the dryrun classes as well, which could probably be removed at this point). The only *practical* difference between the real port and the mock port is that the mock ports stubs out the servers and changes the command line used to spawn off DRTs. This allows us to get virtually 100% coverage of the port-specific code. For that reason, port.name() needs to return the value that the original/un-mocked port would return. However, there is one place where we actually need to know the difference, which is when we spawn off subprocesses to run the workers, in layout_package/manager_worker_broker.py. Here it's important that we can retrieve the argument we need to pass back to --platform in the subprocess in order to re-create the MockDRT port object instead of getting the real port object by accident. It may be that real_name() is a bad name for this method, and we almost certainly need better comments or docstrings around why this exists (since it took all this just for me to remember). But, I'm not sure if there's a better way to actually get the functionality while minimizing the difference in code execution between real ports and mock ports.

(In reply to comment #8) > Yeah, looking at the platform directory, chromium has created a ton of new names, none of which are supported by old-run-webkit-tests or any other webkit port: Yes, Chromium is certainly much more complicated, and that drove a lot of the complexity of this design. Rather than trying to address comments one-by-one (which I did in the bugzilla comment that got stepped on), I'll try to approach things from a different angle. NRWT needs to support multiple different ports that themselves have multiple different dimensions of variability. These differences manifest mostly in the the baseline search path / fallback path, and in the sets of modifiers that are supported in the test_expectations files, but obviously there are some aspects of variance that show up as differences in code in the different class files. Amongst all of the ports we have so far, we have: debug/release operating system operating system version architecture (x86/x86_64/arm) cpu/gpu webkit 1 / webkit 2 "implementation family" (chromium / apple / qt / gtk) obviously, not all ports use all of these dimensions. The TestConfig object only captures some of them right now, but it could be expanded to include the others. Most of these dimensions do show up in the modifiers accepted in the test_expectations.txt files (in fact, the accepted values are determined from the TestConfig object). In the code right now, choosing values on some of these axes is done by specific command line flags (notably --debug/--release) and many of the other axes are selected based on the values passed to the --platform argument. My long-term plan had been to have arguments for individual axes as well as being able to get to any point in the space from a single argument via the platform flag. Ideally, we should be able to enumerate all of the different combinations of all of the different values programmatically, so that --lint-test-files works. In addition, we probably don't want all of these possible combinations to try and co-exist in a single test_expectations.txt file (in particular, Chromium should probably stand alone since it is so much bigger). However, it might make sense for all of other ports to share a single test expectations file, or it might make sense to have one per "implementation family" (apple/chromium/qt/gtk/android/etc.). Whether it makes sense to have a separate set of classes to handle these sets of command line args and try to convert between args, --platform values, and Port objects, or to just clean up the existing classes, I don't know. I'm entirely open to suggestions here. Hopefully this makes a little more sense. I think this is actually the most important "big picture" difference between ORWT and NRWT to grasp and make sure you understand, otherwise a lot of my code structure may not make that much sense to you.

Created attachment 98992 [details] Slightly smaller patch

There are no changes to the patch, just ripped some stuff out.

Comment on attachment 98992 [details] Slightly smaller patch The patch generally looks pretty good. My comments mostly relate to various FIXMEs, docstrings, function names, and other comments ... View in context: https://bugs.webkit.org/attachment.cgi?id=98992&action=review > Tools/Scripts/webkitpy/layout_tests/port/base.py:541 > + """The base name of the port, not including version information.""" We should probably provide examples here ... should it be "apple" or "mac"? "chromium" or "chromium-mac", etc.? How is this different from "name"? > Tools/Scripts/webkitpy/layout_tests/port/base.py:546 > + # FIXME: This should be replaced by "return "%s-%s" (self.port_name(), self.version())" As noted in other comments, this FIXME is wrong and the doc string is still simplistic. I'd replace this by something along the lines of """Returns a name that uniquely identifies this particular type of port (e.g., "mac-snowleopard" or "chromium-gpu-linux-x86_x64" and can be passed back to factory.get() or the constructor"". > Tools/Scripts/webkitpy/layout_tests/port/base.py:551 > + """Returns the actual name of the port, not the delegate's.""" """Returns the name of the port as passed to the --platform command line argument. This can be different from port.name() for ports like the MockDRT port.""" > Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:71 > + self._parse_port_name(port_name) I'm always a little uncomfortable with calling other routines from inside constructors. Maybe we should add a comment to _parse_port_name() that this may be called from inside a constructor and the object may not be fully initialized? Alternatively, we could make this a static member function, and pass a tuple back or something like that ... > Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:70 > + # FIXME: version, name, etc. should be passed separately and we should not have to parse port_name. I don't understand this comment. Why would a routine called _parse_port_name() not have to parse the port name? Obviously you're trying to get at something else ... > Tools/Scripts/webkitpy/layout_tests/port/config.py:121 > + config_module_path = self._filesystem.path_to_module(self.__module__) I suggest you change filesystem.path_to_module() to return an abspath and be clear about it. > Tools/Scripts/webkitpy/layout_tests/port/mac.py:54 > + # FIXME: There is no mac-future directory, I don't understand why this code is here. Remove this per the comments in the bug? There will never be a mac-future directory, but port names do not necessarily map directly to baseline dirs. > Tools/Scripts/webkitpy/layout_tests/port/mac.py:71 > 'wk2': ['mac-wk2', 'mac'], As discussed, "-wk2" isn't really a version, and probably should represent a different axis of the test configuration, just like -gpu- and -x86 do in chromium. I would update the FIXME accordingly. > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:69 > + # port which requires platform-specfic results. We should replace this with a smarter Typo: "specific", not "specfic". Also, do you mean os-version-specific results, or os-specific results? I'm not sure what "platform" refers to in this context. > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:296 > + def wk2_port_name(self): Is this supposed to be a public function? Seems like it should only be _wk2_port_name() instead? > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:330 > + def _skipped_list_as_expectations_file(self): Nit: Technically, you're returning a string here ... I'd call this "_skipped_list_as_expectations()" instead. > Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py:49 > + WebKitPort.__init__(self, port_name="test", executive=executive, filesystem=filesystem, user=MockUser(), **kwargs) Given that "test" refers to the "test" port in port/test.py, this might be confusing. "webkit-test" or "test-webkit" maybe?

Created attachment 99159 [details] further reduced patch, still no functional changes

(In reply to comment #14) > (From update of attachment 98992 [details]) > > Tools/Scripts/webkitpy/layout_tests/port/base.py:546 > > + # FIXME: This should be replaced by "return "%s-%s" (self.port_name(), self.version())" > > As noted in other comments, this FIXME is wrong and the doc string is still simplistic. I'd replace this by something along the lines of > > """Returns a name that uniquely identifies this particular type of port (e.g., "mac-snowleopard" or "chromium-gpu-linux-x86_x64" and can be passed back to factory.get() or the constructor"". I think we want to move to a model where we store all the component pieces on the Port object, and generate the names we need on demand. That's what the comment is trying (and failing) to express. The current model is that we store whatever we were passed in as self._name and parse it for any data we need. I believe that approach to be sub-optimal. I could imagine an approach where the factory does all the parsing by spliting the string on "-" and matching each part to known lists (which the Port objects themselves could contain). Or passing the split string to the port object, having looked it up based on the port_name string it found in the split. For now I think I should just hardcode the port_names on the various ports. To move away from the parsing bit. They don't change. It doesn't make sense for hte Mac port to ever have a port name other than "mac", likewise, for Chromium ports to have a name other than "chromium", or? These various name() accessors can later be custom tuned to return whatever we'd like them to. > > Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:71 > > + self._parse_port_name(port_name) > > I'm always a little uncomfortable with calling other routines from inside constructors. Maybe we should add a comment to _parse_port_name() that this may be called from inside a constructor and the object may not be fully initialized? Alternatively, we could make this a static member function, and pass a tuple back or something like that ... That woudl be OK. I think long-term I'd like it better if the factory did all of the string parsing and called setters on the port object. Something like this: def get(self, port_string) port_components = port_string.split("-") port_name_array = port_components.intersection(port_objects.keys()) # Should only find one entry if not port_name_array: raise "Invalid port name: %s" % port_string port = port_objects.get(port_name_array[0]) # Now either the factory could parse out the remaining pieces of the port components by comparing them to known lists (which it could generate from the list of all ports, or just have hardcoded), or it could pass the port_components off to the Port() constructor and it could call the setters. But in either case it looks something like this: for component in port_components: if component in known_architectures: port.set_architecture(component) elif component in known_versions: port.set_version(component) elif component in known_graphics: port.set_graphics(graphics) else: raise "Invalid port name: %s" % port_string Then of course the various names look like this: def layout_results_name(self) return "%s-%s" % (self._port_name, self._version) or for Chromium maybe something like this: def layout_results_name(self) name = self._port_name if self._graphics != "cpu": name += "-" + self._graphics name += "-" + self._operating_system if self._version: name += "-" + self._version if self._architecture: name += "-" + self._architecture In any case, that would be one approach which would result in consistent storage and connonicalized naming and parsing. I'm not sure it's the best system, but I believe it to be better than today, where it's unclear what _name means. > > Tools/Scripts/webkitpy/layout_tests/port/mac.py:71 > > 'wk2': ['mac-wk2', 'mac'], > > As discussed, "-wk2" isn't really a version, and probably should represent a different axis of the test configuration, just like -gpu- and -x86 do in chromium. I would update the FIXME accordingly. -wk2 can't be correctly handled by that dictionary lookup since its fallback depends on what version of the OS its running on. I'm also still not quite comfortable with this multi-axis configuration. I can see how it's useful to have a single cannonical string to represent a port, but I think we're currently over-using that string and not using the component pieces enough. > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:296 > > + def wk2_port_name(self): > > Is this supposed to be a public function? Seems like it should only be _wk2_port_name() instead? I figured it might get re-used,b ut I can make it private until then. > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:330 > > + def _skipped_list_as_expectations_file(self): > > Nit: Technically, you're returning a string here ... I'd call this "_skipped_list_as_expectations()" instead. Done. > > Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py:49 > > + WebKitPort.__init__(self, port_name="test", executive=executive, filesystem=filesystem, user=MockUser(), **kwargs) > > Given that "test" refers to the "test" port in port/test.py, this might be confusing. "webkit-test" or "test-webkit" maybe? Changed to testwebkitport to match the class name.

Created attachment 99179 [details] Patch

The patch is now bigger again (due to more cleanup as I tweaking to remove the _parse_port_name stuff). But it's much simpler. We're now just adding a cls.port_name value which contains the static information of the generic name of the port. "mac", "chromium", "win", etc. This value is now used for wk2-name computation as well as Skipped list search paths.

Comment on attachment 99179 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99179&action=review I mean, ideally you'd separate out the cleanup from the real change, but ok. > Tools/Scripts/webkitpy/layout_tests/port/gtk.py:71 > def check_build(self, needs_http): > - if not self._check_driver(): > - return False > - return True > + return self._check_driver() Wow

Comment on attachment 99179 [details] Patch Clearing flags on attachment: 99179 Committed r90070: <http://trac.webkit.org/changeset/90070>

All reviewed patches have been landed. Closing bug.

Comment on attachment 99179 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99179&action=review Looks okay to me as well; just a few more nits ... > Tools/Scripts/webkitpy/layout_tests/port/base.py:114 > + self._http_lock = None # FIXME: Why does this live on the port object? Where else would it live, given that locking and unlocking the servers are methods on the port object? Are you asking why isn't a lock just returned from the port object? > Tools/Scripts/webkitpy/layout_tests/port/base.py:136 > + self._test_configuration = None # FIXME: This does not belong on the real object. Yes it does. This is not something for testing, this is the TestConfig object returned from test_configuration() and used by the test_expectations classes. > Tools/Scripts/webkitpy/layout_tests/port/base.py:451 > + """Returns the list of top-level test directories.""" Hmm. I found that comment helpful; oh well. > Tools/Scripts/webkitpy/layout_tests/port/gtk.py:-119 > - Why did you delete this? Because None is implicitly returned? That may be true, but I think it's bad style to rely on that, and better to be explicit. > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:52 > + self._cached_apache_path = None # FIXME: This class should use @memoized instead. Meh :) > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:73 > + return self._wk2_port_name() if self.get_option('webkit_test_runner') else self.port_name Seems like search_path() isn't a good name for this, since only one value is returned (not a list), and since it's a simple string (the port_name), not a path at all. Unless this routine is called from multiple places, I would've made this an explicit if and inlined it into path_to_test_expectations_file(). > Tools/Scripts/webkitpy/layout_tests/port/win.py:49 > + return map(self._webkit_baseline_path, search_paths) Nit ... I think the old name was better, given that it is in fact a list of port names, not search paths.

This regressed 2 tests. Sadly teh cqs are not currently running the python tests. /me shakes his fist at abarth!

Looking into a fix now.

(In reply to comment #22) > (From update of attachment 99179 [details]) Since I'm making a regression fix now, I will add your comments as well. Thanks for the review! > > Tools/Scripts/webkitpy/layout_tests/port/base.py:114 > > + self._http_lock = None # FIXME: Why does this live on the port object? > > Where else would it live, given that locking and unlocking the servers are methods on the port object? Are you asking why isn't a lock just returned from the port object? It seems like the port object does too much. It's storage for information about the actual port. It's confused as to if it's a model or a controller. I don't see why it should store the locks during the actual running of the layout test. Maybe it's used for locking/unlocking, but someone else should do the storage, no? > > Tools/Scripts/webkitpy/layout_tests/port/base.py:136 > > + self._test_configuration = None # FIXME: This does not belong on the real object. > > Yes it does. This is not something for testing, this is the TestConfig object returned from test_configuration() and used by the test_expectations classes. OK... I'll have to read more about what those do, but I'll remove the fixme. > > Tools/Scripts/webkitpy/layout_tests/port/base.py:451 > > + """Returns the list of top-level test directories.""" > > Hmm. I found that comment helpful; oh well. It seemed that "Used by --clobber-old-results." would go quickly out of date, and wasn't any more helpful than just doing a grep, so I removed it. > > Tools/Scripts/webkitpy/layout_tests/port/gtk.py:-119 > > - > > Why did you delete this? Because None is implicitly returned? That may be true, but I think it's bad style to rely on that, and better to be explicit. Yes, I have mixed feelings about this -- silly to add 3 lines just to return None when it's implicit, but it's also nice to be explicit. I'm happy to add the None back. > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:52 > > + self._cached_apache_path = None # FIXME: This class should use @memoized instead. > > Meh :) It will actually simplify things greatly, you'll see. :) > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:73 > > + return self._wk2_port_name() if self.get_option('webkit_test_runner') else self.port_name > > Seems like search_path() isn't a good name for this, since only one value is returned (not a list), and since it's a simple string (the port_name), not a path at all. Unless this routine is called from multiple places, I would've made this an explicit if and inlined it into path_to_test_expectations_file(). Happy to do so. > > Tools/Scripts/webkitpy/layout_tests/port/win.py:49 > > + return map(self._webkit_baseline_path, search_paths) > > Nit ... I think the old name was better, given that it is in fact a list of port names, not search paths. OK.

(In reply to comment #25) > (In reply to comment #22) > > (From update of attachment 99179 [details] [details]) > > Since I'm making a regression fix now, I will add your comments as well. Thanks for the review! > > > > Tools/Scripts/webkitpy/layout_tests/port/base.py:114 > > > + self._http_lock = None # FIXME: Why does this live on the port object? > > > > Where else would it live, given that locking and unlocking the servers are methods on the port object? Are you asking why isn't a lock just returned from the port object? > > It seems like the port object does too much. It's storage for information about the actual port. It's confused as to if it's a model or a controller. I don't see why it should store the locks during the actual running of the layout test. Maybe it's used for locking/unlocking, but someone else should do the storage, no? Well, the implementation of how to do a system-/machine-wide lock is Port-specific. All Port-specific logic used by new-run-webkit-tests is funneled through the Port object. Given that, I'm not sure how differently things could be implemented. That said, I definitely agree that the Port object does too much. There are large chunks of functionality in base.Port that no one overrides and that isn't port-specific. That could probably be moved elsewhere. There are also probably chunks of functionality that could be profitably moved out of Port and into some other "platform-specific" object like we have done for filesystem and user. I think the design principle that "all port-specific logic used by NRWT is funneled through the Port object" is incredibly useful, because it allows for a great variety of implementations (like the Test port, which demonstrates that you can run the whole test suite without a real filesystem). In my current thinking, the Port object should probably be refactored into a composite object that simply forwards methods onto other objects that are mostly generic but could be overridden by the "weirder" ports. One of the other objects would be the set of methods that all of the "real" ports have actually needed to override, like baseline search path, paths to binaries, etc. . I'm still hoping to work on that refactoring as a "fun" project, but I don't want to collide with any of the stuff you're doing, so we should probably coordinate on this. > > > > Tools/Scripts/webkitpy/layout_tests/port/base.py:136 > > > + self._test_configuration = None # FIXME: This does not belong on the real object. > > > > Yes it does. This is not something for testing, this is the TestConfig object returned from test_configuration() and used by the test_expectations classes. > > OK... I'll have to read more about what those do, but I'll remove the fixme. > > > > Tools/Scripts/webkitpy/layout_tests/port/base.py:451 > > > + """Returns the list of top-level test directories.""" > > > > Hmm. I found that comment helpful; oh well. > > It seemed that "Used by --clobber-old-results." would go quickly out of date, and wasn't any more helpful than just doing a grep, so I removed it. > > > > Tools/Scripts/webkitpy/layout_tests/port/gtk.py:-119 > > > - > > > > Why did you delete this? Because None is implicitly returned? That may be true, but I think it's bad style to rely on that, and better to be explicit. > > Yes, I have mixed feelings about this -- silly to add 3 lines just to return None when it's implicit, but it's also nice to be explicit. I'm happy to add the None back. > > > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:52 > > > + self._cached_apache_path = None # FIXME: This class should use @memoized instead. > > > > Meh :) > > It will actually simplify things greatly, you'll see. :) > > > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:73 > > > + return self._wk2_port_name() if self.get_option('webkit_test_runner') else self.port_name > > > > Seems like search_path() isn't a good name for this, since only one value is returned (not a list), and since it's a simple string (the port_name), not a path at all. Unless this routine is called from multiple places, I would've made this an explicit if and inlined it into path_to_test_expectations_file(). > > Happy to do so. > > > > Tools/Scripts/webkitpy/layout_tests/port/win.py:49 > > > + return map(self._webkit_baseline_path, search_paths) > > > > Nit ... I think the old name was better, given that it is in fact a list of port names, not search paths. > > OK.

Committed r90077: <http://trac.webkit.org/changeset/90077>