DevTools: create test infrastructure so devtools drives the test

DevTools: create test infrastructure so devtools drives the test

Improvements:
* devtools drives the test, not the inspected page
* tests are pure JS
* doc-typed test harness
* improved debugging experience (surround test function with TestRunner.debugTest)

This copies a couple of existing tests - once this infra has been proven,
we'll delete the originals.

Perf:
- Old (66 sec) [0]
- New (45 sec) [1]

[0] src/blink/tools/run_layout_tests.py --repeat-each=100 inspector/console/console-clear.html
[1] src/blink/tools/run_layout_tests.py --repeat-each=100 http/tests/inspector/devtools-js/console-clear.js

BUG=667560
Review-Url: https://codereview.chromium.org/2837083003
Cr-Commit-Position: refs/heads/master@{#481045}
Committed: https://chromium.googlesource.com/chromium/src/+/941c636edf9661207927a5da8dd88a622998d1f6

[chenwilliam, 2017-04-25 01:09:06]

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

[chenwilliam, 2017-04-25 01:09:07]

Thanks for all of your help! Much of this is similar to the earlier CL I created
a month ago (https://codereview.chromium.org/2742623002/)

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/console-clear.js
(right):

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/console-clear.js:6:
TestRunner.loadPanels(["console"])
We can make this even fancier and wrap each test script in an async function so
we could do:

await TestRunner.loadPanels['...']

It's more magical, but allows you to avoid creating a wrapper test function.
What do you think?

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/integration_test_runner.json
(right):

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/integration_test_runner.json:2:
"modules" : [
What do you think about creating an extends property to keep this more DRY? It'd
require a bit of change for Runtime + our Python build scripts.

example API:
"extends": ["inspector"],

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/integration_test_runner/module.json
(right):

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/integration_test_runner/module.json:6:
"test_dependencies": [
I want test writers to control the lifetime of loading modules, so I created
this to keep the dependency preprocessor happy.

[dgozman, 2017-04-25 21:08:28]

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

https://codereview.chromium.org/2837083003/diff/1/content/shell/browser/layou...
content/shell/browser/layout_test/layout_test_devtools_bindings.cc:92:
bindings->CreateFrontendHost(devtools_contents_->GetMainFrame());
I now think we can just remove this call. Could you try it out please?

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/console-clear.js
(right):

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/console-clear.js:4:
TestRunner.addResult("Tests that console is cleared upon requestClearMessages
call.\n");
nit: let's have an empty line after copyright.

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/console-clear.js:6:
TestRunner.loadPanels(["console"])
On 2017/04/25 01:09:07, chenwilliam wrote:
> We can make this even fancier and wrap each test script in an async function
so
> we could do:
> 
> await TestRunner.loadPanels['...']
> 
> It's more magical, but allows you to avoid creating a wrapper test function.
> What do you think?

I'm all for it. It would make reusing frontend for tests that much easier.

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/console-clear.js:7:
.then(() => TestRunner.loadLazyModules(["source_frame"]))
Why do we do this? I'd prefer this to be handled by frontend code.

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/integration_test_runner.json
(right):

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/integration_test_runner.json:2:
"modules" : [
On 2017/04/25 01:09:07, chenwilliam wrote:
> What do you think about creating an extends property to keep this more DRY?
It'd
> require a bit of change for Runtime + our Python build scripts.
> 
> example API:
> "extends": ["inspector"],

Maybe later. Let's keep the idea around.

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/integration_test_runner/ConsoleTestRunner.js
(right):

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/integration_test_runner/ConsoleTestRunner.js:7:
* @suppress {accessControls}
If we intend to compile tests and rely on compilation to make changes easier, it
might be beneficial to get rid of this exception.

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/integration_test_runner/module.json
(right):

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/integration_test_runner/module.json:3:
"test_runner",
Why not merge this module into test_runner?

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/integration_test_runner/module.json:6:
"test_dependencies": [
On 2017/04/25 01:09:07, chenwilliam wrote:
> I want test writers to control the lifetime of loading modules, so I created
> this to keep the dependency preprocessor happy.

Why should tests worry about modules lifetime?

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/main/Timer.js (right):

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/main/Timer.js:4: Main.time =
function(label) {
Merge this into Main.js.

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js
(right):

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js:165:
TestRunner.loadPanels = function(panels) {
Do we ever use multiple? Let's have loadPanel.

[chenwilliam, 2017-06-15 01:01:05]

@dgozman, sorry for the very long delay :o
Please take another look. After I finish this, I'll write out a short doc on the
plan to migrate the tests.

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

https://codereview.chromium.org/2837083003/diff/1/content/shell/browser/layou...
content/shell/browser/layout_test/layout_test_devtools_bindings.cc:92:
bindings->CreateFrontendHost(devtools_contents_->GetMainFrame());
On 2017/04/25 21:08:28, dgozman wrote:
> I now think we can just remove this call. Could you try it out please?

Done.

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/console-clear.js
(right):

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/console-clear.js:4:
TestRunner.addResult("Tests that console is cleared upon requestClearMessages
call.\n");
On 2017/04/25 21:08:28, dgozman wrote:
> nit: let's have an empty line after copyright.

Done.

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/console-clear.js:6:
TestRunner.loadPanels(["console"])
On 2017/04/25 21:08:28, dgozman wrote:
> On 2017/04/25 01:09:07, chenwilliam wrote:
> > We can make this even fancier and wrap each test script in an async function
> so
> > we could do:
> > 
> > await TestRunner.loadPanels['...']
> > 
> > It's more magical, but allows you to avoid creating a wrapper test function.
> > What do you think?
> 
> I'm all for it. It would make reusing frontend for tests that much easier.

Done.

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/console-clear.js:7:
.then(() => TestRunner.loadLazyModules(["source_frame"]))
On 2017/04/25 21:08:28, dgozman wrote:
> Why do we do this? I'd prefer this to be handled by frontend code.

This was to reduce flakiness: https://codereview.chromium.org/1064853002

I think it should actually be text_editor though and not source_frame. We're
lazy loading text editor in the front_end here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front...

And, on second thought I don't think we even need this for this test so I've
deleted it.

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/integration_test_runner/ConsoleTestRunner.js
(right):

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/integration_test_runner/ConsoleTestRunner.js:7:
* @suppress {accessControls}
On 2017/04/25 21:08:28, dgozman wrote:
> If we intend to compile tests and rely on compilation to make changes easier,
it
> might be beneficial to get rid of this exception.

Let's revisit this later? In theory I agree with you, in practice I think it'll
be very difficult to migrate our existing tests without using private members.

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/integration_test_runner/module.json
(right):

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/integration_test_runner/module.json:3:
"test_runner",
On 2017/04/25 21:08:28, dgozman wrote:
> Why not merge this module into test_runner?

We can't run certain modules in test_runner (unit tests). Joel has details (I
forgot :o)

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/integration_test_runner/module.json:6:
"test_dependencies": [
On 2017/04/25 21:08:28, dgozman wrote:
> On 2017/04/25 01:09:07, chenwilliam wrote:
> > I want test writers to control the lifetime of loading modules, so I created
> > this to keep the dependency preprocessor happy.
> 
> Why should tests worry about modules lifetime?

I want to avoid loading all modules up-front to avoid the startup cost. I don't
have a concrete case - but I imagine someone might want a test where clicking on
a panel properly loads it. If we eagerly load modules (e.g. text editor) in the
case of console, we might miss some edge cases (e.g. console properly shows
messages before text editor has loaded).

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/main/Timer.js (right):

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/main/Timer.js:4: Main.time =
function(label) {
On 2017/04/25 21:08:28, dgozman wrote:
> Merge this into Main.js.

Done.

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js
(right):

https://codereview.chromium.org/2837083003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js:165:
TestRunner.loadPanels = function(panels) {
On 2017/04/25 21:08:28, dgozman wrote:
> Do we ever use multiple? Let's have loadPanel.

In a couple spots (e.g.
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test...)
but fair enough, I'll do loadPanel and we can figure out something for those
special cases.

[dgozman, 2017-06-15 21:16:49]

Looks pretty good.

https://codereview.chromium.org/2837083003/diff/40001/content/shell/browser/l...
File content/shell/browser/layout_test/blink_test_controller.cc (right):

https://codereview.chromium.org/2837083003/diff/40001/content/shell/browser/l...
content/shell/browser/layout_test/blink_test_controller.cc:303: test_url_ =
LayoutTestDevToolsBindings::MapUnitTestURL(test_url);
Let's move this all to a single call

LayoutTestDevToolsBindings::MapTestURLIfNeeded(GURL* test_url, bool*
is_integration_test);

https://codereview.chromium.org/2837083003/diff/40001/content/shell/browser/l...
content/shell/browser/layout_test/blink_test_controller.cc:382: if
(!secondary_window_) {
Let's have SecondaryWindow() method which lazily creates it.

https://codereview.chromium.org/2837083003/diff/40001/content/shell/browser/s...
File content/shell/browser/shell_devtools_bindings.cc (right):

https://codereview.chromium.org/2837083003/diff/40001/content/shell/browser/s...
content/shell/browser/shell_devtools_bindings.cc:143: if (!frame->GetParent())
This is the same check as previous if - combine them.

https://codereview.chromium.org/2837083003/diff/40001/content/shell/browser/s...
content/shell/browser/shell_devtools_bindings.cc:156: void
ShellDevToolsBindings::CreateFrontendHost(
Inline it to the method above.

https://codereview.chromium.org/2837083003/diff/40001/content/shell/browser/s...
File content/shell/browser/shell_devtools_bindings.h (right):

https://codereview.chromium.org/2837083003/diff/40001/content/shell/browser/s...
content/shell/browser/shell_devtools_bindings.h:64: void
CreateFrontendHost(RenderFrameHost* render_frame_host);
This can be private now.

https://codereview.chromium.org/2837083003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/integration_test_runner/IntegrationTestRunner.js
(right):

https://codereview.chromium.org/2837083003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/integration_test_runner/IntegrationTestRunner.js:90:
self.runtime.loadModulePromise('integration_test_runner').then(() => {
We are already in this module.

[chenwilliam, 2017-06-19 18:32:36]

Description was changed from

==========
DevTools: create test infrastructure so devtools drives the test

Improvements:
* devtools drives the test, not the inspected page
* tests are pure JS
* doc-typed test harness

This copies a couple of existing tests - once this infra has been proven,
we'll delete the originals.

Perf:
- Old (66 sec) [0]
- New (45 sec) [1]

[0] src/blink/tools/run_layout_tests.py --repeat-each=100
inspector/console/console-clear.html
[1] src/blink/tools/run_layout_tests.py --repeat-each=100
http/tests/inspector/devtools-js/console-clear.js

BUG=667560
==========

to

==========
DevTools: create test infrastructure so devtools drives the test

Improvements:
* devtools drives the test, not the inspected page
* tests are pure JS
* doc-typed test harness
* improved debugging experience (surround test function with
TestRunner.debugTest)

This copies a couple of existing tests - once this infra has been proven,
we'll delete the originals.

Perf:
- Old (66 sec) [0]
- New (45 sec) [1]

[0] src/blink/tools/run_layout_tests.py --repeat-each=100
inspector/console/console-clear.html
[1] src/blink/tools/run_layout_tests.py --repeat-each=100
http/tests/inspector/devtools-js/console-clear.js

BUG=667560
==========

[chenwilliam, 2017-06-19 18:33:30]

@dgozman, please take another look :)

https://codereview.chromium.org/2837083003/diff/40001/content/shell/browser/l...
File content/shell/browser/layout_test/blink_test_controller.cc (right):

https://codereview.chromium.org/2837083003/diff/40001/content/shell/browser/l...
content/shell/browser/layout_test/blink_test_controller.cc:303: test_url_ =
LayoutTestDevToolsBindings::MapUnitTestURL(test_url);
On 2017/06/15 21:16:48, dgozman wrote:
> Let's move this all to a single call
> 
> LayoutTestDevToolsBindings::MapTestURLIfNeeded(GURL* test_url, bool*
> is_integration_test);

Done.

https://codereview.chromium.org/2837083003/diff/40001/content/shell/browser/l...
content/shell/browser/layout_test/blink_test_controller.cc:382: if
(!secondary_window_) {
On 2017/06/15 21:16:48, dgozman wrote:
> Let's have SecondaryWindow() method which lazily creates it.

Done.

https://codereview.chromium.org/2837083003/diff/40001/content/shell/browser/s...
File content/shell/browser/shell_devtools_bindings.cc (right):

https://codereview.chromium.org/2837083003/diff/40001/content/shell/browser/s...
content/shell/browser/shell_devtools_bindings.cc:143: if (!frame->GetParent())
On 2017/06/15 21:16:48, dgozman wrote:
> This is the same check as previous if - combine them.

Done.

https://codereview.chromium.org/2837083003/diff/40001/content/shell/browser/s...
content/shell/browser/shell_devtools_bindings.cc:156: void
ShellDevToolsBindings::CreateFrontendHost(
On 2017/06/15 21:16:48, dgozman wrote:
> Inline it to the method above.

Done.

https://codereview.chromium.org/2837083003/diff/40001/content/shell/browser/s...
File content/shell/browser/shell_devtools_bindings.h (right):

https://codereview.chromium.org/2837083003/diff/40001/content/shell/browser/s...
content/shell/browser/shell_devtools_bindings.h:64: void
CreateFrontendHost(RenderFrameHost* render_frame_host);
On 2017/06/15 21:16:48, dgozman wrote:
> This can be private now.

Done.

https://codereview.chromium.org/2837083003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/integration_test_runner/IntegrationTestRunner.js
(right):

https://codereview.chromium.org/2837083003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/integration_test_runner/IntegrationTestRunner.js:90:
self.runtime.loadModulePromise('integration_test_runner').then(() => {
On 2017/06/15 21:16:48, dgozman wrote:
> We are already in this module.

Yep. removed.

[dgozman, 2017-06-19 22:55:52]

https://codereview.chromium.org/2837083003/diff/100001/content/shell/browser/...
File content/shell/browser/layout_test/blink_test_controller.cc (right):

https://codereview.chromium.org/2837083003/diff/100001/content/shell/browser/...
content/shell/browser/layout_test/blink_test_controller.cc:300: bool
is_devtools_integration_test;
bool is_devtools_js_test = false;

https://codereview.chromium.org/2837083003/diff/100001/content/shell/browser/...
content/shell/browser/layout_test/blink_test_controller.cc:387:
SecondaryWindow();
Shell* secondary = SecondaryWindow();
And use |secondary| below.

https://codereview.chromium.org/2837083003/diff/100001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_devtools_bindings.cc (right):

https://codereview.chromium.org/2837083003/diff/100001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_devtools_bindings.cc:66: if
(!*is_integration_test && !is_unit_test) {
nit: no need for {}

https://codereview.chromium.org/2837083003/diff/100001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_devtools_bindings.cc:70: if
(is_integration_test)
*is_integration_test

https://codereview.chromium.org/2837083003/diff/100001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_devtools_bindings.cc:74: return
MapJSTestURL(test_url, test_page);
MapJSTestURL(test_url, is_unit_test ? "unit_test_runner.html" :
"integration_test_runner.html");

https://codereview.chromium.org/2837083003/diff/100001/content/shell/browser/...
File content/shell/browser/shell_devtools_bindings.h (right):

https://codereview.chromium.org/2837083003/diff/100001/content/shell/browser/...
content/shell/browser/shell_devtools_bindings.h:74: void
CreateFrontendHost(RenderFrameHost* render_frame_host);
remove

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/console-clear.js
(right):

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/console-clear.js:9:
await TestRunner.loadLazyModules(["console_test_runner"]);
loadModule("console_test_runner")

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/simple-test.js
(right):

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/simple-test.js:4:
TestRunner.addResult('Start test');
nit: please add a blank line after copyright.

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/simple-test.js:6:
TestRunner.completeTest();
iife?

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/integration_test_runner.js
(right):

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/integration_test_runner.js:4: if
(self.testRunner) {
nit: blank line

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/integration_test_runner/IntegrationTestRunner.js
(right):

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/integration_test_runner/IntegrationTestRunner.js:72:
(function() {
We don't use iife in frontend usually.

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/integration_test_runner/IntegrationTestRunner.js:79:
class TestObserver {
IntegrationTestRunner.TargetObserver

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/main/Main.js (right):

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/main/Main.js:965: Main.time =
function(label) {
Use static function in Main class instead.

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/Connections.js (right):

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/Connections.js:214: if
(Host.isUnderTest())
Why this?

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js
(right):

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js:10:
Host.isUnderTest = () => true;
Why this?

[chenwilliam, 2017-06-19 23:48:18]

ptal.

https://codereview.chromium.org/2837083003/diff/100001/content/shell/browser/...
File content/shell/browser/layout_test/blink_test_controller.cc (right):

https://codereview.chromium.org/2837083003/diff/100001/content/shell/browser/...
content/shell/browser/layout_test/blink_test_controller.cc:300: bool
is_devtools_integration_test;
On 2017/06/19 22:55:51, dgozman wrote:
> bool is_devtools_js_test = false;

Done.

https://codereview.chromium.org/2837083003/diff/100001/content/shell/browser/...
content/shell/browser/layout_test/blink_test_controller.cc:387:
SecondaryWindow();
On 2017/06/19 22:55:51, dgozman wrote:
> Shell* secondary = SecondaryWindow();
> And use |secondary| below.

Done.

https://codereview.chromium.org/2837083003/diff/100001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_devtools_bindings.cc (right):

https://codereview.chromium.org/2837083003/diff/100001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_devtools_bindings.cc:66: if
(!*is_integration_test && !is_unit_test) {
On 2017/06/19 22:55:51, dgozman wrote:
> nit: no need for {}

Done.

https://codereview.chromium.org/2837083003/diff/100001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_devtools_bindings.cc:70: if
(is_integration_test)
On 2017/06/19 22:55:51, dgozman wrote:
> *is_integration_test

Done. Ah, so it was running the unit tests as integration tests which is why I
saw console messages (which I suppressed with the Host.isUnderTest code), my
bad.

https://codereview.chromium.org/2837083003/diff/100001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_devtools_bindings.cc:74: return
MapJSTestURL(test_url, test_page);
On 2017/06/19 22:55:51, dgozman wrote:
> MapJSTestURL(test_url, is_unit_test ? "unit_test_runner.html" :
> "integration_test_runner.html");

Done.

https://codereview.chromium.org/2837083003/diff/100001/content/shell/browser/...
File content/shell/browser/shell_devtools_bindings.h (right):

https://codereview.chromium.org/2837083003/diff/100001/content/shell/browser/...
content/shell/browser/shell_devtools_bindings.h:74: void
CreateFrontendHost(RenderFrameHost* render_frame_host);
On 2017/06/19 22:55:51, dgozman wrote:
> remove

Done.

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/console-clear.js
(right):

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/console-clear.js:9:
await TestRunner.loadLazyModules(["console_test_runner"]);
On 2017/06/19 22:55:51, dgozman wrote:
> loadModule("console_test_runner")

Done.

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/simple-test.js
(right):

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/simple-test.js:4:
TestRunner.addResult('Start test');
On 2017/06/19 22:55:51, dgozman wrote:
> nit: please add a blank line after copyright.

Done.

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/simple-test.js:6:
TestRunner.completeTest();
On 2017/06/19 22:55:52, dgozman wrote:
> iife?

Done.

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/integration_test_runner.js
(right):

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/integration_test_runner.js:4: if
(self.testRunner) {
On 2017/06/19 22:55:52, dgozman wrote:
> nit: blank line

Done.

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/integration_test_runner/IntegrationTestRunner.js
(right):

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/integration_test_runner/IntegrationTestRunner.js:72:
(function() {
On 2017/06/19 22:55:52, dgozman wrote:
> We don't use iife in frontend usually.

Done.

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/integration_test_runner/IntegrationTestRunner.js:79:
class TestObserver {
On 2017/06/19 22:55:52, dgozman wrote:
> IntegrationTestRunner.TargetObserver

Done.

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/main/Main.js (right):

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/main/Main.js:965: Main.time =
function(label) {
On 2017/06/19 22:55:52, dgozman wrote:
> Use static function in Main class instead.

Done.

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/Connections.js (right):

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/Connections.js:214: if
(Host.isUnderTest())
On 2017/06/19 22:55:52, dgozman wrote:
> Why this?

I accidentally ran unit tests w/ integration test (see earlier comment).
removed.

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js
(right):

https://codereview.chromium.org/2837083003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js:10:
Host.isUnderTest = () => true;
On 2017/06/19 22:55:52, dgozman wrote:
> Why this?

I accidentally ran unit tests w/ integration test (see earlier comment).
removed.

[dgozman, 2017-06-20 00:02:07]

lgtm

https://codereview.chromium.org/2837083003/diff/100001/content/shell/browser/...
File content/shell/browser/layout_test/blink_test_controller.cc (right):

https://codereview.chromium.org/2837083003/diff/100001/content/shell/browser/...
content/shell/browser/layout_test/blink_test_controller.cc:300: bool
is_devtools_integration_test;
On 2017/06/19 23:48:17, chenwilliam wrote:
> On 2017/06/19 22:55:51, dgozman wrote:
> > bool is_devtools_js_test = false;
> 
> Done.

I mostly meant to rename the variable :)

https://codereview.chromium.org/2837083003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js
(right):

https://codereview.chromium.org/2837083003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js:165:
TestRunner.loadLazyModules = function(lazyModules) {
Let's kill this function in a follow up?

https://codereview.chromium.org/2837083003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js:227:
TestRunner.debugTest = function(testFunction) {
While debugging, I felt like this should be even shorter:

function debugTest(testFunction) {
}

[chenwilliam, 2017-06-20 01:10:07]

chenwilliam@chromium.org changed reviewers:
+ qyearsley@chromium.org

[chenwilliam, 2017-06-20 01:10:08]

qyearsley@chromium.org: Could you please review changes in
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py? Thank you. 

It's not great - perhaps I can refactor it in a follow-up CL so it's more
maintainable? (e.g. creating a "flag file" .JS_TESTS so we don't have to
hard-code each directory like this)

https://codereview.chromium.org/2837083003/diff/100001/content/shell/browser/...
File content/shell/browser/layout_test/blink_test_controller.cc (right):

https://codereview.chromium.org/2837083003/diff/100001/content/shell/browser/...
content/shell/browser/layout_test/blink_test_controller.cc:300: bool
is_devtools_integration_test;
On 2017/06/20 00:02:06, dgozman wrote:
> On 2017/06/19 23:48:17, chenwilliam wrote:
> > On 2017/06/19 22:55:51, dgozman wrote:
> > > bool is_devtools_js_test = false;
> > 
> > Done.
> 
> I mostly meant to rename the variable :)

ah :) done.

https://codereview.chromium.org/2837083003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js
(right):

https://codereview.chromium.org/2837083003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js:165:
TestRunner.loadLazyModules = function(lazyModules) {
On 2017/06/20 00:02:06, dgozman wrote:
> Let's kill this function in a follow up?

Yeah, sounds good.

https://codereview.chromium.org/2837083003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js:227:
TestRunner.debugTest = function(testFunction) {
On 2017/06/20 00:02:06, dgozman wrote:
> While debugging, I felt like this should be even shorter:
> 
> function debugTest(testFunction) {
> }

Done.

[qyearsley, 2017-06-20 21:30:49]

On 2017/06/20 01:10:08, chenwilliam wrote:
> mailto:qyearsley@chromium.org: Could you please review changes in
> third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py? Thank
you. 

Change in port/base.py LGTM :-)


So, after this CL, there are some tests that were in
LayoutTests/inspector/console that are now in
LayoutTests/http/tests/inspector/devtools-js/; is part of the purpose of this
change to make it so that those tests can also be run with a different test
runner (not just the layout test runner)?

[chenwilliam, 2017-06-20 21:43:30]

On 2017/06/20 21:30:49, qyearsley wrote:
> On 2017/06/20 01:10:08, chenwilliam wrote:
> > mailto:qyearsley@chromium.org: Could you please review changes in
> > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py? Thank
> you. 
> 
> Change in port/base.py LGTM :-)
> 
> 
> So, after this CL, there are some tests that were in
> LayoutTests/inspector/console that are now in
> LayoutTests/http/tests/inspector/devtools-js/; is part of the purpose of this
> change to make it so that those tests can also be run with a different test
> runner (not just the layout test runner)?

Thanks! Yeah, this is just a temporary phase, but I'll follow-up later and
remove the original tests.
It's run under a different set of DevTools test API (which is still using Layout
Tests under the hood) that will be easier for our team to use.

[chenwilliam, 2017-06-20 22:03:32]

The CQ bit was checked by chenwilliam@chromium.org

[chenwilliam, 2017-06-20 22:03:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, qyearsley@chromium.org
Link to the patchset: https://codereview.chromium.org/2837083003/#ps180001
(title: "rebaseline")

[commit-bot: I haz the power, 2017-06-20 22:03:59]

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-21 00:35:37]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1497996212012220,
"parent_rev": "88915910157839872025a8c20ec6d8d63678e3da", "commit_rev":
"941c636edf9661207927a5da8dd88a622998d1f6"}

[commit-bot: I haz the power, 2017-06-21 00:35:50]

Description was changed from

==========
DevTools: create test infrastructure so devtools drives the test

Improvements:
* devtools drives the test, not the inspected page
* tests are pure JS
* doc-typed test harness
* improved debugging experience (surround test function with
TestRunner.debugTest)

This copies a couple of existing tests - once this infra has been proven,
we'll delete the originals.

Perf:
- Old (66 sec) [0]
- New (45 sec) [1]

[0] src/blink/tools/run_layout_tests.py --repeat-each=100
inspector/console/console-clear.html
[1] src/blink/tools/run_layout_tests.py --repeat-each=100
http/tests/inspector/devtools-js/console-clear.js

BUG=667560
==========

to

==========
DevTools: create test infrastructure so devtools drives the test

Improvements:
* devtools drives the test, not the inspected page
* tests are pure JS
* doc-typed test harness
* improved debugging experience (surround test function with
TestRunner.debugTest)

This copies a couple of existing tests - once this infra has been proven,
we'll delete the originals.

Perf:
- Old (66 sec) [0]
- New (45 sec) [1]

[0] src/blink/tools/run_layout_tests.py --repeat-each=100
inspector/console/console-clear.html
[1] src/blink/tools/run_layout_tests.py --repeat-each=100
http/tests/inspector/devtools-js/console-clear.js

BUG=667560

Review-Url: https://codereview.chromium.org/2837083003
Cr-Commit-Position: refs/heads/master@{#481045}
Committed:
https://chromium.googlesource.com/chromium/src/+/941c636edf9661207927a5da8dd8...
==========

[commit-bot: I haz the power, 2017-06-21 00:35:53]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/941c636edf9661207927a5da8dd8...