nrwt: port/Driver needs to support per-test command line args
Created attachment 129163 [details] Patch
Created attachment 129165 [details] remove a couple start() methods I missed
Comment on attachment 129163 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129163&action=review > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:-599 > - def start(self): > - if not self._proc: > - self._start() Isn't this used by run-perf-tests? > Tools/Scripts/webkitpy/layout_tests/port/test.py:355 > - return 'junk' > + return '<path_to_driver>' In other places we've used a string like: MOCK path_to_driver > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:486 > + cmd.extend(per_test_args) This line is different than the one in chromium.py
Created attachment 129172 [details] update w/ review feedback
(In reply to comment #3) > (From update of attachment 129163 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129163&action=review > > > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:-599 > > - def start(self): > > - if not self._proc: > > - self._start() > > Isn't this used by run-perf-tests? > Ugh. So it is. rniwa, is it important that you be able to launch a DRT, then pause, and then start the test? (As opposed to pausing, and then launching DRT and immediately testing)? I've been trying to move to a mode where running tests is mostly stateless in the interface (with stop() provided for cleanup), since the logic of when (and how) you need to restart can be kinda tricky. > > Tools/Scripts/webkitpy/layout_tests/port/test.py:355 > > - return 'junk' > > + return '<path_to_driver>' > > In other places we've used a string like: > > MOCK path_to_driver > Will change. > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:486 > > + cmd.extend(per_test_args) > > This line is different than the one in chromium.py Fixed.
(In reply to comment #5) > (In reply to comment #3) > > (From update of attachment 129163 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=129163&action=review > > > > > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:-599 > > > - def start(self): > > > - if not self._proc: > > > - self._start() > > > > Isn't this used by run-perf-tests? > > > > Ugh. So it is. rniwa, is it important that you be able to launch a DRT, then pause, and then start the test? (As opposed to pausing, and then launching DRT and immediately testing)? Yes. It lets you attach a profiler, debugger, etc... abarth implemented that feature.
(In reply to comment #6) > > Ugh. So it is. rniwa, is it important that you be able to launch a DRT, then pause, and then start the test? (As opposed to pausing, and then launching DRT and immediately testing)? > > Yes. It lets you attach a profiler, debugger, etc... abarth implemented that feature. Ugh, and/or sigh :). Okay, I will figure out some way around this.
Comment on attachment 129172 [details] update w/ review feedback Attachment 129172 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11639503
Created attachment 129348 [details] make driver.start() work again
Please take another look? I don't really like this as a long-term solution, since there's no good way to make driver.start() work with the idea that different tests might require different arguments. Perhaps it would be better to pass in a flag to run_test() to do the pause inside the test (although I don't really like having to push more logic inside that routine, either).
Comment on attachment 129348 [details] make driver.start() work again Attachment 129348 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11714172
Created attachment 129361 [details] patch to make chromium work, fix _start()
Comment on attachment 129361 [details] patch to make chromium work, fix _start() View in context: https://bugs.webkit.org/attachment.cgi?id=129361&action=review > Tools/ChangeLog:14 > + This change adds support for that and removes the no-longer > + needed public Driver.start() method (which no one was calling). This part isn't true anymore, right? > Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:-223 > + driver.start() > if self._options.pause_before_testing: > - driver.start() Why is this change necessary?
Comment on attachment 129361 [details] patch to make chromium work, fix _start() View in context: https://bugs.webkit.org/attachment.cgi?id=129361&action=review >> Tools/ChangeLog:14 >> + needed public Driver.start() method (which no one was calling). > > This part isn't true anymore, right? Right. Will fix; thanks for the review! >> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:-223 >> - driver.start() > > Why is this change necessary? It isn't; I misremembered how the code worked originally. I'll put it back.
This was fixed in http://trac.webkit.org/changeset/109242 . The changelog got confused in a merge :(.