| Summary: | NRWT fails on unreleased versions of Mac OS X | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Adam Roben (:aroben) <aroben> | ||||||
| Component: | Tools / Tests | Assignee: | Adam Barth <abarth> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | abarth, darin, dpranke, eric, mrowe, ojan, simon.fraser, thorton | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
bah. Looks like this broke with some recent refactoring and we don't have a test for it? Yeah, I'm sure I broke it. *** Bug 73927 has been marked as a duplicate of this bug. *** Someone teach me how to fix this. It’s blocking a lot of work on WebKit at Apple. The proximate fix is to add the string mac-future to self.VERSION_FALLBACK_ORDER in apple.py. I'm not sure what else needs to be done. I'm on #webkit and happy to help. From Bug 73927: > I get this on my newfangled post-Lion computer when I use run-webkit-tests. > > Failed to open Skipped file: /Volumes/Home/darin/Safari/OpenSource/LayoutTests/platform/mac-future/Skipped > > This doesn’t seem right. While there might be “mac-future” as an internal version inside the scripts, we have no intention of actually creating mac-future directories. Is this working as designed? It sounds like we need to teach the tool to look for the mac-future skipped list somewhere else, presumably in LayoutTests/platform/mac/Skipped? (In reply to comment #6) > It sounds like we need to teach the tool to look for the mac-future skipped list somewhere else, presumably in LayoutTests/platform/mac/Skipped? That sounds right. Here is the design concept: The directory platform/mac represents Mac in the future and the other directories such as platform/mac-lion mean “things that are different in Lion than the future” and platform/mac-snowleopard means “things that are different in SnowLeopard than Lion”. Skipped files are an anomaly here; they are independent rather than built on top of each other as deltas, but that is a design mistake. The Skipped file in each directory should be a delta from the Skipped file from below. There should be a master Skipped file that is not platform specific, and then each directory should modify that one. I’d expect something similar for the Chrome-style expected results file too. And I’d also like to merge the Skipped concept and the expected results concept into a single file. I'm trying to figure out how I can reproduce this issue on a Lion machine by haxoring the version detection logic. I'm developing a theory. That message is just a warning, by the way. Ok. I see the bug. Should be pretty easy to fix. I need to do an Apple-Mac build to be sure. That's going to take about 22 minutes. I hope to have a patch to most here in about 45 minutes. Created attachment 118078 [details]
Patch
Comment on attachment 118078 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118078&action=review > Tools/Scripts/webkitpy/layout_tests/port/mac.py:79 > - ApplePort.__init__(self, host, **kwargs) > self._operating_system = 'mac' > + ApplePort.__init__(self, host, **kwargs) This change didn't make it into your ChangeLog. > Tools/Scripts/webkitpy/layout_tests/port/win.py:79 > - ApplePort.__init__(self, host, **kwargs) > self._operating_system = 'win' > + ApplePort.__init__(self, host, **kwargs) Nor did this one. > This change didn't make it into your ChangeLog.
Oops. I revised the patch after writing the ChangeLog. Will fix.
Created attachment 118080 [details]
Patch
Comment on attachment 118080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118080&action=review > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:354 > + _log.debug("Skipped does not exist: %s" % filename) It seems like skipped_file_search_paths() shouldn't be returning a directory that doesn't exist for mac-future. Can you add a fixme or file a separate bug for that? As an aside, I'm not sure what the difference between self.port_name and self.name() is supposed to be; I can't remember if I added that or someone else did, but it seems like the two names are confusing so we should probably clean that up at some point as well. > It seems like skipped_file_search_paths() shouldn't be returning a directory that doesn't exist for mac-future. Can you add a fixme or file a separate bug for that? Will do. > As an aside, I'm not sure what the difference between self.port_name and self.name() is supposed to be; I can't remember if I added that or someone else did, but it seems like the two names are confusing so we should probably clean that up at some point as well. That puzzled me as well. It seems like a good longer term direction for this code is to separate out the parsing of port names, the state representing which port we're talking about, and the port-specific behaviors. At the moment, they're fairly tightly bound together. (In reply to comment #16) > That puzzled me as well. It seems like a good longer term direction for this code is to separate out the parsing of port names, the state representing which port we're talking about, and the port-specific behaviors. At the moment, they're fairly tightly bound together. Agreed. Every time I've tried to do this thus far it's devolved into a rewrite-the-world-at-once patch for everything under layout_tests/port/*, but I'm due to try it again. Committed r102161: <http://trac.webkit.org/changeset/102161> Comment on attachment 118080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118080&action=review >> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:354 >> + _log.debug("Skipped does not exist: %s" % filename) > > It seems like skipped_file_search_paths() shouldn't be returning a directory that doesn't exist for mac-future. Can you add a fixme or file a separate bug for that? > > As an aside, I'm not sure what the difference between self.port_name and self.name() is supposed to be; I can't remember if I added that or someone else did, but it seems like the two names are confusing so we should probably clean that up at some point as well. name() is the random, port-specific string which gets passed in and is parsed with different rules per-port. port_name is supposed to be the root name of the port, like "mac", "win", "chromium", "qt", "gtk", etc. It was a somewhat failed attempt at preparing the Port clases for moving "name" parsing out of the Port constructors (into Factory or elsewhere). At least in some places port_name seems to include the version. For example, the port_name argument to __init__ can include the version suffix. |
When I run NRWT on an unreleased version of Mac OS X, I see this: Failed to open Skipped file: /Users/aroben/dev/WebKit/OpenSource/LayoutTests/platform/mac-future/Skipped Starting 1 worker ...Process _Process-1: Traceback (most recent call last): File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/multiprocessing/process.py", line 232, in _bootstrap self.run() File "/Users/aroben/dev/WebKit/OpenSource/Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py", line 259, in run port_obj = host.port_factory.get(self._platform_name, options) File "/Users/aroben/dev/WebKit/OpenSource/Tools/Scripts/webkitpy/layout_tests/port/factory.py", line 147, in get return self._get_kwargs(**kwargs) File "/Users/aroben/dev/WebKit/OpenSource/Tools/Scripts/webkitpy/layout_tests/port/factory.py", line 127, in _get_kwargs return maker(host, **kwargs) File "/Users/aroben/dev/WebKit/OpenSource/Tools/Scripts/webkitpy/layout_tests/port/mac.py", line 78, in __init__ ApplePort.__init__(self, host, **kwargs) File "/Users/aroben/dev/WebKit/OpenSource/Tools/Scripts/webkitpy/layout_tests/port/apple.py", line 73, in __init__ assert port_name in self.VERSION_FALLBACK_ORDER, "%s is not in %s" % (port_name, self.VERSION_FALLBACK_ORDER) AssertionError: mac-future is not in ['mac-leopard', 'mac-snowleopard', 'mac-lion', 'mac'] Starting testing ... And then it just hangs there forever.