[Windows] NRWT reports failure for 'mac' test cases

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Brent Fulgham]

Created attachment 202755 [details]
Screenshot of example output from NRWT.

When running ORWT on the Apple Windows port, several test results 'fall back' to the mac port for expected results.  NRWT also appears to be checking the 'mac' port output folder, but registers a number of items as failures even though there are existing 'win' results that should be overriding these expected results.

[Brent Fulgham]

Created attachment 202756 [details]
Screenshot of ORWT output for same build

[Brent Fulgham]

Created attachment 203076 [details]
Patch

[Brent Fulgham]

Delaying processing of this patch until I run local tests on my Windows builder.

[Brent Fulgham]

Patch seems to work properly.  There were originally 336 failures (with 258 of them coming from the platform/mac folder).  Now there are 78 failures.

[Dirk Pranke]

Comment on attachment 203076 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203076&action=review

> Tools/Scripts/webkitpy/port/win.py:144
> +        return dirs_to_skip

There's too much duplicated code in these routines, I think. 

What I would probably try to do is change the implementation of _tests_for_other_platforms() in base.py to call a new test_search_path() in base.py (like you have here), define test_search_path() in base.py as calling baseline_search_path() (to get the same logic by default), and then override test_search_path() in win.py to call the super method, but filter out the mac ports.

Does that make sense?

[Brent Fulgham]

Created attachment 203094 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 203094 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203094&action=review

> Tools/Scripts/webkitpy/port/win.py:124
> +        # Remove the mac tests, which we never want to execute
> +        regex = re.compile('mac.*')

We don't need to use a regex for this. Also, this regex is going to match anything that happens to start with "mac"; e.g. maclaine.

> Tools/Scripts/webkitpy/port/win.py:125
> +        test_fallback_names = [x for x in self.baseline_search_path() if not regex.match(x)]

Please don't use one letter variable like x. spell out path. We can do path.startswith('mac/') instead.

[Dirk Pranke]

Comment on attachment 203094 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203094&action=review

>> Tools/Scripts/webkitpy/port/win.py:125
>> +        test_fallback_names = [x for x in self.baseline_search_path() if not regex.match(x)]
> 
> Please don't use one letter variable like x. spell out path. We can do path.startswith('mac/') instead.

I was also going to suggest x.startswith('mac'). You can't use 'mac/', since the slash isn't in the path. However, since baseline_search_path is just a list of dirs, I don't think you need to worry about 'maclaine' or other likely conflicts.

[Ryosuke Niwa]

Comment on attachment 203094 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203094&action=review

>>> Tools/Scripts/webkitpy/port/win.py:125
>>> +        test_fallback_names = [x for x in self.baseline_search_path() if not regex.match(x)]
>> 
>> Please don't use one letter variable like x. spell out path. We can do path.startswith('mac/') instead.
> 
> I was also going to suggest x.startswith('mac'). You can't use 'mac/', since the slash isn't in the path. However, since baseline_search_path is just a list of dirs, I don't think you need to worry about 'maclaine' or other likely conflicts.

I see.

[Brent Fulgham]

(In reply to comment #7)
> (From update of attachment 203094 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203094&action=review
> 
> > Tools/Scripts/webkitpy/port/win.py:124
> > +        # Remove the mac tests, which we never want to execute
> > +        regex = re.compile('mac.*')
> 
> We don't need to use a regex for this. Also, this regex is going to match anything that happens to start with "mac"; e.g. maclaine.

That's the desired behavior --we want to exclude mac, mac-lion, mac-future, etc.

[Brent Fulgham]

(In reply to comment #8)
> (From update of attachment 203094 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203094&action=review
> 
> >> Tools/Scripts/webkitpy/port/win.py:125
> >> +        test_fallback_names = [x for x in self.baseline_search_path() if not regex.match(x)]
> > 
> > Please don't use one letter variable like x. spell out path. We can do path.startswith('mac/') instead.
> 
> I was also going to suggest x.startswith('mac'). You can't use 'mac/', since the slash isn't in the path. However, since baseline_search_path is just a list of dirs, I don't think you need to worry about 'maclaine' or other likely conflicts.

The result of calling self.baseline_search_path() is an expanded set of directories, such as:

['/cygdrive/c/Projects/WebKit/OpenSource/LayoutTests/platform/win',
 '/cygdrive/c/Projects/WebKit/OpenSource/LayoutTests/platform/mac-lion',
 '/cygdrive/c/Projects/WebKit/OpenSource/LayoutTests/platform/mac']

Consequently, we can't just search on 'starts with'.

The regex allowed me to get rid of the mac and mac-lion cases here, though I suppose it might have clobbered some future path.

[Brent Fulgham]

Created attachment 203101 [details]
Uses dpranke's suggested syntax for excluding mac paths.

[Brent Fulgham]

Created attachment 203102 [details]
Uses dpranke's suggested syntax for excluding the mac test folders.

[Ryosuke Niwa]

Comment on attachment 203102 [details]
Uses dpranke's suggested syntax for excluding the mac test folders.

View in context: https://bugs.webkit.org/attachment.cgi?id=203102&action=review

> Tools/Scripts/webkitpy/port/win.py:123
> +        # Remove the mac tests, which we never want to execute

This comment more or less repeats the code. Please remove it.

[Brent Fulgham]

Committed r150850: <http://trac.webkit.org/changeset/150850>