[chromedriver] Add Network Conditions Override Manager and tests

Add Network Conditions Override Manager and tests

BUG=https://code.google.com/p/chromedriver/issues/detail?id=984

Committed: https://crrev.com/12a55ff7369b35adb29cd28f5e3dd4ef923e3e6c
Cr-Commit-Position: refs/heads/master@{#319668}

[srawlins, 2015-01-28 19:51:52]

srawlins@google.com changed reviewers:
+ samuong@chromium.org

[srawlins, 2015-01-28 19:51:52]

Hi Sam, I've got the basic override manager working (tested). While I'm working
on the command, would you mind looking confirming good names, like:

class NetworkConditionsOverrideManager - this makes
network_conditions_override_manager_unittests.cc the longest-named file in
chromedriver/chrome :(

struct NetworkConditions - hopefully not awkward or conflicty.

I get "conditions" from places like OverridesUI.js [1] and inspector.js [2] but
I'd be happy to change it to something different ("network settings", "network
limits", "network throttles", ...).

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/devt...

[samuong, 2015-01-28 22:14:51]

On 2015/01/28 19:51:52, srawlins wrote:
> Hi Sam, I've got the basic override manager working (tested). While I'm
working
> on the command, would you mind looking confirming good names, like:
> 
> class NetworkConditionsOverrideManager - this makes
> network_conditions_override_manager_unittests.cc the longest-named file in
> chromedriver/chrome :(
> 
> struct NetworkConditions - hopefully not awkward or conflicty.
> 
> I get "conditions" from places like OverridesUI.js [1] and inspector.js [2]
but
> I'd be happy to change it to something different ("network settings", "network
> limits", "network throttles", ...).
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> [2]
>
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/devt...

How about just calling it NetworkOverrideManager? You could leave
NetworkConditions as is if you think Network is too vague.

[srawlins, 2015-02-05 20:10:07]

OK Sam, this is a working implementation. Qualifiers:

* Offline is disabled. I'm filing a bug for how this severs the connection to
Chromedriver.
* There is a weird speed test in the python tests, validating throughput. But
it's pretty lenient, and AFAIK it doesn't flake. Let me know what you think.
* I don't have the network names yet, like "EDGE" and "WiFi".

[samuong, 2015-02-11 23:22:14]

https://codereview.chromium.org/883083002/diff/20001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/network_conditions_override_manager.h
(right):

https://codereview.chromium.org/883083002/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/network_conditions_override_manager.h:42:
scoped_ptr<NetworkConditions> overridden_network_conditions_;
Does NetworkConditionsOverrideManager need to make/own a copy of the
NetworkConditions? This is already in the Session object, and the
NetworkConditionsOverrideManager's lifetime should never be longer than the
Session's lifetime, so I think a non-scoped pointer will do here? Let me know if
I've missed something...

https://codereview.chromium.org/883083002/diff/20001/chrome/test/chromedriver...
File
chrome/test/chromedriver/chrome/network_conditions_override_manager_unittest.cc
(right):

https://codereview.chromium.org/883083002/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/network_conditions_override_manager_unittest.cc:58:
};
Copies of Command and RecorderDevToolsClient are defined used in a lot of
places. Can we extract this out and share it, rather than copy and paste it? I
think it makes sense to create recorder_devtools_client.cc/h and #include it
from the appropriate *_unittest.cc files. We should only link it into the test
targets, not the main chromedriver one.

https://codereview.chromium.org/883083002/diff/20001/chrome/test/chromedriver...
File chrome/test/chromedriver/client/chromedriver.py (right):

https://codereview.chromium.org/883083002/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/client/chromedriver.py:363: # Until crbug XXXXX is
resolved, we must always set 'offline' to False,
Now that you've got an actual bug for this, the XXXXX can be replaced with the
bug id. Also a URL makes it easier to copy-and-paste (i.e. crbug.com/XXXXX).

https://codereview.chromium.org/883083002/diff/20001/chrome/test/chromedriver...
File chrome/test/chromedriver/server/http_handler.cc (right):

https://codereview.chromium.org/883083002/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/server/http_handler.cc:375:
"session/:sessionId/network_conditions",
Let's put this behind a vendor-specific URL (see
https://w3c.github.io/webdriver/webdriver-spec.html#extending-the-json-http-p...)

https://codereview.chromium.org/883083002/diff/20001/chrome/test/chromedriver...
File chrome/test/chromedriver/test/run_py_tests.py (right):

https://codereview.chromium.org/883083002/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/test/run_py_tests.py:810:
self.assertGreaterEqual(actual_throughput_kbps, throughput_kbps / 1.333)
I'm a bit concerned about flakiness for these tests, but I don't have any ideas
on how to improve this right now. Let's leave it for now and see how problematic
it becomes - we can disable it if it ends up being flaky.

Is there any reason to not assert on latency?

[srawlins, 2015-02-25 22:40:57]

OK I pulled RecorderDevToolsClient into its own shared class. Command is sitting
at the top of that file now. What do you think?

https://codereview.chromium.org/883083002/diff/20001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/network_conditions_override_manager.h
(right):

https://codereview.chromium.org/883083002/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/network_conditions_override_manager.h:42:
scoped_ptr<NetworkConditions> overridden_network_conditions_;
On 2015/02/11 23:22:14, samuong wrote:
> Does NetworkConditionsOverrideManager need to make/own a copy of the
> NetworkConditions? This is already in the Session object, and the
> NetworkConditionsOverrideManager's lifetime should never be longer than the
> Session's lifetime, so I think a non-scoped pointer will do here? Let me know
if
> I've missed something...

Done.

https://codereview.chromium.org/883083002/diff/20001/chrome/test/chromedriver...
File
chrome/test/chromedriver/chrome/network_conditions_override_manager_unittest.cc
(right):

https://codereview.chromium.org/883083002/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/network_conditions_override_manager_unittest.cc:58:
};
On 2015/02/11 23:22:14, samuong wrote:
> Copies of Command and RecorderDevToolsClient are defined used in a lot of
> places. Can we extract this out and share it, rather than copy and paste it? I
> think it makes sense to create recorder_devtools_client.cc/h and #include it
> from the appropriate *_unittest.cc files. We should only link it into the test
> targets, not the main chromedriver one.

Great idea. Done. I pulled it out of 4 different unittests.cc

https://codereview.chromium.org/883083002/diff/20001/chrome/test/chromedriver...
File chrome/test/chromedriver/client/chromedriver.py (right):

https://codereview.chromium.org/883083002/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/client/chromedriver.py:363: # Until crbug XXXXX is
resolved, we must always set 'offline' to False,
On 2015/02/11 23:22:14, samuong wrote:
> Now that you've got an actual bug for this, the XXXXX can be replaced with the
> bug id. Also a URL makes it easier to copy-and-paste (i.e. crbug.com/XXXXX).

Done.

https://codereview.chromium.org/883083002/diff/20001/chrome/test/chromedriver...
File chrome/test/chromedriver/server/http_handler.cc (right):

https://codereview.chromium.org/883083002/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/server/http_handler.cc:375:
"session/:sessionId/network_conditions",
On 2015/02/11 23:22:14, samuong wrote:
> Let's put this behind a vendor-specific URL (see
>
https://w3c.github.io/webdriver/webdriver-spec.html#extending-the-json-http-p...)

Done.

https://codereview.chromium.org/883083002/diff/20001/chrome/test/chromedriver...
File chrome/test/chromedriver/test/run_py_tests.py (right):

https://codereview.chromium.org/883083002/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/test/run_py_tests.py:810:
self.assertGreaterEqual(actual_throughput_kbps, throughput_kbps / 1.333)
On 2015/02/11 23:22:14, samuong wrote:
> I'm a bit concerned about flakiness for these tests, but I don't have any
ideas
> on how to improve this right now. Let's leave it for now and see how
problematic
> it becomes - we can disable it if it ends up being flaky.
> 
> Is there any reason to not assert on latency?

I can presumably add a latency check as well that is equally not flaky :).
Should we do that?

[srawlins, 2015-02-26 21:18:24]

Sorry, I introduced a scoping bug yesterday that wasn't caught with unit tests.
I fixed it today, as well as changed the types of the throughput and latency
fields to doubles, since that's what they really are in chrome:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dev...

[samuong, 2015-02-26 23:07:44]

Nice find with the scoping bug; I didn't notice that the first time I looked
through this.

https://codereview.chromium.org/883083002/diff/60001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/network_conditions_override_manager.cc
(right):

https://codereview.chromium.org/883083002/diff/60001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/network_conditions_override_manager.cc:25:
return ApplyOverrideIfNeeded();
I find it a bit strange that NetworkConditionsOverrideManager will save a
pointer to the overridden network conditions even if it fails to set them.

This could lead to weird situations if we try to set the network conditions, but
fail and return an error to the user. If the test case catches the WebDriver
exception and continues, we will still try to apply the failed override on the
next DevTools event, and if we succeed then this leads to unintuitive behavior.

How about we change ApplyOverrideIfNeeded() so that it takes an argument, and
then only set |overridden_network_condtiions_| if it succeeded?

Status ApplyOverride(const NetworkConditions* overridden_network_conditions);

[srawlins, 2015-03-04 00:34:29]

https://codereview.chromium.org/883083002/diff/60001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/network_conditions_override_manager.cc
(right):

https://codereview.chromium.org/883083002/diff/60001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/network_conditions_override_manager.cc:25:
return ApplyOverrideIfNeeded();
On 2015/02/26 23:07:44, samuong wrote:
> I find it a bit strange that NetworkConditionsOverrideManager will save a
> pointer to the overridden network conditions even if it fails to set them.
> 
> This could lead to weird situations if we try to set the network conditions,
but
> fail and return an error to the user. If the test case catches the WebDriver
> exception and continues, we will still try to apply the failed override on the
> next DevTools event, and if we succeed then this leads to unintuitive
behavior.
> 
> How about we change ApplyOverrideIfNeeded() so that it takes an argument, and
> then only set |overridden_network_condtiions_| if it succeeded?
> 
> Status ApplyOverride(const NetworkConditions* overridden_network_conditions);

ApplyOverrideIfNeeded is also used in the OnConnected callback, and OnEvent when
the event is Page.frameNavigated, which would not be able to provide such an
argument.

I can break ApplyOverrideIfNeeded into a simple ApplyOverrideIfNeeded method
which would call a new ApplyOverride method, which would take the argument like
you suggest. Then OverrideNetworkConditions can call just ApplyOverride, and
then only set |overridden_network_conditions_| if succeeded as you suggest.

How about that?

[samuong, 2015-03-04 06:37:58]

https://codereview.chromium.org/883083002/diff/60001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/network_conditions_override_manager.cc
(right):

https://codereview.chromium.org/883083002/diff/60001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/network_conditions_override_manager.cc:25:
return ApplyOverrideIfNeeded();
On 2015/03/04 00:34:29, srawlins wrote:
> On 2015/02/26 23:07:44, samuong wrote:
> > I find it a bit strange that NetworkConditionsOverrideManager will save a
> > pointer to the overridden network conditions even if it fails to set them.
> > 
> > This could lead to weird situations if we try to set the network conditions,
> but
> > fail and return an error to the user. If the test case catches the WebDriver
> > exception and continues, we will still try to apply the failed override on
the
> > next DevTools event, and if we succeed then this leads to unintuitive
> behavior.
> > 
> > How about we change ApplyOverrideIfNeeded() so that it takes an argument,
and
> > then only set |overridden_network_condtiions_| if it succeeded?
> > 
> > Status ApplyOverride(const NetworkConditions*
overridden_network_conditions);
> 
> ApplyOverrideIfNeeded is also used in the OnConnected callback, and OnEvent
when
> the event is Page.frameNavigated, which would not be able to provide such an
> argument.
> 
> I can break ApplyOverrideIfNeeded into a simple ApplyOverrideIfNeeded method
> which would call a new ApplyOverride method, which would take the argument
like
> you suggest. Then OverrideNetworkConditions can call just ApplyOverride, and
> then only set |overridden_network_conditions_| if succeeded as you suggest.
> 
> How about that?

Sure. I think we're on the same page here, except that I was suggesting that you
inline your ApplyOverrideIfNeeded message.

[srawlins, 2015-03-04 16:16:01]

On 2015/03/04 06:37:58, samuong wrote:
>
https://codereview.chromium.org/883083002/diff/60001/chrome/test/chromedriver...
> File chrome/test/chromedriver/chrome/network_conditions_override_manager.cc
> (right):
> 
>
https://codereview.chromium.org/883083002/diff/60001/chrome/test/chromedriver...
> chrome/test/chromedriver/chrome/network_conditions_override_manager.cc:25:
> return ApplyOverrideIfNeeded();
> On 2015/03/04 00:34:29, srawlins wrote:
> > On 2015/02/26 23:07:44, samuong wrote:
> > > I find it a bit strange that NetworkConditionsOverrideManager will save a
> > > pointer to the overridden network conditions even if it fails to set them.
> > > 
> > > This could lead to weird situations if we try to set the network
conditions,
> > but
> > > fail and return an error to the user. If the test case catches the
WebDriver
> > > exception and continues, we will still try to apply the failed override on
> the
> > > next DevTools event, and if we succeed then this leads to unintuitive
> > behavior.
> > > 
> > > How about we change ApplyOverrideIfNeeded() so that it takes an argument,
> and
> > > then only set |overridden_network_condtiions_| if it succeeded?
> > > 
> > > Status ApplyOverride(const NetworkConditions*
> overridden_network_conditions);
> > 
> > ApplyOverrideIfNeeded is also used in the OnConnected callback, and OnEvent
> when
> > the event is Page.frameNavigated, which would not be able to provide such an
> > argument.
> > 
> > I can break ApplyOverrideIfNeeded into a simple ApplyOverrideIfNeeded method
> > which would call a new ApplyOverride method, which would take the argument
> like
> > you suggest. Then OverrideNetworkConditions can call just ApplyOverride, and
> > then only set |overridden_network_conditions_| if succeeded as you suggest.
> > 
> > How about that?
> 
> Sure. I think we're on the same page here, except that I was suggesting that
you
> inline your ApplyOverrideIfNeeded message.

Okie doke, done.

[srawlins, 2015-03-05 00:54:14]

Added a filtered-out test for offline.

[samuong, 2015-03-05 23:43:21]

lgtm with nits

https://codereview.chromium.org/883083002/diff/100001/chrome/test/chromedrive...
File chrome/test/chromedriver/chrome/network_conditions_override_manager.cc
(right):

https://codereview.chromium.org/883083002/diff/100001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/network_conditions_override_manager.cc:27: 
nit: delete blank line

https://codereview.chromium.org/883083002/diff/100001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/network_conditions_override_manager.cc:50: 
nit: delete blank line

https://codereview.chromium.org/883083002/diff/100001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/network_conditions_override_manager.cc:56: 
nit: delete blank line

https://codereview.chromium.org/883083002/diff/100001/chrome/test/chromedrive...
File chrome/test/chromedriver/test/run_py_tests.py (right):

https://codereview.chromium.org/883083002/diff/100001/chrome/test/chromedrive...
chrome/test/chromedriver/test/run_py_tests.py:828: duration = finish-start
nit: spaces around the '-'

[srawlins, 2015-03-05 23:50:22]

https://codereview.chromium.org/883083002/diff/100001/chrome/test/chromedrive...
File chrome/test/chromedriver/chrome/network_conditions_override_manager.cc
(right):

https://codereview.chromium.org/883083002/diff/100001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/network_conditions_override_manager.cc:27: 
On 2015/03/05 23:43:20, samuong wrote:
> nit: delete blank line

Done.

https://codereview.chromium.org/883083002/diff/100001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/network_conditions_override_manager.cc:50: 
On 2015/03/05 23:43:20, samuong wrote:
> nit: delete blank line

Done.

https://codereview.chromium.org/883083002/diff/100001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/network_conditions_override_manager.cc:56: 
On 2015/03/05 23:43:20, samuong wrote:
> nit: delete blank line

Done.

https://codereview.chromium.org/883083002/diff/100001/chrome/test/chromedrive...
File chrome/test/chromedriver/test/run_py_tests.py (right):

https://codereview.chromium.org/883083002/diff/100001/chrome/test/chromedrive...
chrome/test/chromedriver/test/run_py_tests.py:828: duration = finish-start
On 2015/03/05 23:43:20, samuong wrote:
> nit: spaces around the '-'

Done.

[srawlins, 2015-03-05 23:51:59]

The CQ bit was checked by srawlins@google.com

[srawlins, 2015-03-05 23:52:00]

The patchset sent to the CQ was uploaded after l-g-t-m from samuong@chromium.org
Link to the patchset: https://codereview.chromium.org/883083002/#ps120001
(title: "nits")

[commit-bot: I haz the power, 2015-03-05 23:53:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/883083002/120001

[commit-bot: I haz the power, 2015-03-06 02:42:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-06 02:42:25]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[srawlins, 2015-03-07 00:34:44]

The CQ bit was checked by srawlins@google.com

[srawlins, 2015-03-07 00:34:44]

The patchset sent to the CQ was uploaded after l-g-t-m from samuong@chromium.org
Link to the patchset: https://codereview.chromium.org/883083002/#ps140001
(title: "Fix warning")

[commit-bot: I haz the power, 2015-03-07 00:36:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/883083002/140001

[commit-bot: I haz the power, 2015-03-07 02:37:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-07 02:37:10]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
Timed out jobs are not retried to avoid causing additional load on the builders
with large pending queues.

[srawlins, 2015-03-09 16:44:11]

The CQ bit was checked by srawlins@google.com

[commit-bot: I haz the power, 2015-03-09 16:44:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/883083002/140001

[commit-bot: I haz the power, 2015-03-09 17:30:52]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-03-09 17:31:29]

Patchset 8 (id:??) landed as
https://crrev.com/12a55ff7369b35adb29cd28f5e3dd4ef923e3e6c
Cr-Commit-Position: refs/heads/master@{#319668}