Bug 80355 – Add a mechanism to rebaseline new ports

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ojan Vafai]

Add a mechanism to rebaseline new ports

[Ojan Vafai]

Created attachment 130238 [details]
Patch

[Adam Barth]

Interesting approach.

[Adam Barth]

Ojan, I looked for you in #webkit, but it looks like you might have left for the day.  My understanding of what you're doing in this patch is that you're marking a port as in the process of being "brought up", which changes the default behavior for incorporating new baselines.  Presumably, we'd remove the "new_port_fallback_port_name" attribute when we're done bringing up the port.

Generally, I like the approach, but I'd like to bikeshed about some of the names, if you'll indulge me.

[Adam Barth]

Comment on attachment 130238 [details]
Patch

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

I'm not sure whether these new names are any better.  I think these things are just hard to name.  :(

> Tools/Scripts/webkitpy/layout_tests/port/builders.py:53
> +    "Webkit Mac10.7": {"port_name": "chromium-mac-lion", "specifiers": set(["lion"]), "new_port_fallback_port_name": "chromium-mac-snowleopard"},

Maybe new_port_fallback_port_name -> move_overwritten_baselines_to ?  It's slightly hard to convey what this property does succinctly.

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:59
> +    argument_names = "BUILDER_NAME TEST_NAME [EXISTING_BASELINE_MOVE_TO_DIRECTORY]"

EXISTING_BASELINE_MOVE_TO_DIRECTORY => PLATFORM_TO_MOVE_EXISTING_BASELINES_TO ?  Arg.  These are hard things to name.  This is a port, not a directory name, right?

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:70
> +    def _copy_existing_baseline(self, baseline_move_to_directory, test_name, suffix):

baseline_move_to_directory -> platform_to_move_existing_baselines_to

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:77
> +        new_baseline = os.path.join(port.baseline_path(), self._file_name_for_expected_result(test_name, suffix))

_tool.filesystem.join (we do this to make testing easier on Windows)

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:84
> +            if not self._tool.scm().exists(new_baseline):
> +                self._tool.scm().add(new_baseline)

We should make sure this works if we're creating a new subdirectory for new_baseline.  I think SCM.add does this properly, but we should check.

[Ojan Vafai]

(In reply to comment #3)
> Ojan, I looked for you in #webkit, but it looks like you might have left for the day. 

Yup. Had to leave right after posting this.

> My understanding of what you're doing in this patch is that you're marking a port as in the process of being "brought up", which changes the default behavior for incorporating new baselines.  Presumably, we'd remove the "new_port_fallback_port_name" attribute when we're done bringing up the port.

Correct. I cribbed this idea from the rebaseline server.

> Generally, I like the approach, but I'd like to bikeshed about some of the names, if you'll indulge me.

I'm fine with your renames. They're not clearly better to me, but they're not worse either. :)

Thanks for the review.

[Ojan Vafai]

(In reply to comment #4)
> (From update of attachment 130238 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130238&action=review
> 
> > Tools/Scripts/webkitpy/tool/commands/rebaseline.py:84
> > +            if not self._tool.scm().exists(new_baseline):
> > +                self._tool.scm().add(new_baseline)
> 
> We should make sure this works if we're creating a new subdirectory for new_baseline.  I think SCM.add does this properly, but we should check.

Double-checked. It does add new parent directories.

[Dirk Pranke]

Comment on attachment 130238 [details]
Patch

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

>> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:59
>> +    argument_names = "BUILDER_NAME TEST_NAME [EXISTING_BASELINE_MOVE_TO_DIRECTORY]"
> 
> EXISTING_BASELINE_MOVE_TO_DIRECTORY => PLATFORM_TO_MOVE_EXISTING_BASELINES_TO ?  Arg.  These are hard things to name.  This is a port, not a directory name, right?

In one sense, it seems like we should be able to derive what directory to use by looking for all the ports that have port.baseline_path() in their fallback path, and using the directory that comes *before* that. However, that won't work if we have ports that fall back from win -> mac, and you'd have to specify which port to use, which is what you've done. 

Also, at some point this feels like overdesigning a problem that comes up rarely if you try to make it work with ports and not directories. So I guess this is a long-winded way of saying make this a directory, not a port :).

[Ojan Vafai]

Comment on attachment 130238 [details]
Patch

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

>>> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:59
>>> +    argument_names = "BUILDER_NAME TEST_NAME [EXISTING_BASELINE_MOVE_TO_DIRECTORY]"
>> 
>> EXISTING_BASELINE_MOVE_TO_DIRECTORY => PLATFORM_TO_MOVE_EXISTING_BASELINES_TO ?  Arg.  These are hard things to name.  This is a port, not a directory name, right?
> 
> In one sense, it seems like we should be able to derive what directory to use by looking for all the ports that have port.baseline_path() in their fallback path, and using the directory that comes *before* that. However, that won't work if we have ports that fall back from win -> mac, and you'd have to specify which port to use, which is what you've done. 
> 
> Also, at some point this feels like overdesigning a problem that comes up rarely if you try to make it work with ports and not directories. So I guess this is a long-winded way of saying make this a directory, not a port :).

In practice, it's the port-name. As you said, this is rare enough that hard-coding which port is being replaced is a good enough solution.

[Ojan Vafai]

Committed r109945: <http://trac.webkit.org/changeset/109945>