RESOLVED FIXED 37845
[chromium] new-run-webkit-tests --use-drt should run on Linux
https://bugs.webkit.org/show_bug.cgi?id=37845
Summary [chromium] new-run-webkit-tests --use-drt should run on Linux
Tony Chang
Reported 2010-04-20 01:56:30 PDT
[chromium] new-run-webkit-tests --use-drt should run on Linux
Attachments
Patch (3.40 KB, patch)
2010-04-20 01:56 PDT, Tony Chang
no flags
Patch (6.45 KB, patch)
2010-04-20 02:16 PDT, Tony Chang
no flags
Patch (4.58 KB, patch)
2010-04-22 17:57 PDT, Tony Chang
no flags
Patch (7.17 KB, patch)
2010-05-09 22:19 PDT, Tony Chang
tkent: review+
Tony Chang
Comment 1 2010-04-20 01:56:50 PDT
Tony Chang
Comment 2 2010-04-20 02:16:31 PDT
Tony Chang
Comment 3 2010-04-20 02:17:06 PDT
(In reply to comment #2) > Created an attachment (id=53788) [details] > Patch Updated to fix the output directory of the tests. It was writing the test results into xcodebuild.
Adam Barth
Comment 4 2010-04-22 12:51:17 PDT
Comment on attachment 53788 [details] Patch WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:91 + def results_directory(self): Why is this code copy/pasted twice? It seems better to have it in the base class if its used by more than one subclass. WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:167 + def _path_to_image_diff(self): This code looks copy/pasted too. WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win.py:101 + def results_directory(self): Here the code is again. We don't want any copy/paste code.
Tony Chang
Comment 5 2010-04-22 17:57:35 PDT
Tony Chang
Comment 6 2010-04-22 17:58:36 PDT
(In reply to comment #4) > (From update of attachment 53788 [details]) > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:91 > + def results_directory(self): > Why is this code copy/pasted twice? It seems better to have it in the base > class if its used by more than one subclass. I copied the functions because they call _build_path() which is only defined in the implementation classes. But I see other methods doing this, so I guess that's ok.
Eric Seidel (no email)
Comment 7 2010-05-08 23:26:40 PDT
Comment on attachment 54115 [details] Patch Looks sane, but where are the tests?
Tony Chang
Comment 8 2010-05-09 22:19:12 PDT
Tony Chang
Comment 9 2010-05-09 22:20:44 PDT
(In reply to comment #7) > (From update of attachment 54115 [details]) > Looks sane, but where are the tests? Good point, added some tests.
Tony Chang
Comment 10 2010-05-17 23:40:07 PDT
Maybe Kent-san can review this now. Eric just wanted some tests added earlier.
Kent Tamura
Comment 11 2010-05-17 23:50:47 PDT
Comment on attachment 55519 [details] Patch This looks OK. I'll set r+ tomorrow if no one adds review comments.
Tony Chang
Comment 12 2010-05-18 22:12:28 PDT
Note You need to log in before you can comment on or make changes to this bug.