35982 2010-03-10 12:48:32 -0800 rebaseline_chromium_webkit_tests doesn't handle other ports correctly 2010-03-11 11:56:16 -0800 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Other OS X 10.5 RESOLVED FIXED P2 Normal --- 1 dpranke dpranke dglazkov eric hamaji levin ojan victorw oldest_to_newest 198271 0 dpranke 2010-03-10 12:48:32 -0800 rebaseline_chromium_webkit_tests doesn't handle other ports correctly 198273 1 50427 dpranke 2010-03-10 12:50:27 -0800 Created attachment 50427 Patch 198274 2 dpranke 2010-03-10 12:51:58 -0800 Victor, can you take a look at this and make sure it looks right? 198288 3 50427 ojan 2010-03-10 13:08:14 -0800 Comment on attachment 50427 Patch > + # FIXME: This only works on the Chromium implementations for now. > + options.chromium = True Is this option used anywhere? 198298 4 dpranke 2010-03-10 13:26:41 -0800 (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. 198302 5 ojan 2010-03-10 13:30:37 -0800 (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"? 198307 6 dpranke 2010-03-10 13:34:40 -0800 (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. 198314 7 50427 victorw 2010-03-10 13:41:02 -0800 Comment on attachment 50427 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. 198320 8 dpranke 2010-03-10 13:50:14 -0800 (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'. 198331 9 victorw 2010-03-10 14:02:11 -0800 (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. 198335 10 50437 dpranke 2010-03-10 14:07:58 -0800 Created attachment 50437 Patch 198336 11 dpranke 2010-03-10 14:09:26 -0800 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. 198340 12 victorw 2010-03-10 14:20:16 -0800 (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 198343 13 50437 dglazkov 2010-03-10 14:23:45 -0800 Comment on attachment 50437 Patch If it's good for Victor, it's good for me. 198497 14 hamaji 2010-03-10 22:56:02 -0800 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. 198680 15 victorw 2010-03-11 10:55:04 -0800 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. 198713 16 dpranke 2010-03-11 11:52:53 -0800 (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. 198718 17 dpranke 2010-03-11 11:56:16 -0800 Committed r55850: <http://trac.webkit.org/changeset/55850> 50427 2010-03-10 12:50:27 -0800 2010-03-10 14:07:55 -0800 Patch bug-35982-20100310125026.patch text/plain 1814 dpranke ZGlmZiAtLWdpdCBhL1dlYktpdFRvb2xzL0NoYW5nZUxvZyBiL1dlYktpdFRvb2xzL0NoYW5nZUxv ZwppbmRleCAwYmE3MjA0Li5jYWE0ZTU5IDEwMDY0NAotLS0gYS9XZWJLaXRUb29scy9DaGFuZ2VM b2cKKysrIGIvV2ViS2l0VG9vbHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTAtMDMt MTAgIERpcmsgUHJhbmtlICA8ZHByYW5rZUBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgcmViYXNlbGluZV9jaHJvbWl1bV93ZWJr aXRfdGVzdHMgZG9lc24ndCBoYW5kbGUgb3RoZXIgcG9ydHMgY29ycmVjdGx5LgorICAgICAgICBJ dCBhbHdheXMgdXNlcyB0aGUgcG9ydCB0aGUgc2NyaXB0IGlzIGV4ZWN1dGluZyBvbiBpbnN0ZWFk IG9mIHRoZQorICAgICAgICBwb3J0IGJlaW5nIHJlYmFzZWxpbmVkLCBhbmQgc28gaWYgeW91J3Jl IHJlYmFzZWxpbmluZyByZXN1bHRzIGZyb20KKyAgICAgICAgb3RoZXIgcG9ydHMsIHRoZSBleGVj dXRpbmcgcG9ydHMnIHJlc3VsdHMgZ2V0IG92ZXJ3cml0dGVuLgorCisgICAgICAgIGh0dHBzOi8v YnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0zNTk4MgorCisgICAgICAgICogU2NyaXB0 cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvcmViYXNlbGluZV9jaHJvbWl1bV93ZWJraXRfdGVzdHMu cHk6CisKIDIwMTAtMDMtMTAgIFJvYmVydCBIb2dhbiAgPHJvYmVydEB3ZWJraXQub3JnPgogCiAg ICAgICAgIFJldmlld2VkIGJ5IEhvbGdlciBGcmV5dGhlci4KZGlmZiAtLWdpdCBhL1dlYktpdFRv b2xzL1NjcmlwdHMvd2Via2l0cHkvbGF5b3V0X3Rlc3RzL3JlYmFzZWxpbmVfY2hyb21pdW1fd2Vi a2l0X3Rlc3RzLnB5IGIvV2ViS2l0VG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMv cmViYXNlbGluZV9jaHJvbWl1bV93ZWJraXRfdGVzdHMucHkKaW5kZXggOGJlZWUzYi4uMTUwMjZh NyAxMDA2NDQKLS0tIGEvV2ViS2l0VG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMv cmViYXNlbGluZV9jaHJvbWl1bV93ZWJraXRfdGVzdHMucHkKKysrIGIvV2ViS2l0VG9vbHMvU2Ny aXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvcmViYXNlbGluZV9jaHJvbWl1bV93ZWJraXRfdGVz dHMucHkKQEAgLTQwNCw3ICs0MDQsOCBAQCBjbGFzcyBSZWJhc2VsaW5lcihvYmplY3QpOgogICAg ICAgICBmb3IgbmFtZSBpbiB6aXBfbmFtZWxpc3Q6CiAgICAgICAgICAgICBfbG9nLmRlYnVnKCcg ICcgKyBuYW1lKQogCi0gICAgICAgIHBsYXRmb3JtID0gc2VsZi5fcG9ydC5uYW1lKCkKKyAgICAg ICAgIyBGSVhNRTogTWFrZSB0aGlzIHdvcmsgd2l0aCBub24tY2hyb21pdW0gcGxhdGZvcm1zLgor ICAgICAgICBwbGF0Zm9ybSA9ICdjaHJvbWl1bS0nICsgc2VsZi5fcGxhdGZvcm0KICAgICAgICAg X2xvZy5kZWJ1ZygnUGxhdGZvcm0gZGlyOiAiJXMiJywgcGxhdGZvcm0pCiAKICAgICAgICAgdGVz dF9ubyA9IDEKQEAgLTk4Niw2ICs5ODcsOSBAQCBkZWYgbWFpbigpOgogICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAnIHJlYmFzZWxpbmluZyBjb21wYXJpc29uLicpKQogCiAgICAg b3B0aW9ucyA9IG9wdGlvbl9wYXJzZXIucGFyc2VfYXJncygpWzBdCisKKyAgICAjIEZJWE1FOiBU aGlzIG9ubHkgd29ya3Mgb24gdGhlIENocm9taXVtIGltcGxlbWVudGF0aW9ucyBmb3Igbm93Lgor ICAgIG9wdGlvbnMuY2hyb21pdW0gPSBUcnVlCiAgICAgcG9ydF9vYmogPSBwb3J0LmdldChOb25l LCBvcHRpb25zKQogCiAgICAgIyBTZXQgdXAgb3VyIGxvZ2dpbmcgZm9ybWF0Lgo= 50437 2010-03-10 14:07:58 -0800 2010-03-10 14:23:45 -0800 Patch bug-35982-20100310140757.patch text/plain 3653 dpranke ZGlmZiAtLWdpdCBhL1dlYktpdFRvb2xzL0NoYW5nZUxvZyBiL1dlYktpdFRvb2xzL0NoYW5nZUxv ZwppbmRleCAwYmE3MjA0Li45MzY3YjYzIDEwMDY0NAotLS0gYS9XZWJLaXRUb29scy9DaGFuZ2VM b2cKKysrIGIvV2ViS2l0VG9vbHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTggQEAKKzIwMTAtMDMt MTAgIERpcmsgUHJhbmtlICA8ZHByYW5rZUBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgcmViYXNlbGluZV9jaHJvbWl1bV93ZWJr aXRfdGVzdHMgZG9lc24ndCBoYW5kbGUgb3RoZXIgcGxhZm9ybXMKKyAgICAgICAgY29ycmVjdGx5 IChlLmcuLCBpZiB5b3UgcnVuIG9uIHRoZSBNYWMgcGxhdGZvcm0gYW5kIHRyeSB0byAKKyAgICAg ICAgcmViYXNlbGluZSB0aGUgV0lOIHJlc3VsdHMsIHRoZSByZXN1bHQgZ2V0cyB3cml0dGVuIGlu dG8gCisgICAgICAgIHBsYXRmb3JtL21hYyBpbnN0ZWFkIG9mIHBsYXRmb3JtL2Nocm9taXVtLXdp bikuIEFsc28sIHRoaXMgc2NyaXB0CisgICAgICAgIGRvZXNuJ3Qgd29yayBvbiBub24tQ2hyb21p dW0gcG9ydHMsIHNvIHdlIG5lZWQgdG8gZml4IHRoYXQgYXQgc29tZQorICAgICAgICBwb2ludC4K KworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MzU5ODIK KworICAgICAgICAqIFNjcmlwdHMvd2Via2l0cHkvbGF5b3V0X3Rlc3RzL3JlYmFzZWxpbmVfY2hy b21pdW1fd2Via2l0X3Rlc3RzLnB5OgorCiAyMDEwLTAzLTEwICBSb2JlcnQgSG9nYW4gIDxyb2Jl cnRAd2Via2l0Lm9yZz4KIAogICAgICAgICBSZXZpZXdlZCBieSBIb2xnZXIgRnJleXRoZXIuCmRp ZmYgLS1naXQgYS9XZWJLaXRUb29scy9TY3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9wb3J0 L2Jhc2UucHkgYi9XZWJLaXRUb29scy9TY3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9wb3J0 L2Jhc2UucHkKaW5kZXggMGM3MzE4Mi4uODE3OGQ4ZCAxMDA2NDQKLS0tIGEvV2ViS2l0VG9vbHMv U2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvcG9ydC9iYXNlLnB5CisrKyBiL1dlYktpdFRv b2xzL1NjcmlwdHMvd2Via2l0cHkvbGF5b3V0X3Rlc3RzL3BvcnQvYmFzZS5weQpAQCAtNDY1LDYg KzQ2NSwxMiBAQCBjbGFzcyBQb3J0KG9iamVjdCk6CiAgICAgICAgIG1pZ2h0IHJldHVybiAnbWFj JyBhcyBhIHRlc3RfcGxhdGZvcm0gbmFtZScuIiIiCiAgICAgICAgIHJhaXNlIE5vdEltcGxlbWVu dGVkRXJyb3IoJ1BvcnQucGxhdGZvcm1zJykKIAorICAgIGRlZiB0ZXN0X3BsYXRmb3JtX25hbWVf dG9fbmFtZShzZWxmLCB0ZXN0X3BsYXRmb3JtX25hbWUpOgorICAgICAgICAiIiJSZXR1cm5zIHRo ZSBQb3J0IHBsYXRmb3JtIG5hbWUgdGhhdCBjb3JyZXNwb25kcyB0byB0aGUgbmFtZSBhcworICAg ICAgICByZWZlcmVuY2VkIGluIHRoZSBleHBlY3RhdGlvbnMgZmlsZS4gRS5nLiwgIm1hYyIgcmV0 dXJucworICAgICAgICAiY2hyb21pdW0tbWFjIiBvbiB0aGUgQ2hyb21pdW0gcG9ydHMuIiIiCisg ICAgICAgIHJhaXNlIE5vdEltcGxlbWVudGVkRXJyb3IoJ1BvcnQudGVzdF9wbGF0Zm9ybV9uYW1l X3RvX25hbWUnKQorCiAgICAgZGVmIHZlcnNpb24oc2VsZik6CiAgICAgICAgICIiIlJldHVybnMg YSBzdHJpbmcgaW5kaWNhdGluZyB0aGUgdmVyc2lvbiBvZiBhIGdpdmVuIHBsYXRmb3JtLCBlLmcu CiAgICAgICAgICctbGVvcGFyZCcgb3IgJy14cCcuCmRpZmYgLS1naXQgYS9XZWJLaXRUb29scy9T Y3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9wb3J0L2Nocm9taXVtLnB5IGIvV2ViS2l0VG9v bHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvcG9ydC9jaHJvbWl1bS5weQppbmRleCAz N2U1Njk5Li4xM2NhZjRhIDEwMDY0NAotLS0gYS9XZWJLaXRUb29scy9TY3JpcHRzL3dlYmtpdHB5 L2xheW91dF90ZXN0cy9wb3J0L2Nocm9taXVtLnB5CisrKyBiL1dlYktpdFRvb2xzL1NjcmlwdHMv d2Via2l0cHkvbGF5b3V0X3Rlc3RzL3BvcnQvY2hyb21pdW0ucHkKQEAgLTE2OSw2ICsxNjksMTIg QEAgY2xhc3MgQ2hyb21pdW1Qb3J0KGJhc2UuUG9ydCk6CiAgICAgICAgIHJldHVybiBzZWxmLnRl c3RfYmFzZV9wbGF0Zm9ybV9uYW1lcygpICsgKCd3aW4teHAnLAogICAgICAgICAgICAgJ3dpbi12 aXN0YScsICd3aW4tNycpCiAKKyAgICBkZWYgdGVzdF9wbGF0Zm9ybV9uYW1lX3RvX25hbWUoc2Vs ZiwgdGVzdF9wbGF0Zm9ybV9uYW1lKToKKyAgICAgICAgaWYgdGVzdF9wbGF0Zm9ybV9uYW1lIGlu IHNlbGYudGVzdF9wbGF0Zm9ybV9uYW1lcygpOgorICAgICAgICAgICAgcmV0dXJuICdjaHJvbWl1 bS0nICsgdGVzdF9wbGF0Zm9ybV9uYW1lCisgICAgICAgIHJhaXNlIFZhbHVlRXJyb3IoJ1Vuc3Vw cG9ydGVkIHRlc3RfcGxhdGZvcm1fbmFtZTogJXMnICUKKyAgICAgICAgICAgICAgICAgICAgICAg ICB0ZXN0X3BsYXRmb3JtX25hbWUpCisKICAgICAjCiAgICAgIyBQUk9URUNURUQgTUVUSE9EUwog ICAgICMKZGlmZiAtLWdpdCBhL1dlYktpdFRvb2xzL1NjcmlwdHMvd2Via2l0cHkvbGF5b3V0X3Rl c3RzL3JlYmFzZWxpbmVfY2hyb21pdW1fd2Via2l0X3Rlc3RzLnB5IGIvV2ViS2l0VG9vbHMvU2Ny aXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvcmViYXNlbGluZV9jaHJvbWl1bV93ZWJraXRfdGVz dHMucHkKaW5kZXggOGJlZWUzYi4uZDk2NjdkZCAxMDA2NDQKLS0tIGEvV2ViS2l0VG9vbHMvU2Ny aXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvcmViYXNlbGluZV9jaHJvbWl1bV93ZWJraXRfdGVz dHMucHkKKysrIGIvV2ViS2l0VG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvcmVi YXNlbGluZV9jaHJvbWl1bV93ZWJraXRfdGVzdHMucHkKQEAgLTQwNCw3ICs0MDQsNyBAQCBjbGFz cyBSZWJhc2VsaW5lcihvYmplY3QpOgogICAgICAgICBmb3IgbmFtZSBpbiB6aXBfbmFtZWxpc3Q6 CiAgICAgICAgICAgICBfbG9nLmRlYnVnKCcgICcgKyBuYW1lKQogCi0gICAgICAgIHBsYXRmb3Jt ID0gc2VsZi5fcG9ydC5uYW1lKCkKKyAgICAgICAgcGxhdGZvcm0gPSBzZWxmLl9wb3J0LnRlc3Rf cGxhdGZvcm1fbmFtZV90b19uYW1lKHNlbGYuX3BsYXRmb3JtKQogICAgICAgICBfbG9nLmRlYnVn KCdQbGF0Zm9ybSBkaXI6ICIlcyInLCBwbGF0Zm9ybSkKIAogICAgICAgICB0ZXN0X25vID0gMQpA QCAtOTg2LDYgKzk4NiwxMSBAQCBkZWYgbWFpbigpOgogICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAnIHJlYmFzZWxpbmluZyBjb21wYXJpc29uLicpKQogCiAgICAgb3B0aW9ucyA9 IG9wdGlvbl9wYXJzZXIucGFyc2VfYXJncygpWzBdCisKKyAgICAjIEZJWE1FOiBTZXR0aW5nIG9w dGlvbnMuY2hyb21pdW0gdG8gVHJ1ZSBmb3JjZXMgcG9ydC5nZXQoKSB0byBvbmx5IGxvb2sKKyAg ICAjIGF0IENocm9taXVtIHBvcnRzOyB3ZSBoYXJkLWNvZGUgdGhpcyBiZWNhdXNlIHRoaXMgc2Ny aXB0IGRvZXNuJ3Qgd29yaworICAgICMgb24gbm9uLUNocm9taXVtIHBvcnRzIHlldC4gV2Ugc2hv dWxkIGZpeCB0aGF0LgorICAgIG9wdGlvbnMuY2hyb21pdW0gPSBUcnVlCiAgICAgcG9ydF9vYmog PSBwb3J0LmdldChOb25lLCBvcHRpb25zKQogCiAgICAgIyBTZXQgdXAgb3VyIGxvZ2dpbmcgZm9y bWF0Lgo=