rebaseline_chromium_webkit_tests doesn't handle other ports correctly
Created attachment 50427 [details] Patch
Victor, can you take a look at this and make sure it looks right?
Comment on attachment 50427 [details] Patch > + # FIXME: This only works on the Chromium implementations for now. > + options.chromium = True Is this option used anywhere?
(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.
(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"?
(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.
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.
(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'.
(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.
Created attachment 50437 [details] Patch
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.
(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
Comment on attachment 50437 [details] Patch If it's good for Victor, it's good for me.
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.
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.
(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.
Committed r55850: <http://trac.webkit.org/changeset/55850>