Bug 35982 – rebaseline_chromium_webkit_tests doesn't handle other ports correctly

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Dirk Pranke]

rebaseline_chromium_webkit_tests doesn't handle other ports correctly

[Dirk Pranke]

Created attachment 50427 [details]
Patch

[Dirk Pranke]

Victor, can you take a look at this and make sure it looks right?

[Ojan Vafai]

Comment on attachment 50427 [details]
Patch

> +    # FIXME: This only works on the Chromium implementations for now.
> +    options.chromium = True

Is this option used anywhere?

[Dirk Pranke]

(In reply to comment #3)
> (From update of attachment 50427 [details])
> > +    # FIXME: This only works on the Chromium implementations for now.
> > +    options.chromium = True
> 
> Is this option used anywhere?

It is used in port/factory.py to change the default on the Mac port from "mac" to "chromium-mac".

I can add a comment to that effect.

[Ojan Vafai]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 50427 [details] [details])
> > > +    # FIXME: This only works on the Chromium implementations for now.
> > > +    options.chromium = True
> > 
> > Is this option used anywhere?
> 
> It is used in port/factory.py to change the default on the Mac port from "mac"
> to "chromium-mac".
> 
> I can add a comment to that effect.

No that's fine. Please just update the change description. Also, I don't really understand the FIXME. What is "this"?

[Dirk Pranke]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 50427 [details] [details] [details])
> > > > +    # FIXME: This only works on the Chromium implementations for now.
> > > > +    options.chromium = True
> > > 
> > > Is this option used anywhere?
> > 
> > It is used in port/factory.py to change the default on the Mac port from "mac"
> > to "chromium-mac".
> > 
> > I can add a comment to that effect.
> 
> No that's fine. Please just update the change description. Also, I don't really
> understand the FIXME. What is "this"?

This == "this entire script". I can clarify that as well.

[Victor Wang]

Comment on attachment 50427 [details]
Patch

platform = 'chromium-' + self._platform

This does not seem to be right. The platform here should match those in test_expectations.txt like "WIN", "LINUX" etc. no "chromium-" prefix.

[Dirk Pranke]

(In reply to comment #7)
> (From update of attachment 50427 [details])
> platform = 'chromium-' + self._platform
> 
> This does not seem to be right. The platform here should match those in
> test_expectations.txt like "WIN", "LINUX" etc. no "chromium-" prefix.

The platform local variable in this case is only used in a log message and then to form the path to the baseline directory (see line 440), which needs to be of the form 'chromium-X', not 'X'.

[Victor Wang]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 50427 [details] [details])
> > platform = 'chromium-' + self._platform
> > 
> > This does not seem to be right. The platform here should match those in
> > test_expectations.txt like "WIN", "LINUX" etc. no "chromium-" prefix.
> 
> The platform local variable in this case is only used in a log message and then
> to form the path to the baseline directory (see line 440), which needs to be of
> the form 'chromium-X', not 'X'.

ya, you are right. I was looking at the wrong line.

[Dirk Pranke]

Created attachment 50437 [details]
Patch

[Dirk Pranke]

added a revised patch with some clarified comments and one of the FIXME's fixed - added a hook to the Port interface to convert a test platform name (e.g., 'win') back into the port name 'chromium-win') so that we can write into the correct baseline directory.

[Victor Wang]

(In reply to comment #11)
> added a revised patch with some clarified comments and one of the FIXME's fixed
> - added a hook to the Port interface to convert a test platform name (e.g.,
> 'win') back into the port name 'chromium-win') so that we can write into the
> correct baseline directory.
LGTM

[Dimitri Glazkov (Google)]

Comment on attachment 50437 [details]
Patch

If it's good for Victor, it's good for me.

[Shinichiro Hamaji]

Thanks for the quick fix!

I think we need to fix the following line in _is_dup_baseline, too.

        all_baselines = self._port.expected_baselines(test_filepath,
                                                      suffix, True)

If the result of windows is same as the result of linux, the rebaseline tool should remove linux's one. However, when I ran the rebaseline tool on linux, it removed the result of windows. I work-arounded by changing

    port_obj = port.get(None, options)

to

    port_obj = port.get('chromium-win', options)

But I'm not sure what is the best way to fix this issue.

[Victor Wang]

I think the problem is the port_obj is wrong. port_object needs to be set based on platform to rebaseline, not the platform the script is running.

I think the fix is setting port_obj by giving right port_name using the new function test_platform_name_to_name in this patch.

Dirk,
Looks like you haven't commit your patch, do you want fix this with your patch? or you could go ahead commit yours and I can fix it in separate patch.

(In reply to comment #14)
> Thanks for the quick fix!
> 
> I think we need to fix the following line in _is_dup_baseline, too.
> 
>         all_baselines = self._port.expected_baselines(test_filepath,
>                                                       suffix, True)
> 
> If the result of windows is same as the result of linux, the rebaseline tool
> should remove linux's one. However, when I ran the rebaseline tool on linux, it
> removed the result of windows. I work-arounded by changing
> 
>     port_obj = port.get(None, options)
> 
> to
> 
>     port_obj = port.get('chromium-win', options)
> 
> But I'm not sure what is the best way to fix this issue.

[Dirk Pranke]

(In reply to comment #15)
> I think the problem is the port_obj is wrong. port_object needs to be set based
> on platform to rebaseline, not the platform the script is running.
> 
> I think the fix is setting port_obj by giving right port_name using the new
> function test_platform_name_to_name in this patch.
> 
> Dirk,
> Looks like you haven't commit your patch, do you want fix this with your patch?
> or you could go ahead commit yours and I can fix it in separate patch.
> 

Victor,

I think your analysis is correct but I'm not familiar enough with this code to be sure about the best way to fix it. I'll commit my patch and then we can fix this in a second patch.

[Dirk Pranke]

Committed r55850: <http://trac.webkit.org/changeset/55850>