[telemetry] Make default flags passed to WebPageRelay overridable.

[telemetry] Make default flags passed to WebPageRelay overridable via --extra-wpr-args by placing them earlier on the command line.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221538

[pauljensen, 2013-09-03 16:58:16]

Dennis, I'd like to override certain default WPR arguments (--log_level in
particular) via the --extra-wpr-args telemetry argument but this doesn't work
currently because these arguments are inserted last (i.e. after --extra-wpr-args
arguments).  The "Can be overridden if needed" comment confuses me.  What do you
think of this proposed change?  My knowledge of telemetry is very limited, so I
don't really have a good idea of how to reason about the correctness of this
change or how to test it thoroughly.

[dennisjeffrey, 2013-09-03 17:23:28]

Hi Paul - this WPR file is specific to PyAuto tests (PyAuto is a deprecated
test automation framework), and to my knowledge isn't associated with
Telemetry.  How are you using this code?

I'm also cc'ing tonyg@ in case he can help too -- I have just moved to
another team so have limited time to work on chrome-related issues.


On Tue, Sep 3, 2013 at 9:58 AM, <pauljensen@chromium.org> wrote:

> Reviewers: dennis_jeffrey,
>
> Message:
> Dennis, I'd like to override certain default WPR arguments (--log_level in
> particular) via the --extra-wpr-args telemetry argument but this doesn't
> work
> currently because these arguments are inserted last (i.e. after
> --extra-wpr-args
> arguments).  The "Can be overridden if needed" comment confuses me.  What
> do you
> think of this proposed change?  My knowledge of telemetry is very limited,
> so I
> don't really have a good idea of how to reason about the correctness of
> this
> change or how to test it thoroughly.
>
> Description:
> Make default flags passed to WebPageRelay overridable.
>
> Please review this at
https://codereview.chromium.**org/23523017/<https://codereview.chromium.org/2...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>   M chrome/test/functional/**webpagereplay.py
>
>
> Index: chrome/test/functional/**webpagereplay.py
> diff --git a/chrome/test/functional/**webpagereplay.py
> b/chrome/test/functional/**webpagereplay.py
> index 0cfba58cf55cb7b534899c0f3ca3a1**544df3d050..**
> 8b0ca9303317836c958974f35cf5b7**1a460e86a2 100755
> --- a/chrome/test/functional/**webpagereplay.py
> +++ b/chrome/test/functional/**webpagereplay.py
> @@ -120,14 +120,14 @@ class ReplayServer(object):
>
>    def _AddDefaultReplayOptions(self)**:
>      """Set WPR command-line options. Can be overridden if needed."""
> -    self.replay_options += [
> +    self.replay_options = [
>          '--host', str(self._replay_host),
>          '--port', str(self._http_port),
>          '--ssl_port', str(self._https_port),
>          '--use_closest_match',
>          '--no-dns_forwarding',
>          '--log_level', 'warning'
> -        ]
> +        ] + self.replay_options
>
>    def _CheckPath(self, label, path):
>      if not os.path.exists(path):
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[pauljensen, 2013-09-04 17:40:17]

chrome/test/functional/webpagereplay.py is imported by
tools/telemetry/telemetry/core/wpr_server.py and is used by Telemetry to
start/control WebPageReplay.

On 2013/09/03 17:23:28, dennisjeffrey wrote:
> Hi Paul - this WPR file is specific to PyAuto tests (PyAuto is a deprecated
> test automation framework), and to my knowledge isn't associated with
> Telemetry.  How are you using this code?
> 
> I'm also cc'ing tonyg@ in case he can help too -- I have just moved to
> another team so have limited time to work on chrome-related issues.
> 
> 
> On Tue, Sep 3, 2013 at 9:58 AM, <mailto:pauljensen@chromium.org> wrote:
> 
> > Reviewers: dennis_jeffrey,
> >
> > Message:
> > Dennis, I'd like to override certain default WPR arguments (--log_level in
> > particular) via the --extra-wpr-args telemetry argument but this doesn't
> > work
> > currently because these arguments are inserted last (i.e. after
> > --extra-wpr-args
> > arguments).  The "Can be overridden if needed" comment confuses me.  What
> > do you
> > think of this proposed change?  My knowledge of telemetry is very limited,
> > so I
> > don't really have a good idea of how to reason about the correctness of
> > this
> > change or how to test it thoroughly.
> >
> > Description:
> > Make default flags passed to WebPageRelay overridable.
> >
> > Please review this at
>
https://codereview.chromium.**org/23523017/%3Chttps://codereview.chromium.org...>
> >
> > SVN Base:
>
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
> >
> > Affected files:
> >   M chrome/test/functional/**webpagereplay.py
> >
> >
> > Index: chrome/test/functional/**webpagereplay.py
> > diff --git a/chrome/test/functional/**webpagereplay.py
> > b/chrome/test/functional/**webpagereplay.py
> > index 0cfba58cf55cb7b534899c0f3ca3a1**544df3d050..**
> > 8b0ca9303317836c958974f35cf5b7**1a460e86a2 100755
> > --- a/chrome/test/functional/**webpagereplay.py
> > +++ b/chrome/test/functional/**webpagereplay.py
> > @@ -120,14 +120,14 @@ class ReplayServer(object):
> >
> >    def _AddDefaultReplayOptions(self)**:
> >      """Set WPR command-line options. Can be overridden if needed."""
> > -    self.replay_options += [
> > +    self.replay_options = [
> >          '--host', str(self._replay_host),
> >          '--port', str(self._http_port),
> >          '--ssl_port', str(self._https_port),
> >          '--use_closest_match',
> >          '--no-dns_forwarding',
> >          '--log_level', 'warning'
> > -        ]
> > +        ] + self.replay_options
> >
> >    def _CheckPath(self, label, path):
> >      if not os.path.exists(path):
> >
> >
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[dennis_jeffrey, 2013-09-04 17:51:34]

On 2013/09/04 17:40:17, pauljensen wrote:
> chrome/test/functional/webpagereplay.py is imported by
> tools/telemetry/telemetry/core/wpr_server.py and is used by Telemetry to
> start/control WebPageReplay.
> 
> On 2013/09/03 17:23:28, dennisjeffrey wrote:
> > Hi Paul - this WPR file is specific to PyAuto tests (PyAuto is a deprecated
> > test automation framework), and to my knowledge isn't associated with
> > Telemetry.  How are you using this code?
> > 
> > I'm also cc'ing tonyg@ in case he can help too -- I have just moved to
> > another team so have limited time to work on chrome-related issues.
> > 
> > 
> > On Tue, Sep 3, 2013 at 9:58 AM, <mailto:pauljensen@chromium.org> wrote:
> > 
> > > Reviewers: dennis_jeffrey,
> > >
> > > Message:
> > > Dennis, I'd like to override certain default WPR arguments (--log_level in
> > > particular) via the --extra-wpr-args telemetry argument but this doesn't
> > > work
> > > currently because these arguments are inserted last (i.e. after
> > > --extra-wpr-args
> > > arguments).  The "Can be overridden if needed" comment confuses me.  What
> > > do you
> > > think of this proposed change?  My knowledge of telemetry is very limited,
> > > so I
> > > don't really have a good idea of how to reason about the correctness of
> > > this
> > > change or how to test it thoroughly.
> > >
> > > Description:
> > > Make default flags passed to WebPageRelay overridable.
> > >
> > > Please review this at
> >
>
https://codereview.chromium.**org/23523017/%253Chttps://codereview.chromium.o...>
> > >
> > > SVN Base:
> >
>
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
> > >
> > > Affected files:
> > >   M chrome/test/functional/**webpagereplay.py
> > >
> > >
> > > Index: chrome/test/functional/**webpagereplay.py
> > > diff --git a/chrome/test/functional/**webpagereplay.py
> > > b/chrome/test/functional/**webpagereplay.py
> > > index 0cfba58cf55cb7b534899c0f3ca3a1**544df3d050..**
> > > 8b0ca9303317836c958974f35cf5b7**1a460e86a2 100755
> > > --- a/chrome/test/functional/**webpagereplay.py
> > > +++ b/chrome/test/functional/**webpagereplay.py
> > > @@ -120,14 +120,14 @@ class ReplayServer(object):
> > >
> > >    def _AddDefaultReplayOptions(self)**:
> > >      """Set WPR command-line options. Can be overridden if needed."""
> > > -    self.replay_options += [
> > > +    self.replay_options = [
> > >          '--host', str(self._replay_host),
> > >          '--port', str(self._http_port),
> > >          '--ssl_port', str(self._https_port),
> > >          '--use_closest_match',
> > >          '--no-dns_forwarding',
> > >          '--log_level', 'warning'
> > > -        ]
> > > +        ] + self.replay_options
> > >
> > >    def _CheckPath(self, label, path):
> > >      if not os.path.exists(path):
> > >
> > >
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

Ah, I see.  However, from my reading of the code, wpr_server.py is overriding
the _AddDefaultReplayOptions() function (originally defined in webpagereplay.py
in your CL) in class _WebPageReplayServer.  So wouldn't the proposed change in
this CL essentially be a no-op from Telemetry's point of view?

Also, Tony, maybe it's time to move webpagereplay.py into a more
Telemetry-specific location? (I see there's a TODO in wpr_server.py: "#
TODO(tonyg): Move webpagereplay.py to a common location.").  I don't believe
this file is useful any more to the deprecated PyAuto tests.

[pauljensen, 2013-09-04 18:33:03]

On 2013/09/04 17:51:34, dennis_jeffrey wrote:
> On 2013/09/04 17:40:17, pauljensen wrote:
> > chrome/test/functional/webpagereplay.py is imported by
> > tools/telemetry/telemetry/core/wpr_server.py and is used by Telemetry to
> > start/control WebPageReplay.
> > 
> > On 2013/09/03 17:23:28, dennisjeffrey wrote:
> > > Hi Paul - this WPR file is specific to PyAuto tests (PyAuto is a
deprecated
> > > test automation framework), and to my knowledge isn't associated with
> > > Telemetry.  How are you using this code?
> > > 
> > > I'm also cc'ing tonyg@ in case he can help too -- I have just moved to
> > > another team so have limited time to work on chrome-related issues.
> > > 
> > > 
> > > On Tue, Sep 3, 2013 at 9:58 AM, <mailto:pauljensen@chromium.org> wrote:
> > > 
> > > > Reviewers: dennis_jeffrey,
> > > >
> > > > Message:
> > > > Dennis, I'd like to override certain default WPR arguments (--log_level
in
> > > > particular) via the --extra-wpr-args telemetry argument but this doesn't
> > > > work
> > > > currently because these arguments are inserted last (i.e. after
> > > > --extra-wpr-args
> > > > arguments).  The "Can be overridden if needed" comment confuses me. 
What
> > > > do you
> > > > think of this proposed change?  My knowledge of telemetry is very
limited,
> > > > so I
> > > > don't really have a good idea of how to reason about the correctness of
> > > > this
> > > > change or how to test it thoroughly.
> > > >
> > > > Description:
> > > > Make default flags passed to WebPageRelay overridable.
> > > >
> > > > Please review this at
> > >
> >
>
https://codereview.chromium.**org/23523017/%25253Chttps://codereview.chromium...>
> > > >
> > > > SVN Base:
> > >
> >
>
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
> > > >
> > > > Affected files:
> > > >   M chrome/test/functional/**webpagereplay.py
> > > >
> > > >
> > > > Index: chrome/test/functional/**webpagereplay.py
> > > > diff --git a/chrome/test/functional/**webpagereplay.py
> > > > b/chrome/test/functional/**webpagereplay.py
> > > > index 0cfba58cf55cb7b534899c0f3ca3a1**544df3d050..**
> > > > 8b0ca9303317836c958974f35cf5b7**1a460e86a2 100755
> > > > --- a/chrome/test/functional/**webpagereplay.py
> > > > +++ b/chrome/test/functional/**webpagereplay.py
> > > > @@ -120,14 +120,14 @@ class ReplayServer(object):
> > > >
> > > >    def _AddDefaultReplayOptions(self)**:
> > > >      """Set WPR command-line options. Can be overridden if needed."""
> > > > -    self.replay_options += [
> > > > +    self.replay_options = [
> > > >          '--host', str(self._replay_host),
> > > >          '--port', str(self._http_port),
> > > >          '--ssl_port', str(self._https_port),
> > > >          '--use_closest_match',
> > > >          '--no-dns_forwarding',
> > > >          '--log_level', 'warning'
> > > > -        ]
> > > > +        ] + self.replay_options
> > > >
> > > >    def _CheckPath(self, label, path):
> > > >      if not os.path.exists(path):
> > > >
> > > >
> > > >
> > > 
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> Ah, I see.  However, from my reading of the code, wpr_server.py is overriding
> the _AddDefaultReplayOptions() function (originally defined in
webpagereplay.py
> in your CL) in class _WebPageReplayServer.  So wouldn't the proposed change in
> this CL essentially be a no-op from Telemetry's point of view?
> 
> Also, Tony, maybe it's time to move webpagereplay.py into a more
> Telemetry-specific location? (I see there's a TODO in wpr_server.py: "#
> TODO(tonyg): Move webpagereplay.py to a common location.").  I don't believe
> this file is useful any more to the deprecated PyAuto tests.

As of r221047 which landed yesterday wpr_server.py no longer overrides
_AddDefaultReplayOptions() so my change needs to happen here.

[dennis_jeffrey, 2013-09-04 18:43:12]

I'm ok with this change because I'm not aware of any problems that will arise
from changing the order of arguments in the "self.replay_options" list. 
However, I am not familiar enough with the potential effect on Telemetry itself
to be comfortable approving this change, so am deferring final approval to Tony,
one of the Telemetry OWNERS.

[tonyg, 2013-09-05 00:19:58]

lgtm

[commit-bot: I haz the power, 2013-09-05 16:05:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pauljensen@chromium.org/23523017/1

[commit-bot: I haz the power, 2013-09-05 16:19:23]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[pauljensen, 2013-09-05 16:40:02]

Dennis, could you provide the OWNERS LGTM?

[dennis_jeffrey, 2013-09-05 16:42:31]

LGTM

[commit-bot: I haz the power, 2013-09-05 16:49:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pauljensen@chromium.org/23523017/1

[commit-bot: I haz the power, 2013-09-05 23:14:53]

Change committed as 221538