Add a lot of Pipeline tests to cover stopping/error handling while in a variety of states.

Add a lot of Pipeline tests to cover stopping/error handling while in a variety of states.

While the use of gmock's InSequence is unfortunate (read: brittle tests), it does help capture the call sequences for various pipeline operations. You may notice in the original incarnation of these tests in http://codereview.chromium.org/10828045/ didn't need InSequence. I'm aiming to remove InSequence after I'm done rewriting the guts of Pipeline.

BUG=110228
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=150816

[scherkus (not reviewing), 2012-08-08 03:47:00]

As mentioned in http://codereview.chromium.org/10826196/

These are originally from http://codereview.chromium.org/10828045/ -- I expect
the gmock expectations to change as I start collapsing states and eliminating
unnecessary sequences (i.e., pause+flush on shutdown, waiting for seeks to
complete before shutdown)

[Ami GONE FROM CHROMIUM, 2012-08-08 17:21:08]

Basically LGTM but it'd be nice to de-dup the tests as much as possible.  ATM
there's a lot of boilerplate which makes it harder than necessary to see what's
being tested.

https://chromiumcodereview.appspot.com/10832197/diff/3001/media/base/pipeline...
File media/base/pipeline_unittest.cc (right):

https://chromiumcodereview.appspot.com/10832197/diff/3001/media/base/pipeline...
media/base/pipeline_unittest.cc:22: using ::testing::InSequence;
TODO(scherkus): drop InSequence from this file.

https://chromiumcodereview.appspot.com/10832197/diff/3001/media/base/pipeline...
media/base/pipeline_unittest.cc:52: ACTION(RunPipelineStatusCB1) {
FWIW, this is a dup of RunPipelineStatusCB in mock_filters.h.

https://chromiumcodereview.appspot.com/10832197/diff/3001/media/base/pipeline...
media/base/pipeline_unittest.cc:296: void SetupShutdownTest() {
I can't tell from this impl, nor from the name, what this function has to do
with shutdown.

https://chromiumcodereview.appspot.com/10832197/diff/3001/media/base/pipeline...
media/base/pipeline_unittest.cc:503: TEST_F(PipelineTest, Stop_DuringPause) {
Stop_During{Pause,Flush,Seek} look like they can be seriously de-duped.

https://chromiumcodereview.appspot.com/10832197/diff/3001/media/base/pipeline...
media/base/pipeline_unittest.cc:605: // TODO(scherkus): Don't pause+flush on
shutdown, see http://crbug.com/110228
Can you pull out this stanza into a helper ExpectPauseFlushStopDance()?

[scherkus (not reviewing), 2012-08-08 22:14:29]

PTAL -- this time w/ helpers

feel free to nit on naming and such

http://codereview.chromium.org/10832197/diff/3001/media/base/pipeline_unittes...
File media/base/pipeline_unittest.cc (right):

http://codereview.chromium.org/10832197/diff/3001/media/base/pipeline_unittes...
media/base/pipeline_unittest.cc:52: ACTION(RunPipelineStatusCB1) {
On 2012/08/08 17:21:08, Ami Fischman wrote:
> FWIW, this is a dup of RunPipelineStatusCB in mock_filters.h.

you're living in the past, man!!! see http://crrev.com/147619

http://codereview.chromium.org/10832197/diff/3001/media/base/pipeline_unittes...
media/base/pipeline_unittest.cc:296: void SetupShutdownTest() {
On 2012/08/08 17:21:08, Ami Fischman wrote:
> I can't tell from this impl, nor from the name, what this function has to do
> with shutdown.

Done.

[Ami GONE FROM CHROMIUM, 2012-08-08 23:43:55]

http://codereview.chromium.org/10832197/diff/6001/media/base/pipeline_unittes...
File media/base/pipeline_unittest.cc (right):

http://codereview.chromium.org/10832197/diff/6001/media/base/pipeline_unittes...
media/base/pipeline_unittest.cc:53: ACTION(RunPipelineStatusCB1) {
s/1//

http://codereview.chromium.org/10832197/diff/6001/media/base/pipeline_unittes...
media/base/pipeline_unittest.cc:313: enum StopOnSeekState {
I read your TODO above, but AFAICT these are only used to signal when you want
the Stop to be triggered.  So in fact they can be unified into
enum SeekDancePhase { kPause, kFlush, kSeek };
and then the methods below turn into
void TestStopOrErrorDuringSeekDance(SeekDancePhase, StopOrError);
with an
enum StopOrError { kStop, kError };

Once you do that, then it's unclear to me why not just fully parameterize the
tests; this test helper then just takes the state of the pipeline (playing/not)
and the phase in which to trigger stop, and goes to town.

http://codereview.chromium.org/10832197/diff/6001/media/base/pipeline_unittes...
media/base/pipeline_unittest.cc:319: enum ErrorOnSeekState {
Ditto.

http://codereview.chromium.org/10832197/diff/6001/media/base/pipeline_unittes...
media/base/pipeline_unittest.cc:346:
RunPipelineStatusCB1WithStatus(PIPELINE_OK)));
Isn't this just RunPipelineStatusCB1()?

[scherkus (not reviewing), 2012-08-09 01:18:19]

isn't reviewing all this code fun!??!?!? :)

http://codereview.chromium.org/10832197/diff/6001/media/base/pipeline_unittes...
File media/base/pipeline_unittest.cc (right):

http://codereview.chromium.org/10832197/diff/6001/media/base/pipeline_unittes...
media/base/pipeline_unittest.cc:53: ACTION(RunPipelineStatusCB1) {
On 2012/08/08 23:43:55, Ami Fischman wrote:
> s/1//

I swore we had other versions around but I guess I deleted them all

http://codereview.chromium.org/10832197/diff/6001/media/base/pipeline_unittes...
media/base/pipeline_unittest.cc:313: enum StopOnSeekState {
On 2012/08/08 23:43:55, Ami Fischman wrote:
> I read your TODO above, but AFAICT these are only used to signal when you want
> the Stop to be triggered.  So in fact they can be unified into
> enum SeekDancePhase { kPause, kFlush, kSeek };
> and then the methods below turn into
> void TestStopOrErrorDuringSeekDance(SeekDancePhase, StopOrError);
> with an
> enum StopOrError { kStop, kError };
> 
> Once you do that, then it's unclear to me why not just fully parameterize the
> tests; this test helper then just takes the state of the pipeline
(playing/not)
> and the phase in which to trigger stop, and goes to town.

Took a stab but I strongly dislike how gtest parameterized tests look on the
console so I manually instantiated them myself

http://codereview.chromium.org/10832197/diff/6001/media/base/pipeline_unittes...
media/base/pipeline_unittest.cc:346:
RunPipelineStatusCB1WithStatus(PIPELINE_OK)));
On 2012/08/08 23:43:55, Ami Fischman wrote:
> Isn't this just RunPipelineStatusCB1()?

Done.

[Ami GONE FROM CHROMIUM, 2012-08-09 04:41:43]

LGTM
I <3 this test.

http://codereview.chromium.org/10832197/diff/6001/media/base/pipeline_unittes...
File media/base/pipeline_unittest.cc (right):

http://codereview.chromium.org/10832197/diff/6001/media/base/pipeline_unittes...
media/base/pipeline_unittest.cc:313: enum StopOnSeekState {
On 2012/08/09 01:18:19, scherkus wrote:
> On 2012/08/08 23:43:55, Ami Fischman wrote:
> > I read your TODO above, but AFAICT these are only used to signal when you
want
> > the Stop to be triggered.  So in fact they can be unified into
> > enum SeekDancePhase { kPause, kFlush, kSeek };
> > and then the methods below turn into
> > void TestStopOrErrorDuringSeekDance(SeekDancePhase, StopOrError);
> > with an
> > enum StopOrError { kStop, kError };
> > 
> > Once you do that, then it's unclear to me why not just fully parameterize
the
> > tests; this test helper then just takes the state of the pipeline
> (playing/not)
> > and the phase in which to trigger stop, and goes to town.
> 
> Took a stab but I strongly dislike how gtest parameterized tests look on the
> console so I manually instantiated them myself

Yeah, I didn't mean TEST_P necessarily.

http://codereview.chromium.org/10832197/diff/1006/media/base/pipeline_unittes...
File media/base/pipeline_unittest.cc (right):

http://codereview.chromium.org/10832197/diff/1006/media/base/pipeline_unittes...
media/base/pipeline_unittest.cc:1027: // TODO(scherkus): Why separate stop/error
enums and helper functions? We do
rewrite comment.

[scherkus (not reviewing), 2012-08-09 16:52:39]

http://codereview.chromium.org/10832197/diff/1006/media/base/pipeline_unittes...
File media/base/pipeline_unittest.cc (right):

http://codereview.chromium.org/10832197/diff/1006/media/base/pipeline_unittes...
media/base/pipeline_unittest.cc:1027: // TODO(scherkus): Why separate stop/error
enums and helper functions? We do
On 2012/08/09 04:41:43, Ami Fischman wrote:
> rewrite comment.

Done.