[DevTools] New harness for inspector-protocol layout tests

[DevTools] New harness for inspector-protocol layout tests

New harness loads a helper html file in a main testing window,
connects it to a browser-wide DevToolsAgentHost for manipulating pages
and evaluates the javascript test.

DevToolsProtocolTestBindings is a small implementation of
DevToolsAgentHost <--> harness communication (based on DevToolsFrontendHost).

Harness has a Page and Session class, which will make it possible to test
multiple sessions connected to the same page, and (if need arises) multiple
pages interaction.

This patch converts ~50 out of ~220 inspector-protocol tests to the new harness
to ensure no roadblocks. More tests will be converted later, with old harness
being deleted afterwards.

BUG=590878

[dgozman, 2017-06-14 21:12:29]

The CQ bit was checked by dgozman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 21:12:42]

Dry run: CQ is trying da patch.

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

[dgozman, 2017-06-14 21:12:44]

dgozman@chromium.org changed reviewers:
+ chenwilliam@chromium.org, pfeldman@chromium.org

[dgozman, 2017-06-14 21:12:45]

Could you please take a look?

[chenwilliam, 2017-06-14 22:06:41]

Nice. Seems like this should have its own dedicated bug since it'll be multiple
CLs?

https://codereview.chromium.org/2942573003/diff/1/content/shell/browser/layou...
File content/shell/browser/layout_test/devtools_protocol_test_bindings.cc
(right):

https://codereview.chromium.org/2942573003/diff/1/content/shell/browser/layou...
content/shell/browser/layout_test/devtools_protocol_test_bindings.cc:50:
std::string dir = "/inspector-protocol-2/";
let's make this directory name more descriptive? (e.g.
/inspector-protocol-multi-client/)

https://codereview.chromium.org/2942573003/diff/1/content/shell/browser/layou...
content/shell/browser/layout_test/devtools_protocol_test_bindings.cc:84:
base::DictionaryValue* dict = NULL;
nullptr? :)

[commit-bot: I haz the power, 2017-06-14 23:26:38]

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

[commit-bot: I haz the power, 2017-06-14 23:26:38]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-06-19 17:48:17]

Description was changed from

==========
[DevTools] New harness for inspector-protocol layout tests

New harness loads a helper html file in a main testing window,
connects it to a browser-wide DevToolsAgentHost for manipulating pages
and evaluates the javascript test.

DevToolsProtocolTestBindings is a small implementation of
DevToolsAgentHost <--> harness communication (based on DevToolsFrontendHost).

Harness has a Page and Session class, which will make it possible to test
multiple sessions connected to the same page, and (if need arises) multiple
pages interaction.

BUG=590878
==========

to

==========
[DevTools] New harness for inspector-protocol layout tests

New harness loads a helper html file in a main testing window,
connects it to a browser-wide DevToolsAgentHost for manipulating pages
and evaluates the javascript test.

DevToolsProtocolTestBindings is a small implementation of
DevToolsAgentHost <--> harness communication (based on DevToolsFrontendHost).

Harness has a Page and Session class, which will make it possible to test
multiple sessions connected to the same page, and (if need arises) multiple
pages interaction.

This patch converts ~50 out of ~220 inspector-protocol tests to the new harness
to ensure no roadblocks. More tests will be converted later, with old harness
being deleted afterwards.

BUG=590878
==========

[dgozman, 2017-06-19 17:48:17]

dgozman@chromium.org changed reviewers:
+ caseq@chromium.org

[dgozman, 2017-06-19 17:48:59]

https://codereview.chromium.org/2942573003/diff/1/content/shell/browser/layou...
File content/shell/browser/layout_test/devtools_protocol_test_bindings.cc
(right):

https://codereview.chromium.org/2942573003/diff/1/content/shell/browser/layou...
content/shell/browser/layout_test/devtools_protocol_test_bindings.cc:50:
std::string dir = "/inspector-protocol-2/";
On 2017/06/14 22:06:41, chenwilliam wrote:
> let's make this directory name more descriptive? (e.g.
> /inspector-protocol-multi-client/)

This was a temporary name :)

https://codereview.chromium.org/2942573003/diff/1/content/shell/browser/layou...
content/shell/browser/layout_test/devtools_protocol_test_bindings.cc:84:
base::DictionaryValue* dict = NULL;
On 2017/06/14 22:06:41, chenwilliam wrote:
> nullptr? :)

Done.

[dgozman, 2017-06-19 17:49:16]

The CQ bit was checked by dgozman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-19 17:49:42]

Dry run: CQ is trying da patch.

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

[dgozman, 2017-06-19 17:50:15]

dgozman@chromium.org changed reviewers:
+ alph@chromium.org

[dgozman, 2017-06-19 17:50:18]

Could you please take a look at the new tests and raise any concerns? I am going
to land this in chunks afterwards.

[pfeldman, 2017-06-19 18:44:52]

lgtm

https://codereview.chromium.org/2942573003/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector-protocol/console/console-let-const-with-api.js
(right):

https://codereview.chromium.org/2942573003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector-protocol/console/console-let-const-with-api.js:2:
let {page, session, Protocol} = await InspectorTest.startBlank('');
s/Protocol/dp/

[pfeldman, 2017-06-19 18:45:02]

lgtm

[chenwilliam, 2017-06-19 19:09:20]

https://codereview.chromium.org/2942573003/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
(left):

https://codereview.chromium.org/2942573003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG:17121:
crbug.com/591099
inspector-enabled/sources/debugger/script-formatter-breakpoints-4.html [ Failure
]
Don't forget to also update TestExpectations:
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/TestExpec...

https://codereview.chromium.org/2942573003/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network-data-length.html
(right):

https://codereview.chromium.org/2942573003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network-data-length.html:4:
<script
src="../inspector-protocol/resources/inspector-protocol-test.js"></script>
Is the plan that these http tests are migrated in a second wave?

https://codereview.chromium.org/2942573003/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector-protocol/page/javascriptDialogEvents.js
(right):

https://codereview.chromium.org/2942573003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector-protocol/page/javascriptDialogEvents.js:1:
(async function() {
I know we talked about this but it seems confusing as a DevTools team member
when to put copyrights when it comes to test files. From a quick scan of other
js files in LayoutTests in Blink it seems like most of them don't have any
copyrights: https://cs.chromium.org/search/?q=f:layouttests+f:js&type=cs

Can we adopt a convention where tests don't need a copyright notice?

https://codereview.chromium.org/2942573003/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector-protocol/resources/inspector-protocol-test.js
(right):

https://codereview.chromium.org/2942573003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector-protocol/resources/inspector-protocol-test.js:5:
var InspectorTest = {};
Just a thought for f/u work, what do you think about using ProtocolTest as a
namespace (make it clear that this has nothing to do with the old integration
tests) and adding doc-types to improve maintainability? I can help with this.

https://codereview.chromium.org/2942573003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector-protocol/resources/inspector-protocol-test.js:346:
InspectorTest.startBlank = description => InspectorTest._start(description,
null, null);
It seems like the description parameter is never actually used? (when I scanned
a few of the tests). It's a bit confusing when reading the call sites in tests
when they're just ''. Test writers can always do InspectorTest.log() as a
convention.

https://codereview.chromium.org/2942573003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector-protocol/resources/inspector-protocol-test.js:350:
InspectorTest.debugTest = function(testFunction) {
after this lands, let's include a short description on debugging protocol tests
in our GitHub wiki
https://github.com/ChromeDevTools/devtools-frontend/wiki/Layout-Tests

[commit-bot: I haz the power, 2017-06-19 19:42:08]

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

[commit-bot: I haz the power, 2017-06-19 19:42:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dgozman, 2017-06-19 21:49:22]

https://codereview.chromium.org/2942573003/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
(left):

https://codereview.chromium.org/2942573003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG:17121:
crbug.com/591099
inspector-enabled/sources/debugger/script-formatter-breakpoints-4.html [ Failure
]
On 2017/06/19 19:09:20, chenwilliam wrote:
> Don't forget to also update TestExpectations:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/TestExpec...

I did for those which I touched. There is a presubmit check, thankfully.

https://codereview.chromium.org/2942573003/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network-data-length.html
(right):

https://codereview.chromium.org/2942573003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network-data-length.html:4:
<script
src="../inspector-protocol/resources/inspector-protocol-test.js"></script>
On 2017/06/19 19:09:20, chenwilliam wrote:
> Is the plan that these http tests are migrated in a second wave?

Exactly.

https://codereview.chromium.org/2942573003/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector-protocol/console/console-let-const-with-api.js
(right):

https://codereview.chromium.org/2942573003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector-protocol/console/console-let-const-with-api.js:2:
let {page, session, Protocol} = await InspectorTest.startBlank('');
On 2017/06/19 18:44:52, pfeldman wrote:
> s/Protocol/dp/

Done.

https://codereview.chromium.org/2942573003/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector-protocol/page/javascriptDialogEvents.js
(right):

https://codereview.chromium.org/2942573003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector-protocol/page/javascriptDialogEvents.js:1:
(async function() {
On 2017/06/19 19:09:20, chenwilliam wrote:
> I know we talked about this but it seems confusing as a DevTools team member
> when to put copyrights when it comes to test files. From a quick scan of other
> js files in LayoutTests in Blink it seems like most of them don't have any
> copyrights: https://cs.chromium.org/search/?q=f:layouttests+f:js&type=cs
> 
> Can we adopt a convention where tests don't need a copyright notice?

Well, this was a rename (although git didn't get it), so copyright is retained
(in this case - no copyright).

Let's discuss test copyrights on blink-dev maybe?

https://codereview.chromium.org/2942573003/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector-protocol/resources/inspector-protocol-test.js
(right):

https://codereview.chromium.org/2942573003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector-protocol/resources/inspector-protocol-test.js:5:
var InspectorTest = {};
On 2017/06/19 19:09:20, chenwilliam wrote:
> Just a thought for f/u work, what do you think about using ProtocolTest as a
> namespace (make it clear that this has nothing to do with the old integration
> tests) and adding doc-types to improve maintainability? I can help with this.

Yeah, I actually want to turn this into TestRunner class, and pass an instance
to a test once we experiment with parallel runs.

https://codereview.chromium.org/2942573003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector-protocol/resources/inspector-protocol-test.js:346:
InspectorTest.startBlank = description => InspectorTest._start(description,
null, null);
On 2017/06/19 19:09:20, chenwilliam wrote:
> It seems like the description parameter is never actually used? (when I
scanned
> a few of the tests). It's a bit confusing when reading the call sites in tests
> when they're just ''. Test writers can always do InspectorTest.log() as a
> convention.

It is in some tests. This is actually to force new tests to provide description!

https://codereview.chromium.org/2942573003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector-protocol/resources/inspector-protocol-test.js:350:
InspectorTest.debugTest = function(testFunction) {
On 2017/06/19 19:09:20, chenwilliam wrote:
> after this lands, let's include a short description on debugging protocol
tests
> in our GitHub wiki
> https://github.com/ChromeDevTools/devtools-frontend/wiki/Layout-Tests

Sure, thanks!

[dgozman, 2017-06-19 21:49:36]

The CQ bit was checked by dgozman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-19 21:50:17]

Dry run: 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-06-19 23:19:09]

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

[commit-bot: I haz the power, 2017-06-19 23:19:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)