Implement the LitePage Integration Test with ChromeDriver

Implement the LitePage Integration Test with ChromeDriver

BUG=680567
Review-Url: https://codereview.chromium.org/2717563002
Cr-Commit-Position: refs/heads/master@{#453279}
Committed: https://chromium.googlesource.com/chromium/src/+/e72a513a6c84075089ecda0570ce658d0b45f6dd

[megjablon, 2017-02-23 20:49:36]

Patchset #1 (id:1) has been deleted

[megjablon, 2017-02-23 22:07:14]

Patchset #1 (id:20001) has been deleted

[megjablon, 2017-02-23 23:31:14]

megjablon@chromium.org changed reviewers:
+ tbansal@chromium.org

[megjablon, 2017-02-23 23:31:14]

PTAL, thanks!

[tbansal1, 2017-02-23 23:35:21]

https://codereview.chromium.org/2717563002/diff/40001/tools/chrome_proxy/webd...
File tools/chrome_proxy/webdriver/lofi.py (right):

https://codereview.chromium.org/2717563002/diff/40001/tools/chrome_proxy/webd...
tools/chrome_proxy/webdriver/lofi.py:52:
test_driver.AddChromeArg('--disable-quic')
Why disable quic?

https://codereview.chromium.org/2717563002/diff/40001/tools/chrome_proxy/webd...
tools/chrome_proxy/webdriver/lofi.py:69: self.assertTrue('lite-page' in
cpat_request)
Should not these 2 assertions be always true (regardless of whether lite-page
was present in cpct-response)?

If yes, then you can move these out of the if-conditional.

[Robert Ogden, 2017-02-23 23:47:47]

robertogden@chromium.org changed reviewers:
+ robertogden@chromium.org

[Robert Ogden, 2017-02-23 23:47:49]

https://codereview.chromium.org/2717563002/diff/60001/tools/chrome_proxy/webd...
File tools/chrome_proxy/webdriver/lofi.py (right):

https://codereview.chromium.org/2717563002/diff/60001/tools/chrome_proxy/webd...
tools/chrome_proxy/webdriver/lofi.py:58: if '/csi?' in response.url:
Why is this needed? Maybe leave a comment.

https://codereview.chromium.org/2717563002/diff/60001/tools/chrome_proxy/webd...
tools/chrome_proxy/webdriver/lofi.py:67:
self.assertTrue('exp=ignore_preview_blacklist' in
Use assertIn
https://docs.python.org/2/library/unittest.html#unittest.TestCase.assertIn

[megjablon, 2017-02-23 23:47:51]

megjablon@chromium.org changed reviewers:
- robertogden@chromium.org

[megjablon, 2017-02-23 23:47:52]

https://codereview.chromium.org/2717563002/diff/40001/tools/chrome_proxy/webd...
File tools/chrome_proxy/webdriver/lofi.py (right):

https://codereview.chromium.org/2717563002/diff/40001/tools/chrome_proxy/webd...
tools/chrome_proxy/webdriver/lofi.py:52:
test_driver.AddChromeArg('--disable-quic')
On 2017/02/23 23:35:21, tbansal1 wrote:
> Why disable quic?

There was some reason you disabled it in the telemetry test. Not 100% sure why
https://codereview.chromium.org/2357723002

https://codereview.chromium.org/2717563002/diff/40001/tools/chrome_proxy/webd...
tools/chrome_proxy/webdriver/lofi.py:69: self.assertTrue('lite-page' in
cpat_request)
On 2017/02/23 23:35:21, tbansal1 wrote:
> Should not these 2 assertions be always true (regardless of whether lite-page
> was present in cpct-response)?
> 
> If yes, then you can move these out of the if-conditional.

For the first one, no, we should only request on the main frame so there should
only really be one request and response with lite-page directives. Changed the
very last assertion in the test based on this.

Second one I moved outside.

[megjablon, 2017-02-23 23:53:30]

Patchset #3 (id:80001) has been deleted

[Robert Ogden, 2017-02-23 23:53:33]

robertogden@chromium.org changed reviewers:
+ robertogden@chromium.org

[Robert Ogden, 2017-02-23 23:53:34]

lgtm, thanks!

[tbansal1, 2017-02-23 23:54:05]

lgtm % disable-quic flag is removed.

https://codereview.chromium.org/2717563002/diff/40001/tools/chrome_proxy/webd...
File tools/chrome_proxy/webdriver/lofi.py (right):

https://codereview.chromium.org/2717563002/diff/40001/tools/chrome_proxy/webd...
tools/chrome_proxy/webdriver/lofi.py:52:
test_driver.AddChromeArg('--disable-quic')
On 2017/02/23 23:47:52, megjablon wrote:
> On 2017/02/23 23:35:21, tbansal1 wrote:
> > Why disable quic?
> 
> There was some reason you disabled it in the telemetry test. Not 100% sure why
> https://codereview.chromium.org/2357723002

It was necessary for older testing framework. It should not be required now.

https://codereview.chromium.org/2717563002/diff/40001/tools/chrome_proxy/webd...
tools/chrome_proxy/webdriver/lofi.py:69: self.assertTrue('lite-page' in
cpat_request)
On 2017/02/23 23:47:52, megjablon wrote:
> On 2017/02/23 23:35:21, tbansal1 wrote:
> > Should not these 2 assertions be always true (regardless of whether
lite-page
> > was present in cpct-response)?
> > 
> > If yes, then you can move these out of the if-conditional.
> 
> For the first one, no, we should only request on the main frame so there
should
> only really be one request and response with lite-page directives. Changed the
> very last assertion in the test based on this.
> 
> Second one I moved outside. 

Acknowledged.

[megjablon, 2017-02-23 23:57:29]

Patchset #3 (id:100001) has been deleted

[megjablon, 2017-02-23 23:58:18]

https://codereview.chromium.org/2717563002/diff/40001/tools/chrome_proxy/webd...
File tools/chrome_proxy/webdriver/lofi.py (right):

https://codereview.chromium.org/2717563002/diff/40001/tools/chrome_proxy/webd...
tools/chrome_proxy/webdriver/lofi.py:52:
test_driver.AddChromeArg('--disable-quic')
On 2017/02/23 23:54:05, tbansal1 wrote:
> On 2017/02/23 23:47:52, megjablon wrote:
> > On 2017/02/23 23:35:21, tbansal1 wrote:
> > > Why disable quic?
> > 
> > There was some reason you disabled it in the telemetry test. Not 100% sure
why
> > https://codereview.chromium.org/2357723002
> 
> It was necessary for older testing framework. It should not be required now.

Good to know. Done.

https://codereview.chromium.org/2717563002/diff/60001/tools/chrome_proxy/webd...
File tools/chrome_proxy/webdriver/lofi.py (right):

https://codereview.chromium.org/2717563002/diff/60001/tools/chrome_proxy/webd...
tools/chrome_proxy/webdriver/lofi.py:58: if '/csi?' in response.url:
On 2017/02/23 23:47:48, Robert Ogden wrote:
> Why is this needed? Maybe leave a comment.

Done.

https://codereview.chromium.org/2717563002/diff/60001/tools/chrome_proxy/webd...
tools/chrome_proxy/webdriver/lofi.py:67:
self.assertTrue('exp=ignore_preview_blacklist' in
On 2017/02/23 23:47:48, Robert Ogden wrote:
> Use assertIn
> https://docs.python.org/2/library/unittest.html#unittest.TestCase.assertIn

Done.

[megjablon, 2017-02-25 00:46:05]

The CQ bit was checked by megjablon@chromium.org

[megjablon, 2017-02-25 00:46:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertogden@chromium.org, tbansal@chromium.org
Link to the patchset: https://codereview.chromium.org/2717563002/#ps120001
(title: "comments")

[commit-bot: I haz the power, 2017-02-25 00:46:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-25 00:48:09]

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

[commit-bot: I haz the power, 2017-02-25 00:48:11]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[megjablon, 2017-02-27 18:51:57]

The CQ bit was checked by megjablon@chromium.org

[commit-bot: I haz the power, 2017-02-27 18:52:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-27 19:05:45]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488221517761370,
"parent_rev": "d8e1c0b6e41d77182b9500179a49392c9a245a34", "commit_rev":
"e72a513a6c84075089ecda0570ce658d0b45f6dd"}

[commit-bot: I haz the power, 2017-02-27 19:06:27]

Description was changed from

==========
Implement the LitePage Integration Test with ChromeDriver

BUG=680567
==========

to

==========
Implement the LitePage Integration Test with ChromeDriver

BUG=680567

Review-Url: https://codereview.chromium.org/2717563002
Cr-Commit-Position: refs/heads/master@{#453279}
Committed:
https://chromium.googlesource.com/chromium/src/+/e72a513a6c84075089ecda0570ce...
==========

[commit-bot: I haz the power, 2017-02-27 19:06:28]

Committed patchset #3 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/e72a513a6c84075089ecda0570ce...