Print out label and description on task entry in Audit task runner

Print out label and description on task entry in Audit task runner

This CL changes how to specify the label/description of a test task.


audit.define('label', taskFunc);
audit.define({ label: 'label', description: 'description' }, taskFunc);


Even if the description is not given the entry of task will be 
displayed. Note that task.describe() will be deprecated.

BUG=692610
TEST=
LayoutTests/webaudio/unit-tests/audit.html

Review-Url: https://codereview.chromium.org/2705483002
Cr-Commit-Position: refs/heads/master@{#452074}
Committed: https://chromium.googlesource.com/chromium/src/+/c8c334dedbfa6e68e78e4aa1075a4c2ef17d5796

[hongchan, 2017-02-16 21:44:31]

hongchan@chromium.org changed reviewers:
+ rtoy@chromium.org

[hongchan, 2017-02-16 21:44:31]

PTAL.

[hongchan, 2017-02-16 21:46:27]

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

[commit-bot: I haz the power, 2017-02-16 21:48:08]

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

[Raymond Toy, 2017-02-16 21:50:40]

https://codereview.chromium.org/2705483002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/webaudio/BiquadFilter/biquadfilternode-basic-expected.txt
(right):

https://codereview.chromium.org/2705483002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/webaudio/BiquadFilter/biquadfilternode-basic-expected.txt:5:
PASS   : Basic tests for BiquadFilterNode
This looks odd. If there's a task.describe(), I think it should replace "task
started" with the description, like what we have now.

https://codereview.chromium.org/2705483002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/webaudio/resources/audit.js (right):

https://codereview.chromium.org/2705483002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/webaudio/resources/audit.js:964: _logPassed('> ['
+ this._label + '] task started.');
I think I'd prefer nothing be printed.  Because what you have now is
indistinguishable from a task description of "task started.".  (Granted, you
could also have a description of "", but that seems much less likely.)

[commit-bot: I haz the power, 2017-02-16 23:51:53]

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

[commit-bot: I haz the power, 2017-02-16 23:51:55]

Dry run: This issue passed the CQ dry run.

[hongchan, 2017-02-16 23:52:37]

Description was changed from

==========
Print out label and description on task entry in Audit task runner

This CL fixes the missing task label/description when the description
is not given.

PASS > [basic] Simple unit tests for basic assertions.

PASS > [basic] task started.
PASS   : Simple unit tests for basic assertions.

Even if the description is not given (omitting task.describe()),
the entry of task will be displayed.

BUG=692610
TEST=
LayoutTests/webaudio/unit-tests/audit.html
LayoutTests/webaudio/unit-tests/audit-failures.html
LayoutTests/webaudio/BiquadFilter/biquadfilternode-basic.html
==========

to

==========
Print out label and description on task entry in Audit task runner

This CL fixes the missing task label/description when the description
is not given.

PASS > [basic] Simple unit tests for basic assertions.

PASS > [basic] task started.
PASS   : Simple unit tests for basic assertions.

Even if the description is not given (omitting task.describe()),
the entry of task will be displayed.

BUG=692610
TEST=
LayoutTests/webaudio/unit-tests/audit.html
==========

[hongchan, 2017-02-16 23:54:49]

As we discussed offline, now |taskLabel| param supports string or object type.

https://codereview.chromium.org/2705483002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/webaudio/resources/audit.js (right):

https://codereview.chromium.org/2705483002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/webaudio/resources/audit.js:964: _logPassed('> ['
+ this._label + '] task started.');
On 2017/02/16 21:50:40, Raymond Toy wrote:
> I think I'd prefer nothing be printed.  Because what you have now is
> indistinguishable from a task description of "task started.".  (Granted, you
> could also have a description of "", but that seems much less likely.)

If your description is undefined or null, you'll get nothing.

[Raymond Toy, 2017-02-17 00:03:46]

lgtm with nit.

I think we need a test for audit.define('test') without a task.describe() to
verify the output.

Please update the CL description a bit to say we're supporting a string or
dictionary.

[Raymond Toy, 2017-02-17 18:05:13]

On 2017/02/17 00:03:46, Raymond Toy wrote:
> lgtm with nit.
> 
> I think we need a test for audit.define('test') without a task.describe() to
> verify the output.
> 
> Please update the CL description a bit to say we're supporting a string or
> dictionary.

Question:  Couldn't we get the result we want if we initialized the description
to the empty string, and always printed out the description?

I do like being able to specify the description in the first arg, the whole
point of this was to get the test name always printed out even if
task.describe() was not called.

[Raymond Toy, 2017-02-17 18:06:50]

On 2017/02/17 18:05:13, Raymond Toy wrote:
> On 2017/02/17 00:03:46, Raymond Toy wrote:
> > lgtm with nit.
> > 
> > I think we need a test for audit.define('test') without a task.describe() to
> > verify the output.
> > 
> > Please update the CL description a bit to say we're supporting a string or
> > dictionary.
> 
> Question:  Couldn't we get the result we want if we initialized the
description
> to the empty string, and always printed out the description?
> 
> I do like being able to specify the description in the first arg, the whole
> point of this was to get the test name always printed out even if
> task.describe() was not called.

Oh, never mind.  This is printed out when task.describe() is called; if it's not
called, nothing is done.

[hongchan, 2017-02-17 18:11:24]

On 2017/02/17 00:03:46, Raymond Toy wrote:
> lgtm with nit.
> 
> I think we need a test for audit.define('test') without a task.describe() to
> verify the output.
> 
> Please update the CL description a bit to say we're supporting a string or
> dictionary.

It turned out that there is an issue in the current patch set. Consider these 6
cases:

1. audit.define({ label: 'label', desc: 'desc' }, (task) => {})
- This case works fine. 

2. audit.define('label', (task) => {})
- This case is also okay. We can omit the description.

3. audit.define('label', (task) => { task.describe('desc'); })
- Problematic. the description is defined within the task and we cannot print it
the headline.

4. audit.define({ label: 'label' }, (task) => { task.describe('desc'); })
- Same problem with the case 3.

5. audit.define({ label: 'label' }, (task) => {})
- Identical to the case 2. This is okay.

6. audit.define({ label: 'label', desc: 'desc' }, (task) => {
task.describe('desc'); })
- We can treat this like the case 1 and ignore the description inside.

Due to case 3 and 4, it is hard to add the change that is compatible with the
old pattern.

I might have to bite the bullet and rewrite all describe() instances.

[Raymond Toy, 2017-02-17 18:16:31]

On 2017/02/17 18:11:24, hongchan wrote:
> On 2017/02/17 00:03:46, Raymond Toy wrote:
> > lgtm with nit.
> > 
> > I think we need a test for audit.define('test') without a task.describe() to
> > verify the output.
> > 
> > Please update the CL description a bit to say we're supporting a string or
> > dictionary.
> 
> It turned out that there is an issue in the current patch set. Consider these
6
> cases:
> 
> 1. audit.define({ label: 'label', desc: 'desc' }, (task) => {})
> - This case works fine. 
> 
> 2. audit.define('label', (task) => {})
> - This case is also okay. We can omit the description.
> 
> 3. audit.define('label', (task) => { task.describe('desc'); })
> - Problematic. the description is defined within the task and we cannot print
it
> the headline.
> 
> 4. audit.define({ label: 'label' }, (task) => { task.describe('desc'); })
> - Same problem with the case 3.
> 
> 5. audit.define({ label: 'label' }, (task) => {})
> - Identical to the case 2. This is okay.
> 
> 6. audit.define({ label: 'label', desc: 'desc' }, (task) => {
> task.describe('desc'); })
> - We can treat this like the case 1 and ignore the description inside.
> 
> Due to case 3 and 4, it is hard to add the change that is compatible with the
> old pattern.
> 
> I might have to bite the bullet and rewrite all describe() instances.

I think I'm ok if this CL makes task.describe() do nothing.  That fixes case 3
and 4 (in a less than ideal way).  Then a follow-up CL can change task.describe
everywhere and move the description to the first arg.  And then also remove
task.describe.

I think we can write a small script to do this task.describe conversion for us.

Or you can do that all in this CL.

[Raymond Toy, 2017-02-17 18:51:58]

On 2017/02/17 18:16:31, Raymond Toy wrote:
> On 2017/02/17 18:11:24, hongchan wrote:
> > On 2017/02/17 00:03:46, Raymond Toy wrote:
> > > lgtm with nit.
> > > 
> > > I think we need a test for audit.define('test') without a task.describe()
to
> > > verify the output.
> > > 
> > > Please update the CL description a bit to say we're supporting a string or
> > > dictionary.
> > 
> > It turned out that there is an issue in the current patch set. Consider
these
> 6
> > cases:
> > 
> > 1. audit.define({ label: 'label', desc: 'desc' }, (task) => {})
> > - This case works fine. 
> > 
> > 2. audit.define('label', (task) => {})
> > - This case is also okay. We can omit the description.
> > 
> > 3. audit.define('label', (task) => { task.describe('desc'); })
> > - Problematic. the description is defined within the task and we cannot
print
> it
> > the headline.
> > 
> > 4. audit.define({ label: 'label' }, (task) => { task.describe('desc'); })
> > - Same problem with the case 3.
> > 
> > 5. audit.define({ label: 'label' }, (task) => {})
> > - Identical to the case 2. This is okay.
> > 
> > 6. audit.define({ label: 'label', desc: 'desc' }, (task) => {
> > task.describe('desc'); })
> > - We can treat this like the case 1 and ignore the description inside.
> > 
> > Due to case 3 and 4, it is hard to add the change that is compatible with
the
> > old pattern.
> > 
> > I might have to bite the bullet and rewrite all describe() instances.
> 
> I think I'm ok if this CL makes task.describe() do nothing.  That fixes case 3
> and 4 (in a less than ideal way).  Then a follow-up CL can change
task.describe
> everywhere and move the description to the first arg.  And then also remove
> task.describe.
> 
> I think we can write a small script to do this task.describe conversion for
us.
> 
> Or you can do that all in this CL.

Actually, I think I prefer this to be separate, if we go down this path.

[hongchan, 2017-02-21 17:40:28]

Description was changed from

==========
Print out label and description on task entry in Audit task runner

This CL fixes the missing task label/description when the description
is not given.

PASS > [basic] Simple unit tests for basic assertions.

PASS > [basic] task started.
PASS   : Simple unit tests for basic assertions.

Even if the description is not given (omitting task.describe()),
the entry of task will be displayed.

BUG=692610
TEST=
LayoutTests/webaudio/unit-tests/audit.html
==========

to

==========
Print out label and description on task entry in Audit task runner

This CL changes how to specify the label/description of a test task.


audit.define('label', taskFunc);

audit.define({ label: 'label', description: 'description' }, taskFunc);

Even if the description is not given the entry of task will be 
displayed. Note that task.describe() will be deprecated.

BUG=692610
TEST=
LayoutTests/webaudio/unit-tests/audit.html
==========

[hongchan, 2017-02-21 17:40:40]

Description was changed from

==========
Print out label and description on task entry in Audit task runner

This CL changes how to specify the label/description of a test task.


audit.define('label', taskFunc);

audit.define({ label: 'label', description: 'description' }, taskFunc);

Even if the description is not given the entry of task will be 
displayed. Note that task.describe() will be deprecated.

BUG=692610
TEST=
LayoutTests/webaudio/unit-tests/audit.html
==========

to

==========
Print out label and description on task entry in Audit task runner

This CL changes how to specify the label/description of a test task.


audit.define('label', taskFunc);
audit.define({ label: 'label', description: 'description' }, taskFunc);


Even if the description is not given the entry of task will be 
displayed. Note that task.describe() will be deprecated.

BUG=692610
TEST=
LayoutTests/webaudio/unit-tests/audit.html
==========

[hongchan, 2017-02-21 18:06:42]

Please take one final look.

In the follow-up CL, I will remove task.describe() and use the new description
argument.

[Raymond Toy, 2017-02-21 18:37:18]

lgtm with another nit:

So does task.describe() not do anything?  If so, add a comment to that effect. 
Unless you're going to do a follow up CL right away fixing this.

https://codereview.chromium.org/2705483002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-expected.txt
(right):

https://codereview.chromium.org/2705483002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-expected.txt:36: PASS <
[dummy-label-string] All assertions passed. (total 0 assertions)
Nit, not relevant to this CL.  There are no assertions, so did they really pass?
 Should this be a warning (or error)?

[hongchan, 2017-02-21 18:42:01]

On 2017/02/21 18:37:18, Raymond Toy wrote:
> lgtm with another nit:
> 
> So does task.describe() not do anything?  If so, add a comment to that effect.

> Unless you're going to do a follow up CL right away fixing this.
> 
>
https://codereview.chromium.org/2705483002/diff/40001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-expected.txt
> (right):
> 
>
https://codereview.chromium.org/2705483002/diff/40001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-expected.txt:36: PASS
<
> [dummy-label-string] All assertions passed. (total 0 assertions)
> Nit, not relevant to this CL.  There are no assertions, so did they really
pass?
>  Should this be a warning (or error)?

Well, this is a different issue. We never thought we need a task with zero
assertion. Not critical and will address it later.

[hongchan, 2017-02-21 18:44:14]

The CQ bit was checked by hongchan@chromium.org

[hongchan, 2017-02-21 18:44:14]

The patchset sent to the CQ was uploaded after l-g-t-m from rtoy@chromium.org
Link to the patchset: https://codereview.chromium.org/2705483002/#ps60001
(title: "Clarify deprecation of task.describe()")

[commit-bot: I haz the power, 2017-02-21 18:45: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-02-21 19:58:40]

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

[commit-bot: I haz the power, 2017-02-21 19:58:41]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[hongchan, 2017-02-21 23:39:20]

The CQ bit was checked by hongchan@chromium.org

[hongchan, 2017-02-21 23:39:20]

The patchset sent to the CQ was uploaded after l-g-t-m from rtoy@chromium.org
Link to the patchset: https://codereview.chromium.org/2705483002/#ps80001
(title: "Rebased")

[commit-bot: I haz the power, 2017-02-21 23:41:54]

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

[commit-bot: I haz the power, 2017-02-22 01:45:15]

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

[commit-bot: I haz the power, 2017-02-22 01:45:15]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[hongchan, 2017-02-22 16:00:44]

The CQ bit was checked by hongchan@chromium.org

[commit-bot: I haz the power, 2017-02-22 16:01:04]

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

[commit-bot: I haz the power, 2017-02-22 16:07:44]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487779244385320,
"parent_rev": "028db2b1bf62a5ce0574b3c1e7f42e42607cb242", "commit_rev":
"c8c334dedbfa6e68e78e4aa1075a4c2ef17d5796"}

[commit-bot: I haz the power, 2017-02-22 16:08:25]

Description was changed from

==========
Print out label and description on task entry in Audit task runner

This CL changes how to specify the label/description of a test task.


audit.define('label', taskFunc);
audit.define({ label: 'label', description: 'description' }, taskFunc);


Even if the description is not given the entry of task will be 
displayed. Note that task.describe() will be deprecated.

BUG=692610
TEST=
LayoutTests/webaudio/unit-tests/audit.html
==========

to

==========
Print out label and description on task entry in Audit task runner

This CL changes how to specify the label/description of a test task.


audit.define('label', taskFunc);
audit.define({ label: 'label', description: 'description' }, taskFunc);


Even if the description is not given the entry of task will be 
displayed. Note that task.describe() will be deprecated.

BUG=692610
TEST=
LayoutTests/webaudio/unit-tests/audit.html

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

[commit-bot: I haz the power, 2017-02-22 16:08:26]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/c8c334dedbfa6e68e78e4aa1075a...