Bug 79733 – nrwt: port/Driver needs to support per-test command line args

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

nrwt: port/Driver needs to support per-test command line args

[Dirk Pranke]

Created attachment 129163 [details]
Patch

[Dirk Pranke]

Created attachment 129165 [details]
remove a couple start() methods I missed

[Adam Barth]

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

[Dirk Pranke]

Created attachment 129172 [details]
update w/ review feedback

[Dirk Pranke]

(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.

[Ryosuke Niwa]

(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.

[Dirk Pranke]

(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.

[WebKit Review Bot]

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

[Dirk Pranke]

Created attachment 129348 [details]
make driver.start() work again

[Dirk Pranke]

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).

[WebKit Review Bot]

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

[Dirk Pranke]

Created attachment 129361 [details]
patch to make chromium work, fix _start()

[Adam Barth]

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?

[Dirk Pranke]

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.

[Dirk Pranke]

This was fixed in http://trac.webkit.org/changeset/109242 . The changelog got confused in a merge :(.