DevTools: Convert inspector-unit tests to use reusable test harness

DevTools: Convert inspector-unit tests to use reusable test harness

BUG=667560
Committed: https://crrev.com/0a7cd2cdb19c86e206a9b8f96ff03378635f135c
Cr-Commit-Position: refs/heads/master@{#435771}

[chenwilliam, 2016-11-22 01:17:20]

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

[chenwilliam, 2016-11-22 01:17:20]

Please take a look - I started with Joel's unit tests as part of the effort to
convert tests to Javascript since it's the most straightforward.

If this looks good, then I'll convert the rest of the small batch of
inspector-unit tests in this CL (a mechanical HTML -> JS change).

[einbinder, 2016-11-22 02:19:46]

https://codereview.chromium.org/2513423003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector-unit/inspector-unit-test.js
(right):

https://codereview.chromium.org/2513423003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector-unit/inspector-unit-test.js:128:
Runtime.startApplication("/inspector-unit/inspector-unit-test").then(() =>
runTest(test));
Maybe don't force things to be wrapped in function test?

https://codereview.chromium.org/2513423003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector-unit/resources/inspector-unit-test-shell.html
(right):

https://codereview.chromium.org/2513423003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector-unit/resources/inspector-unit-test-shell.html:7:
function getQueryParameter(queryParameter) {
Runtime.queryParam?

[chenwilliam, 2016-11-22 20:56:05]

Please take another look. I updated based on feedback and migrated all of the
inspector-unit tests into js tests.

https://codereview.chromium.org/2513423003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector-unit/inspector-unit-test.js
(right):

https://codereview.chromium.org/2513423003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector-unit/inspector-unit-test.js:128:
Runtime.startApplication("/inspector-unit/inspector-unit-test").then(() =>
runTest(test));
On 2016/11/22 02:19:45, einbinder wrote:
> Maybe don't force things to be wrapped in function test?

Done.

https://codereview.chromium.org/2513423003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector-unit/resources/inspector-unit-test-shell.html
(right):

https://codereview.chromium.org/2513423003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector-unit/resources/inspector-unit-test-shell.html:7:
function getQueryParameter(queryParameter) {
On 2016/11/22 02:19:46, einbinder wrote:
> Runtime.queryParam?

Done.

[dgozman, 2016-11-22 21:59:05]

Looks really good!

Let's try to do this with a real inspector test now?

https://codereview.chromium.org/2513423003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector-unit/inspector-unit-test-helper.js
(right):

https://codereview.chromium.org/2513423003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector-unit/inspector-unit-test-helper.js:156:
.then((data) => data.text())
Let's not .then() synchronous actions.

https://codereview.chromium.org/2513423003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector-unit/inspector-unit-test-helper.js:157:
.then((testScript) => eval(testScript))
Let's wrap the source in closure to improve isolation between tests for the
future.

[chenwilliam, 2016-11-23 00:27:41]

PTAL.

https://codereview.chromium.org/2513423003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector-unit/inspector-unit-test-helper.js
(right):

https://codereview.chromium.org/2513423003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector-unit/inspector-unit-test-helper.js:156:
.then((data) => data.text())
On 2016/11/22 21:59:05, dgozman wrote:
> Let's not .then() synchronous actions.

The text() method on the Response object from a fetch returns a promise.

https://developer.mozilla.org/en-US/docs/Web/API/Body/text

https://codereview.chromium.org/2513423003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector-unit/inspector-unit-test-helper.js:157:
.then((testScript) => eval(testScript))
On 2016/11/22 21:59:05, dgozman wrote:
> Let's wrap the source in closure to improve isolation between tests for the
> future.

Done.

[dgozman, 2016-11-28 18:44:51]

lgtm

https://codereview.chromium.org/2513423003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector-unit/inspector-unit-test-helper.js
(right):

https://codereview.chromium.org/2513423003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector-unit/inspector-unit-test-helper.js:108:
return Promise.all(lazyModules.map(lazyModule =>
window.runtime.loadModulePromise(lazyModule)));
window -> self

https://codereview.chromium.org/2513423003/diff/60001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py
(right):

https://codereview.chromium.org/2513423003/diff/60001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py:794: if
'inspector-unit' in dirname:
No idea whether this is a correct place to do a check :-)

[pfeldman, 2016-11-28 19:01:03]

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

https://codereview.chromium.org/2513423003/diff/60001/content/shell/browser/l...
content/shell/browser/layout_test/blink_test_controller.cc:271: test_url_ =
LayoutTestDevToolsFrontend::MapJSTestURL(test_url);
We should not defer to devtools-specific code here. If we are doing something
specifically for devtools, it should be explicit and reside in the same
controller object.

I'm thinking that we could create a "test" front-end application that would
capture only what is needed to the ui tests and run it here + pass the js to it.
That way what you are doing would be in line with our longer term plans.

[chenwilliam, 2016-11-29 22:30:55]

Description was changed from

==========
DevTools: Make inspector-unit test into javascript test

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

to

==========
DevTools: Convert inspector-unit tests to use reusable test harness

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

[chenwilliam, 2016-11-29 22:33:34]

Please take another look.

I've created a front-end test application which lets us get rid of the hacks for
inspector-unit (e.g. monkey-patching Runtime and using <base href...>) and later
reuse the test harness for unit and integration tests.

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

https://codereview.chromium.org/2513423003/diff/60001/content/shell/browser/l...
content/shell/browser/layout_test/blink_test_controller.cc:271: test_url_ =
LayoutTestDevToolsFrontend::MapJSTestURL(test_url);
On 2016/11/28 19:01:03, pfeldman wrote:
> We should not defer to devtools-specific code here. If we are doing something
> specifically for devtools, it should be explicit and reside in the same
> controller object.
> 
> I'm thinking that we could create a "test" front-end application that would
> capture only what is needed to the ui tests and run it here + pass the js to
it.
> That way what you are doing would be in line with our longer term plans.

Done.

https://codereview.chromium.org/2513423003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector-unit/inspector-unit-test-helper.js
(right):

https://codereview.chromium.org/2513423003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector-unit/inspector-unit-test-helper.js:108:
return Promise.all(lazyModules.map(lazyModule =>
window.runtime.loadModulePromise(lazyModule)));
On 2016/11/28 18:44:51, dgozman wrote:
> window -> self

Done.

[pfeldman, 2016-11-29 23:06:17]

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

https://codereview.chromium.org/2513423003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/unit_test_runner.js:12: function
initializeTest() {
test runner module should do this on its own.

[chenwilliam, 2016-11-30 02:21:02]

Please take another look. I've extracted the logic to initialize unit test into
a new shell module.

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

https://codereview.chromium.org/2513423003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/unit_test_runner.js:12: function
initializeTest() {
On 2016/11/29 23:06:17, pfeldman wrote:
> test runner module should do this on its own.

Done - moved logic to new shell module now.

[dgozman, 2016-11-30 18:19:54]

https://codereview.chromium.org/2513423003/diff/120001/components/test_runner...
File components/test_runner/test_interfaces.cc (right):

https://codereview.chromium.org/2513423003/diff/120001/components/test_runner...
components/test_runner/test_interfaces.cc:100:
spec.find("unit_test_runner.html") == std::string::npos) {
Why this? How does the url for unit_test_runner.html look?

https://codereview.chromium.org/2513423003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/shell/TestShell.js (right):

https://codereview.chromium.org/2513423003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/shell/TestShell.js:13:
document.addEventListener('DOMContentLoaded', () => this.initializeUnitTest());
There is runOnWindowLoad function in DOMExtension.js

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

https://codereview.chromium.org/2513423003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js:16:
TestRunner.results = [];
_results ?

https://codereview.chromium.org/2513423003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js:41:
testRunner.notifyDone();
nit: self.testRunner

https://codereview.chromium.org/2513423003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js:44:
TestRunner.addResult = function(text) {
Let's JSDoc everything, since we are compiling now.

https://codereview.chromium.org/2513423003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js:140:
self['onerror'] = completeTestOnError;
Let's wrap this function and assignment into closure.

[einbinder, 2016-11-30 19:51:06]

https://codereview.chromium.org/2513423003/diff/120001/components/test_runner...
File components/test_runner/test_interfaces.cc (right):

https://codereview.chromium.org/2513423003/diff/120001/components/test_runner...
components/test_runner/test_interfaces.cc:100:
spec.find("unit_test_runner.html") == std::string::npos) {
On 2016/11/30 at 18:19:54, dgozman wrote:
> Why this? How does the url for unit_test_runner.html look?

Shouldn't unit_test_runner.html be under a /front_end/ folder not an /inspector/
folder?

https://codereview.chromium.org/2513423003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/externs.js (right):

https://codereview.chromium.org/2513423003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/externs.js:809: var testRunner =
{};
Can we somehow enforce that this might be undefined? I'm worried about a
situation where someone uses testRunner before checking if it exists, all the
tests on the bots pass, but it breaks devtools or at least the tests outside of
content shell.

[chenwilliam, 2016-11-30 19:53:09]

PTAL.

https://codereview.chromium.org/2513423003/diff/120001/components/test_runner...
File components/test_runner/test_interfaces.cc (right):

https://codereview.chromium.org/2513423003/diff/120001/components/test_runner...
components/test_runner/test_interfaces.cc:100:
spec.find("unit_test_runner.html") == std::string::npos) {
On 2016/11/30 18:19:54, dgozman wrote:
> Why this? How does the url for unit_test_runner.html look?

I added this because the unit_test_runner app file is served from
"/resources/inspector".

file:///.../chromium/src/out/Release/resources/inspector/unit_test_runner.html?experiments=true&test=http://localhost:8080/inspector-unit/test-failure.js

https://codereview.chromium.org/2513423003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/shell/TestShell.js (right):

https://codereview.chromium.org/2513423003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/shell/TestShell.js:13:
document.addEventListener('DOMContentLoaded', () => this.initializeUnitTest());
On 2016/11/30 18:19:54, dgozman wrote:
> There is runOnWindowLoad function in DOMExtension.js

Done.

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

https://codereview.chromium.org/2513423003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js:16:
TestRunner.results = [];
On 2016/11/30 18:19:54, dgozman wrote:
> _results ?

Done.

https://codereview.chromium.org/2513423003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js:41:
testRunner.notifyDone();
On 2016/11/30 18:19:54, dgozman wrote:
> nit: self.testRunner

Done.

https://codereview.chromium.org/2513423003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js:44:
TestRunner.addResult = function(text) {
On 2016/11/30 18:19:54, dgozman wrote:
> Let's JSDoc everything, since we are compiling now.

Done.

https://codereview.chromium.org/2513423003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js:140:
self['onerror'] = completeTestOnError;
On 2016/11/30 18:19:54, dgozman wrote:
> Let's wrap this function and assignment into closure.

Done.

[chenwilliam, 2016-11-30 23:06:04]

https://codereview.chromium.org/2513423003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/externs.js (right):

https://codereview.chromium.org/2513423003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/externs.js:809: var testRunner =
{};
On 2016/11/30 19:51:06, einbinder wrote:
> Can we somehow enforce that this might be undefined? I'm worried about a
> situation where someone uses testRunner before checking if it exists, all the
> tests on the bots pass, but it breaks devtools or at least the tests outside
of
> content shell.

I moved the type definition into the test_runner module so it's a closure type
error if you use self.testRunner in a production module. It's still possible to
break tests outside of content shell (when we write them in the future) but I
think we can address this later once we have them.

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

https://codereview.chromium.org/2513423003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js:6:
self.testRunner;
It doesn't seem like closure compiler enforces an if-guard to make sure
testRunner is not undefined before using it.

[einbinder, 2016-12-01 02:07:51]

lgtm

https://codereview.chromium.org/2513423003/diff/120001/components/test_runner...
File components/test_runner/test_interfaces.cc (right):

https://codereview.chromium.org/2513423003/diff/120001/components/test_runner...
components/test_runner/test_interfaces.cc:100:
spec.find("unit_test_runner.html") == std::string::npos) {
On 2016/11/30 at 19:53:08, chenwilliam wrote:
> On 2016/11/30 18:19:54, dgozman wrote:
> > Why this? How does the url for unit_test_runner.html look?
> 
> I added this because the unit_test_runner app file is served from
"/resources/inspector".
> 
>
file:///.../chromium/src/out/Release/resources/inspector/unit_test_runner.html?experiments=true&test=http://localhost:8080/inspector-unit/test-failure.js

Nice!

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

https://codereview.chromium.org/2513423003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js:6:
self.testRunner;
On 2016/11/30 at 23:06:04, chenwilliam wrote:
> It doesn't seem like closure compiler enforces an if-guard to make sure
testRunner is not undefined before using it.

Looks good enough to me.

[pfeldman, 2016-12-01 21:38:20]

lgtm

[chenwilliam, 2016-12-01 21:40:24]

The CQ bit was checked by chenwilliam@chromium.org

[chenwilliam, 2016-12-01 21:40:24]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2513423003/#ps160001
(title: "Moved type definition out of externs")

[commit-bot: I haz the power, 2016-12-01 21:40:49]

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, 2016-12-01 23:49:48]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1480628424198230,
"parent_rev": "a7bae288eef9028bfc4b82bb126bb10d3654ca31", "commit_rev":
"16dac9b8716f829cb933d2276ff60171b305c9da"}

[commit-bot: I haz the power, 2016-12-01 23:50:19]

Description was changed from

==========
DevTools: Convert inspector-unit tests to use reusable test harness

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

to

==========
DevTools: Convert inspector-unit tests to use reusable test harness

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

[commit-bot: I haz the power, 2016-12-01 23:50:20]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-12-01 23:52:48]

Description was changed from

==========
DevTools: Convert inspector-unit tests to use reusable test harness

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

to

==========
DevTools: Convert inspector-unit tests to use reusable test harness

BUG=667560

Committed: https://crrev.com/0a7cd2cdb19c86e206a9b8f96ff03378635f135c
Cr-Commit-Position: refs/heads/master@{#435771}
==========

[commit-bot: I haz the power, 2016-12-01 23:52:49]

Patchset 9 (id:??) landed as
https://crrev.com/0a7cd2cdb19c86e206a9b8f96ff03378635f135c
Cr-Commit-Position: refs/heads/master@{#435771}