Bug 72748 – NRWT fails on unreleased versions of Mac OS X

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

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.

[Dirk Pranke]

bah. Looks like this broke with some recent refactoring and we don't have a test for it?

[Eric Seidel (no email)]

Yeah, I'm sure I broke it.

[Adam Roben (:aroben)]

*** Bug 73927 has been marked as a duplicate of this bug. ***

[Darin Adler]

Someone teach me how to fix this. It’s blocking a lot of work on WebKit at Apple.

[Adam Barth]

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.

[Adam Barth]

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?

[Darin Adler]

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

[Adam Barth]

I'm trying to figure out how I can reproduce this issue on a Lion machine by haxoring the version detection logic.

[Adam Barth]

I'm developing a theory.  That message is just a warning, by the way.

[Adam Barth]

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.

[Adam Barth]

Created attachment 118078 [details]
Patch

[Adam Roben (:aroben)]

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.

[Adam Barth]

> This change didn't make it into your ChangeLog.

Oops.  I revised the patch after writing the ChangeLog.  Will fix.

[Adam Barth]

Created attachment 118080 [details]
Patch

[Dirk Pranke]

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.

[Adam Barth]

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

[Dirk Pranke]

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

[Adam Barth]

Committed r102161: <http://trac.webkit.org/changeset/102161>

[Eric Seidel (no email)]

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

[Adam Barth]

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.