Add a general PostProcessorUnittest.

Add a general PostProcessorUnittest.

Future PostProcessors can be validated quickly using this, or it can
be used as a starting point for more specialized tests.

BUG=internal b/62593135
TEST=libcast_governor_unittests

Review-Url: https://codereview.chromium.org/2937123005
Cr-Commit-Position: refs/heads/master@{#481087}
Committed: https://chromium.googlesource.com/chromium/src/+/8431adefb3d888ec231c9f35515b8bd9c8abcda2

[bshaya, 2017-06-16 20:28:14]

Description was changed from

==========
Add a general PostProcessorUnittest.

Future PostProcessors can be validated quickly using this, or it can
be used as a starting point for more specialized tests.

Bug: 62593135
Test: libcast_governor_unittests
Change-Id: I78bbe559287212f57465913243d476e6248a2cc3
==========

to

==========
Add a general PostProcessorUnittest.

Future PostProcessors can be validated quickly using this, or it can
be used as a starting point for more specialized tests.

BUG=internal b/62593135
TEST=libcast_governor_unittests
==========

[bshaya, 2017-06-16 20:46:17]

bshaya@google.com changed reviewers:
+ gfhuang@chromium.org, kmackay@chromium.org, montvelishsky@google.com,
slan@chromium.org

[kmackay, 2017-06-16 21:12:19]

lgtm % nits

https://codereview.chromium.org/2937123005/diff/1/chromecast/media/cma/backen...
File chromecast/media/cma/backend/alsa/post_processors/post_processor_unittest.h
(right):

https://codereview.chromium.org/2937123005/diff/1/chromecast/media/cma/backen...
chromecast/media/cma/backend/alsa/post_processors/post_processor_unittest.h:8:
#include <cstdint>
is stdint used?

https://codereview.chromium.org/2937123005/diff/1/chromecast/media/cma/backen...
chromecast/media/cma/backend/alsa/post_processors/post_processor_unittest.h:16:
//  DelayTest (tests return value from ProcessFrames)
TestDelay

https://codereview.chromium.org/2937123005/diff/1/chromecast/media/cma/backen...
chromecast/media/cma/backend/alsa/post_processors/post_processor_unittest.h:17:
//  RingingTimeTest (from GetRingingTimeFrames)
TestRingingTime

https://codereview.chromium.org/2937123005/diff/1/chromecast/media/cma/backen...
chromecast/media/cma/backend/alsa/post_processors/post_processor_unittest.h:20:
//  Data passthrough (tests data in = data out, accounting for delay).
TestPassthrough

https://codereview.chromium.org/2937123005/diff/1/chromecast/media/cma/backen...
chromecast/media/cma/backend/alsa/post_processors/post_processor_unittest.h:72:
~PostProcessorTest() override;
add blank line

https://codereview.chromium.org/2937123005/diff/1/chromecast/media/cma/backen...
chromecast/media/cma/backend/alsa/post_processors/post_processor_unittest.h:76:
}  // namespace PostProcessorTest
post_processor_test

[bshaya, 2017-06-17 01:40:42]

https://codereview.chromium.org/2937123005/diff/1/chromecast/media/cma/backen...
File chromecast/media/cma/backend/alsa/post_processors/post_processor_unittest.h
(right):

https://codereview.chromium.org/2937123005/diff/1/chromecast/media/cma/backen...
chromecast/media/cma/backend/alsa/post_processors/post_processor_unittest.h:8:
#include <cstdint>
On 2017/06/16 21:12:19, kmackay wrote:
> is stdint used?

Done.

https://codereview.chromium.org/2937123005/diff/1/chromecast/media/cma/backen...
chromecast/media/cma/backend/alsa/post_processors/post_processor_unittest.h:16:
//  DelayTest (tests return value from ProcessFrames)
On 2017/06/16 21:12:19, kmackay wrote:
> TestDelay

Done.

https://codereview.chromium.org/2937123005/diff/1/chromecast/media/cma/backen...
chromecast/media/cma/backend/alsa/post_processors/post_processor_unittest.h:17:
//  RingingTimeTest (from GetRingingTimeFrames)
On 2017/06/16 21:12:19, kmackay wrote:
> TestRingingTime

Done.

https://codereview.chromium.org/2937123005/diff/1/chromecast/media/cma/backen...
chromecast/media/cma/backend/alsa/post_processors/post_processor_unittest.h:20:
//  Data passthrough (tests data in = data out, accounting for delay).
On 2017/06/16 21:12:19, kmackay wrote:
> TestPassthrough

Done.

https://codereview.chromium.org/2937123005/diff/1/chromecast/media/cma/backen...
chromecast/media/cma/backend/alsa/post_processors/post_processor_unittest.h:72:
~PostProcessorTest() override;
On 2017/06/16 21:12:19, kmackay wrote:
> add blank line

Done.

https://codereview.chromium.org/2937123005/diff/1/chromecast/media/cma/backen...
chromecast/media/cma/backend/alsa/post_processors/post_processor_unittest.h:76:
}  // namespace PostProcessorTest
On 2017/06/16 21:12:19, kmackay wrote:
> post_processor_test

Done.

[bshaya, 2017-06-17 01:40:51]

The CQ bit was checked by bshaya@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-17 01:41:02]

Dry run: CQ is trying da patch.

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

[mont, 2017-06-17 01:50:47]

lgtm

lgtm

[mont, 2017-06-17 01:51:05]

lgtm

[commit-bot: I haz the power, 2017-06-17 03:02:03]

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

[commit-bot: I haz the power, 2017-06-17 03:02:03]

Dry run: This issue passed the CQ dry run.

[bshaya, 2017-06-19 17:37:07]

The CQ bit was checked by bshaya@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-19 17:37:45]

Dry run: CQ is trying da patch.

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

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

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

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

Dry run: This issue passed the CQ dry run.

[slan, 2017-06-19 20:50:36]

The test code l-g-t-m. I am only concerned about the atypical GN pattern.

https://codereview.chromium.org/2937123005/diff/20001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/alsa/BUILD.gn (right):

https://codereview.chromium.org/2937123005/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/BUILD.gn:192: "//base/test:run_all_unittests",
Why link this in at this level? This means that this can't be linked as even an
indirect dependency of any other target. 

For that matter, why not just add these files to the test target directly?

[bshaya, 2017-06-19 21:22:48]

https://codereview.chromium.org/2937123005/diff/20001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/alsa/BUILD.gn (right):

https://codereview.chromium.org/2937123005/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/BUILD.gn:192: "//base/test:run_all_unittests",
On 2017/06/19 20:50:36, slan wrote:
> Why link this in at this level? This means that this can't be linked as even
an
> indirect dependency of any other target. 
> 
> For that matter, why not just add these files to the test target directly?

Removed run_all_unittests as a dependency here.

These files will be included in various parts of the chromecast  where
post-processors are defined, so it is useful to have a source_set for them
(libcast_governor_unittests will not be their only use).

[slan, 2017-06-19 21:26:17]

On 2017/06/19 21:22:48, bshaya wrote:
>
https://codereview.chromium.org/2937123005/diff/20001/chromecast/media/cma/ba...
> File chromecast/media/cma/backend/alsa/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2937123005/diff/20001/chromecast/media/cma/ba...
> chromecast/media/cma/backend/alsa/BUILD.gn:192:
"//base/test:run_all_unittests",
> On 2017/06/19 20:50:36, slan wrote:
> > Why link this in at this level? This means that this can't be linked as even
> an
> > indirect dependency of any other target. 
> > 
> > For that matter, why not just add these files to the test target directly?
> 
> Removed run_all_unittests as a dependency here.
> 
> These files will be included in various parts of the chromecast  where
> post-processors are defined, so it is useful to have a source_set for them
> (libcast_governor_unittests will not be their only use).

SGTM - the more conventional GN name for a target like this is test_support, but
I won't stick on that.

lgtm!

[bshaya, 2017-06-19 22:05:27]

On 2017/06/19 21:26:17, slan wrote:
> On 2017/06/19 21:22:48, bshaya wrote:
> >
>
https://codereview.chromium.org/2937123005/diff/20001/chromecast/media/cma/ba...
> > File chromecast/media/cma/backend/alsa/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/2937123005/diff/20001/chromecast/media/cma/ba...
> > chromecast/media/cma/backend/alsa/BUILD.gn:192:
> "//base/test:run_all_unittests",
> > On 2017/06/19 20:50:36, slan wrote:
> > > Why link this in at this level? This means that this can't be linked as
even
> > an
> > > indirect dependency of any other target. 
> > > 
> > > For that matter, why not just add these files to the test target directly?
> > 
> > Removed run_all_unittests as a dependency here.
> > 
> > These files will be included in various parts of the chromecast  where
> > post-processors are defined, so it is useful to have a source_set for them
> > (libcast_governor_unittests will not be their only use).
> 
> SGTM - the more conventional GN name for a target like this is test_support,
but
> I won't stick on that.
> 
> lgtm!

Done

[bshaya, 2017-06-19 22:05:32]

The CQ bit was checked by bshaya@google.com

[bshaya, 2017-06-19 22:05:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
montvelishsky@google.com, kmackay@chromium.org, slan@chromium.org
Link to the patchset: https://codereview.chromium.org/2937123005/#ps60001
(title: "rename test to test_support")

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

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

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

[commit-bot: I haz the power, 2017-06-20 00:21:36]

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...)

[bshaya, 2017-06-21 00:44:36]

The CQ bit was checked by bshaya@google.com

[commit-bot: I haz the power, 2017-06-21 00:44:56]

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 02:26:48]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498005876448370,
"parent_rev": "c7afae5cf0f1f03bd454443a9a637bd94fdc8656", "commit_rev":
"8431adefb3d888ec231c9f35515b8bd9c8abcda2"}

[commit-bot: I haz the power, 2017-06-21 02:27:00]

Description was changed from

==========
Add a general PostProcessorUnittest.

Future PostProcessors can be validated quickly using this, or it can
be used as a starting point for more specialized tests.

BUG=internal b/62593135
TEST=libcast_governor_unittests
==========

to

==========
Add a general PostProcessorUnittest.

Future PostProcessors can be validated quickly using this, or it can
be used as a starting point for more specialized tests.

BUG=internal b/62593135
TEST=libcast_governor_unittests

Review-Url: https://codereview.chromium.org/2937123005
Cr-Commit-Position: refs/heads/master@{#481087}
Committed:
https://chromium.googlesource.com/chromium/src/+/8431adefb3d888ec231c9f35515b...
==========

[commit-bot: I haz the power, 2017-06-21 02:27:02]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/8431adefb3d888ec231c9f35515b...