DevTools: add console test helpers to new test runner & migrate a console test

DevTools: add console test helpers to new test runner & migrate a console test

- Copies the remaining methods in console-test.js into ConsoleTestRunner.js and adds JsDocs
- Migrates a console test (will delete original in f/u CL).

This will be followed-up by migrating batches of console test.

BUG=667560
Review-Url: https://codereview.chromium.org/2975523002
Cr-Commit-Position: refs/heads/master@{#486559}
Committed: https://chromium.googlesource.com/chromium/src/+/928d360b185de165acb06cddfdba459f0100a666

[chenwilliam, 2017-07-06 23:55:56]

Description was changed from

==========
DevTools: add console test helpers to new test runner & migrate a console test

- Copies the remaining methods in console-test.js into ConsoleTestRunner.js and
adds JsDocs
- Migrates a console test

This will be followed-up by migrating batches of console test.

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

to

==========
DevTools: add console test helpers to new test runner & migrate a console test

- Copies the remaining methods in console-test.js into ConsoleTestRunner.js and
adds JsDocs
- Migrates a console test

This will be followed-up by migrating batches of console test.

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

[chenwilliam, 2017-07-06 23:55:56]

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

[chenwilliam, 2017-07-06 23:58:32]

I found it easier to just convert all of the console test helpers upfront since
I'll have to do it anyway rather than just converting a few at a time.

I'll probably convert a few more console tests by hand to iron out more issues
before trying to auto-migrate them.

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/.clang-format
(right):

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/.clang-format:1:
---
For now, I'm running clang-format manually on these new js tests. Eventually,
I'll do a presubmit check (like we do for front-end).

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js
(right):

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:147:
ConsoleTestRunner.selectMainExecutionContext = function() {
Most of this was copy and paste from console-test.js - I did some very basic
cleanup (e.g. unused variables, parameters) and added js docs.

[luoe, 2017-07-07 22:26:40]

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js
(right):

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:12:
* @param {boolean} dumpClassNames
The first two printOriginating, dumpClassNames are actually optionally undefined
as well.

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:22:
* @param {boolean} dumpClassNames
ditto

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:160:
* @param {boolean} dontForceMainContext
callback and dontForce are Optional

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:189:
* @param {boolean} dontForceMainContext
callback and dontForce are Optional

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:194:
function mycallback(text) {
JSDoc for this, too?  :D

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:216:
function isNotEmptyLine(string) {
JSDoc these too?

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:242:
* @param {boolean} dumpClassNames
First two are optional

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:265:
* @param {boolean} sortMessages
Optional

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:298:
* @param {!Function} sectionFilter
I think JSDoc for function params on *filter would be useful.  They're also
optional.
Should be something like !Function(!Element):boolean
For the sectionFilter, the section should be an
!ObjectUI.ObjectPropertiesSection

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:393:
ConsoleTestRunner.checkConsoleMessagesDontHaveParameters = function() {
Looks like this function is no longer used?  Let's get rid of it.

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:512:
function selectionContainerAndOffset(container, offset) {
jsdoc here too?

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/console_test_runner/module.json
(right):

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/module.json:7:
"main"
It seems unfortunate to add this dependency for just 2 tests that use Main's
error counters.

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

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/integration_test_runner/IntegrationTestRunner.js:44:
* @param {string} code
string|Function ?

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/integration_test_runner/IntegrationTestRunner.js:49:
code = `(${code.toString()})()`;
Neat

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/integration_test_runner/IntegrationTestRunner.js:58:
* @param {string} code
ditto

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/integration_test_runner/IntegrationTestRunner.js:78:
TestRunner.loadHTML = function(html) {
It would be nice in the future if this aligned with the new test harness used by
protocol, which looks almost the same: testRunner.startHTML(html, description)

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js
(right):

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js:190: *
@param {!Function} func
I think this is optional, too?

[dgozman, 2017-07-09 18:15:40]

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/console_test_runner/module.json
(right):

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/module.json:7:
"main"
On 2017/07/07 22:26:40, luoe wrote:
> It seems unfortunate to add this dependency for just 2 tests that use Main's
> error counters.

Perhaps, we can extract counters into module?

[chenwilliam, 2017-07-10 21:14:33]

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

[chenwilliam, 2017-07-10 21:14:35]

please take another look

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js
(right):

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:12:
* @param {boolean} dumpClassNames
On 2017/07/07 22:26:40, luoe wrote:
> The first two printOriginating, dumpClassNames are actually optionally
undefined
> as well.

Done.

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:22:
* @param {boolean} dumpClassNames
On 2017/07/07 22:26:40, luoe wrote:
> ditto

Done.

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:160:
* @param {boolean} dontForceMainContext
On 2017/07/07 22:26:40, luoe wrote:
> callback and dontForce are Optional

Done.

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:189:
* @param {boolean} dontForceMainContext
On 2017/07/07 22:26:39, luoe wrote:
> callback and dontForce are Optional

Done.

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:194:
function mycallback(text) {
On 2017/07/07 22:26:40, luoe wrote:
> JSDoc for this, too?  :D

Done.

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:216:
function isNotEmptyLine(string) {
On 2017/07/07 22:26:40, luoe wrote:
> JSDoc these too?

Done.

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:242:
* @param {boolean} dumpClassNames
On 2017/07/07 22:26:39, luoe wrote:
> First two are optional

Done.

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:265:
* @param {boolean} sortMessages
On 2017/07/07 22:26:40, luoe wrote:
> Optional

Done.

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:298:
* @param {!Function} sectionFilter
On 2017/07/07 22:26:40, luoe wrote:
> I think JSDoc for function params on *filter would be useful.  They're also
> optional.
> Should be something like !Function(!Element):boolean
> For the sectionFilter, the section should be an
> !ObjectUI.ObjectPropertiesSection

Done.

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:393:
ConsoleTestRunner.checkConsoleMessagesDontHaveParameters = function() {
On 2017/07/07 22:26:39, luoe wrote:
> Looks like this function is no longer used?  Let's get rid of it.

Done.

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:512:
function selectionContainerAndOffset(container, offset) {
On 2017/07/07 22:26:39, luoe wrote:
> jsdoc here too?

Done.

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/console_test_runner/module.json
(right):

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/module.json:7:
"main"
On 2017/07/09 18:15:40, dgozman_ooo (send to pfeldman) wrote:
> On 2017/07/07 22:26:40, luoe wrote:
> > It seems unfortunate to add this dependency for just 2 tests that use Main's
> > error counters.
> 
> Perhaps, we can extract counters into module?

Extracted!

Dgozman - working on a weekend?!

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

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/integration_test_runner/IntegrationTestRunner.js:44:
* @param {string} code
On 2017/07/07 22:26:40, luoe wrote:
> string|Function ?

Done.

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/integration_test_runner/IntegrationTestRunner.js:58:
* @param {string} code
On 2017/07/07 22:26:40, luoe wrote:
> ditto

Done.

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/integration_test_runner/IntegrationTestRunner.js:78:
TestRunner.loadHTML = function(html) {
On 2017/07/07 22:26:40, luoe wrote:
> It would be nice in the future if this aligned with the new test harness used
by
> protocol, which looks almost the same: testRunner.startHTML(html, description)

This code is actually almost the same as page.loadHTML
(https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/inspector...)

startHTML is kicking off the test (starting the debugger session) & creating the
DOM fixture vs. this method is only about creating the DOM fixture.

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js
(right):

https://codereview.chromium.org/2975523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js:190: *
@param {!Function} func
On 2017/07/07 22:26:40, luoe wrote:
> I think this is optional, too?

Oy.. you're right.

[chenwilliam, 2017-07-10 21:16:56]

Description was changed from

==========
DevTools: add console test helpers to new test runner & migrate a console test

- Copies the remaining methods in console-test.js into ConsoleTestRunner.js and
adds JsDocs
- Migrates a console test

This will be followed-up by migrating batches of console test.

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

to

==========
DevTools: add console test helpers to new test runner & migrate a console test

- Copies the remaining methods in console-test.js into ConsoleTestRunner.js and
adds JsDocs
- Migrates a console test (will delete original in f/u CL).

This will be followed-up by migrating batches of console test.

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

[chenwilliam, 2017-07-10 23:44:16]

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

[chenwilliam, 2017-07-10 23:44:16]

@caseq - would you mind taking a look at this CL? I'm starting to work on
migrating our tests to the new infrastructure and will be working on a series of
CL to do this. Design doc: go/devtools-test-infra.

[luoe, 2017-07-11 22:26:09]

lgtm

Is it now possible to have console just depend on counters instead of main?

https://codereview.chromium.org/2975523002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/console_test_runner/module.json
(right):

https://codereview.chromium.org/2975523002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/module.json:7:
"main"
counters?

[chenwilliam, 2017-07-11 23:06:01]

https://codereview.chromium.org/2975523002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/console_test_runner/module.json
(right):

https://codereview.chromium.org/2975523002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/module.json:7:
"main"
On 2017/07/11 22:26:09, luoe wrote:
> counters?

Done.

[caseq, 2017-07-12 01:04:34]

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/console/command-line-api.js
(right):

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/console/command-line-api.js:10:
await TestRunner.loadPanel("console");
I wonder whether these need to be sequential. Would we have somewhat better
performance if we had something like

await Promise.all([
    TestRunner.loadModule(...),
    TestRunner.loadModule(...),
    TestRunner.loadPanel(...),
    TestRunner.loadHTML(...)
]);

?

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/console/command-line-api.js:42:
ConsoleTestRunner.evaluateInConsole(expression, step1);
Can we make it return a promise straight away? Also, if you get rid of
Common.console.log() above, this could be simpler -- we only need to wait for
the last one.

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/inspector/initial-modules-load-expected.txt
(right):

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/initial-modules-load-expected.txt:10:
counters
this recently changed, you would need to rebase.

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js
(right):

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:182:
var sniffer = function(viewMessage) {
why not just pass override directly? Also, perhaps addSnifferPromise instead?

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:195:
callback = TestRunner.safeWrap(callback);
No need for this, you already wrap in evalueteInConsole()

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:291:
TestRunner.addResult(result[i]);
just TestRunner.addResults(result)?

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:312:
ConsoleTestRunner.expandConsoleMessages = function(callback, deepFilter,
sectionFilter) {
Let's switch all these to promises!

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/counters/module.json (right):

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/counters/module.json:1: {
Let's call this console_counters? Also, I think this could as well be extracted
into a separate CL.

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

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/integration_test_runner/IntegrationTestRunner.js:48:
if (typeof code === 'function')
Should we assert code.length === 0 in this case, since you're calling it without
arguments?

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js
(right):

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js:190: *
@param {!Function=} func
I think it's technically {!Function|undefined} rather than !Function= -- i.e.
you don't want this parameter to be omitted, but when specified, it can be
undefined.

[dgozman, 2017-07-12 17:40:25]

Note that the plan is to not change any semantics of test helpers, so that
automatic test migration could be done. We are going to follow-up with promises
and everything after migration.

[chenwilliam, 2017-07-12 22:02:58]

@caseq - thanks for the review. could you take another look?

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/console/command-line-api.js
(right):

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/console/command-line-api.js:10:
await TestRunner.loadPanel("console");
On 2017/07/12 01:04:33, caseq wrote:
> I wonder whether these need to be sequential. Would we have somewhat better
> performance if we had something like
> 
> await Promise.all([
>     TestRunner.loadModule(...),
>     TestRunner.loadModule(...),
>     TestRunner.loadPanel(...),
>     TestRunner.loadHTML(...)
> ]);
> 
> ?

I a/b tested it (ran this test 100 times) and the perf difference is negligible.
I don't think these need to be sequential either, but I feel it's safer doing it
sequentially in case there's a race condition with loading our test runner
modules (there shouldn't be, but our existing test helpers are quite hairy).

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/console/command-line-api.js:42:
ConsoleTestRunner.evaluateInConsole(expression, step1);
On 2017/07/12 01:04:33, caseq wrote:
> Can we make it return a promise straight away? Also, if you get rid of
> Common.console.log() above, this could be simpler -- we only need to wait for
> the last one.

Adding to @dgozman's comment, the plan is to keep the test semantics the same
and then later migrate our test API to promises.

I'm working on a script that will automatically transform our old HTML tests ->
new JS tests.

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/inspector/initial-modules-load-expected.txt
(right):

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/initial-modules-load-expected.txt:10:
counters
On 2017/07/12 01:04:33, caseq wrote:
> this recently changed, you would need to rebase.

Ah, thanks.

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js
(right):

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:182:
var sniffer = function(viewMessage) {
On 2017/07/12 01:04:33, caseq wrote:
> why not just pass override directly? Also, perhaps addSnifferPromise instead?

I passed the override directly.

See earlier comment about promisifying the API.

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:195:
callback = TestRunner.safeWrap(callback);
On 2017/07/12 01:04:33, caseq wrote:
> No need for this, you already wrap in evalueteInConsole()

Done.

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:291:
TestRunner.addResult(result[i]);
On 2017/07/12 01:04:33, caseq wrote:
> just TestRunner.addResults(result)?

Done.

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:312:
ConsoleTestRunner.expandConsoleMessages = function(callback, deepFilter,
sectionFilter) {
On 2017/07/12 01:04:33, caseq wrote:
> Let's switch all these to promises!

See earlier comment about promisifying the API.

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/counters/module.json (right):

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/counters/module.json:1: {
On 2017/07/12 01:04:33, caseq wrote:
> Let's call this console_counters? Also, I think this could as well be
extracted
> into a separate CL.

OK - I'll do it in a f/u CL.

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

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/integration_test_runner/IntegrationTestRunner.js:48:
if (typeof code === 'function')
On 2017/07/12 01:04:33, caseq wrote:
> Should we assert code.length === 0 in this case, since you're calling it
without
> arguments?

Done.

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js
(right):

https://codereview.chromium.org/2975523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/test_runner/TestRunner.js:190: *
@param {!Function=} func
On 2017/07/12 01:04:33, caseq wrote:
> I think it's technically {!Function|undefined} rather than !Function= -- i.e.
> you don't want this parameter to be omitted, but when specified, it can be
> undefined.

Done.

[caseq, 2017-07-13 17:50:00]

lgtm

https://codereview.chromium.org/2975523002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/console/command-line-api.js
(right):

https://codereview.chromium.org/2975523002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/console/command-line-api.js:8:
await TestRunner.loadModule("console_test_runner");
nit: s/"/'/g since this appears to be in new style.

https://codereview.chromium.org/2975523002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js
(right):

https://codereview.chromium.org/2975523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:424:
var promise = new Promise(fullfill => (callback = fullfill));
Let's use a shorter idiom for these, this is actually a use case that shaped the
contemporary Promise interface:

return new Promise(fulfill =>
ConsoleTestRunner.waitUntilMessageReceived(fulfill));

https://codereview.chromium.org/2975523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:450:
ConsoleTestRunner.waitUntilNthMessageReceived(count, callback);
ditto.

https://codereview.chromium.org/2975523002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/console_test_runner/module.json
(right):

https://codereview.chromium.org/2975523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console_test_runner/module.json:7:
"main"
That's supposed to be console_counters, I assume?

[chenwilliam, 2017-07-13 19:16:45]

Thanks for the review

https://codereview.chromium.org/2975523002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/console/command-line-api.js
(right):

https://codereview.chromium.org/2975523002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/devtools-js/console/command-line-api.js:8:
await TestRunner.loadModule("console_test_runner");
On 2017/07/13 17:50:00, caseq wrote:
> nit: s/"/'/g since this appears to be in new style.

Done.

https://codereview.chromium.org/2975523002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js
(right):

https://codereview.chromium.org/2975523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:424:
var promise = new Promise(fullfill => (callback = fullfill));
On 2017/07/13 17:50:00, caseq wrote:
> Let's use a shorter idiom for these, this is actually a use case that shaped
the
> contemporary Promise interface:
> 
> return new Promise(fulfill =>
> ConsoleTestRunner.waitUntilMessageReceived(fulfill));

Nice. Done.

https://codereview.chromium.org/2975523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console_test_runner/ConsoleTestRunner.js:450:
ConsoleTestRunner.waitUntilNthMessageReceived(count, callback);
On 2017/07/13 17:50:00, caseq wrote:
> ditto.

Done.

https://codereview.chromium.org/2975523002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/console_test_runner/module.json
(right):

https://codereview.chromium.org/2975523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console_test_runner/module.json:7:
"main"
On 2017/07/13 17:50:00, caseq wrote:
> That's supposed to be console_counters, I assume?

Yup.

[chenwilliam, 2017-07-13 19:16:51]

The CQ bit was checked by chenwilliam@chromium.org

[chenwilliam, 2017-07-13 19:16:52]

The patchset sent to the CQ was uploaded after l-g-t-m from luoe@chromium.org,
caseq@chromium.org
Link to the patchset: https://codereview.chromium.org/2975523002/#ps180001
(title: "fix")

[commit-bot: I haz the power, 2017-07-13 19:17:09]

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-07-13 20:38:08]

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

[commit-bot: I haz the power, 2017-07-13 20:38:09]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[chenwilliam, 2017-07-13 20:40:56]

The CQ bit was checked by chenwilliam@chromium.org

[commit-bot: I haz the power, 2017-07-13 20:41:35]

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-07-14 00:56:31]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1499978456947890,
"parent_rev": "9f204165f361bd4e8c3a125517603be2af437c99", "commit_rev":
"928d360b185de165acb06cddfdba459f0100a666"}

[commit-bot: I haz the power, 2017-07-14 00:56:44]

Description was changed from

==========
DevTools: add console test helpers to new test runner & migrate a console test

- Copies the remaining methods in console-test.js into ConsoleTestRunner.js and
adds JsDocs
- Migrates a console test (will delete original in f/u CL).

This will be followed-up by migrating batches of console test.

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

to

==========
DevTools: add console test helpers to new test runner & migrate a console test

- Copies the remaining methods in console-test.js into ConsoleTestRunner.js and
adds JsDocs
- Migrates a console test (will delete original in f/u CL).

This will be followed-up by migrating batches of console test.

BUG=667560

Review-Url: https://codereview.chromium.org/2975523002
Cr-Commit-Position: refs/heads/master@{#486559}
Committed:
https://chromium.googlesource.com/chromium/src/+/928d360b185de165acb06cddfdba...
==========

[commit-bot: I haz the power, 2017-07-14 00:56:46]

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