Refactor FFmpegDemuxerTest to use gmock and eliminate flakiness.

Refactor FFmpegDemuxerTest to use gmock and eliminate flakiness.

This eliminates all final traces of the old global-functions-and-variables style of mocking, which was a massive headache to maintain and verify correctness.

No new tests have been added, however gmock creates much stronger assertions for the tests themselves.

I also made FFmpegDemuxerTest a friend of FFmpegDemuxer to let tests verify that all tasks on the internal demuxing thread have completed.

BUG=13933
TEST=FFmpegDemuxerTest should more awesome and less flaky

[scherkus (not reviewing), 2009-06-17 23:24:19]

It's a substantial change, so I'm not sure how much value there is in the diffs
:(

cc'd kylep so he can see some unit tests and gmock in action!

[awong, 2009-06-17 23:54:49]

couple of small comments, but looks awesome!  You're like a gmock ninja now with
all the new fangled checkpoints, custom actions, etc. :)

http://codereview.chromium.org/126306/diff/1/2
File media/base/mock_ffmpeg.cc (right):

http://codereview.chromium.org/126306/diff/1/2#newcode88
Line 88: // implement a cheap version that's capable of overflowing.
Was noticing this in my own code today.  At some point, we'll want a set of
accuracy and regression tests that can be run against ffmpeg's implementation to
ensure none of our expectations have problems.

http://codereview.chromium.org/126306/diff/1/3
File media/base/mock_ffmpeg.h (right):

http://codereview.chromium.org/126306/diff/1/3#newcode47
Line 47: static void DestructPacket(AVPacket* packet);
Isn't there a non-test version of this function somewhere?

http://codereview.chromium.org/126306/diff/1/3#newcode51
Line 51: template <class, class, class>
OMG...you're kidding.

template <class, class, class>?

Is there macro in gmock somewhere to prettify this?

http://codereview.chromium.org/126306/diff/1/3#newcode61
Line 61: int outstanding_packets_;
Don't we have a heapcheck on our unittests?  Do we need this to verify parity of
calls?

I'm tempted to expose an increment/decrement package public API into MockFFmpeg
instead of friending these thigns.

http://codereview.chromium.org/126306/diff/1/6
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/126306/diff/1/6#newcode28
Line 28: class NotifyTask : public Task {
Huh...can't we use a NewRunnableTask to create this somehow?

http://codereview.chromium.org/126306/diff/1/6#newcode68
Line 68: static const int kDurations[AV_STREAM_MAX];
Why declare teh size here but not in kAudioData/kVideoData?

http://codereview.chromium.org/126306/diff/1/6#newcode149
Line 149: EXPECT_TRUE(demuxer_->Initialize(data_source_.get()));
For these functions, can you verify that on a test failure, the log message
gives enough informaiton to know which test it came from?

http://codereview.chromium.org/126306/diff/1/6#newcode290
Line 290: TEST_F(FFmpegDemuxerTest, Read) {
Explain what the main invaraints at a high level that we're checking.

[scherkus (not reviewing), 2009-06-18 00:29:28]

http://codereview.chromium.org/126306/diff/1/3
File media/base/mock_ffmpeg.h (right):

http://codereview.chromium.org/126306/diff/1/3#newcode47
Line 47: static void DestructPacket(AVPacket* packet);
On 2009/06/17 23:54:49, awong wrote:
> Isn't there a non-test version of this function somewhere?
not that I'm aware of..

one used to be declared in ffmpeg_demuxer_unittest.cc

ffmpeg usually sets the function pointer to an internal method inside the
library itself, which we don't have in this case

http://codereview.chromium.org/126306/diff/1/3#newcode51
Line 51: template <class, class, class>
On 2009/06/17 23:54:49, awong wrote:
> OMG...you're kidding.
> 
> template <class, class, class>?
> 
> Is there macro in gmock somewhere to prettify this?

don't believe so :(

http://code.google.com/p/googlemock/wiki/CookBook#Using_the_ACTION_Object's_Type

http://codereview.chromium.org/126306/diff/1/3#newcode61
Line 61: int outstanding_packets_;
On 2009/06/17 23:54:49, awong wrote:
> Don't we have a heapcheck on our unittests?  Do we need this to verify parity
of
> calls?
> 
> I'm tempted to expose an increment/decrement package public API into
MockFFmpeg
> instead of friending these thigns.

We are essentially checking for parity.  But that's why the mocks are awesome --
we don't actually allocate any real memory on packet->data (although
av_new_packet() does).

We *could* allocate memory on packet->data, set the destruct function pointer
and then let purify/valgrind catch the heap leak, but those take hours to run.

I'll add an "inc/dec_outstanding_packets()" though.

http://codereview.chromium.org/126306/diff/1/6
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/126306/diff/1/6#newcode28
Line 28: class NotifyTask : public Task {
On 2009/06/17 23:54:49, awong wrote:
> Huh...can't we use a NewRunnableTask to create this somehow?

whoops forgot about NewRunnableFunction

however we can't use NewRunnableMethod because it'll require FFmpegDemuxerTest
to be refcounted, and we'll end up deleting ourself when the task finishes.

http://codereview.chromium.org/126306/diff/1/6#newcode68
Line 68: static const int kDurations[AV_STREAM_MAX];
On 2009/06/17 23:54:49, awong wrote:
> Why declare teh size here but not in kAudioData/kVideoData?

kDataSize declares their size, but C++ doesn't let us use that constant when
declaring kAudioData/kVideoData

using an enum was sort of cheating

http://codereview.chromium.org/126306/diff/1/6#newcode149
Line 149: EXPECT_TRUE(demuxer_->Initialize(data_source_.get()));
On 2009/06/17 23:54:49, awong wrote:
> For these functions, can you verify that on a test failure, the log message
> gives enough informaiton to know which test it came from?

these are called at the top of tests, so I can make these ASSERTs so the test
will start and immediately terminate

the mocks would be trickier :(

http://codereview.chromium.org/126306/diff/1/6#newcode290
Line 290: TEST_F(FFmpegDemuxerTest, Read) {
On 2009/06/17 23:54:49, awong wrote:
> Explain what the main invaraints at a high level that we're checking.

Done.

[awong, 2009-06-18 00:38:08]

http://codereview.chromium.org/126306/diff/1/6
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/126306/diff/1/6#newcode68
Line 68: static const int kDurations[AV_STREAM_MAX];
On 2009/06/18 00:29:28, scherkus wrote:
> On 2009/06/17 23:54:49, awong wrote:
> > Why declare teh size here but not in kAudioData/kVideoData?
> 
> kDataSize declares their size, but C++ doesn't let us use that constant when
> declaring kAudioData/kVideoData
> 
> using an enum was sort of cheating

Wasn't so much the enum I was complaining about as the fact that the
AV_STREAM_MAX here I think does nothing. But I could be wrong.

http://codereview.chromium.org/126306/diff/1/6#newcode149
Line 149: EXPECT_TRUE(demuxer_->Initialize(data_source_.get()));
On 2009/06/18 00:29:28, scherkus wrote:
> On 2009/06/17 23:54:49, awong wrote:
> > For these functions, can you verify that on a test failure, the log message
> > gives enough informaiton to know which test it came from?
> 
> these are called at the top of tests, so I can make these ASSERTs so the test
> will start and immediately terminate
> 
> the mocks would be trickier :(

EXPECTs are fine still I think.  Infact, ASSERTs don't have the right behavior;
inside a subfunction, they don't actually abort the test.

The issue is that I think the macro reports the failure in the "current"
function.  I guess InitializeDemuxer() is only called once per test so it's not
a big deal. However, if you called it more than once per test, you wouln't know
which instance failed.

http://codereview.chromium.org/126306/diff/3003/3008#newcode160
Line 160: wait_for_demuxer_.Wait();
nice :)

http://codereview.chromium.org/126306/diff/3003/3008#newcode279
Line 279: stream = demuxer_->GetStream(DS_STREAM_AUDIO);
I usually put these kinda of comments inside the TEST_F function since the
declaration is kinda funky already.

Oh, and newline after the colon. :)

http://codereview.chromium.org/126306/diff/3003/3008#newcode282
Line 282: EXPECT_TRUE(stream->media_format().GetAsString(MediaFormat::kMimeType,
Make much more sense.  thanks for addin git. :)

http://codereview.chromium.org/126306/diff/3003/3008#newcode403
Line 403: }
Newline after the colon.

[Alpha Left Google, 2009-06-18 18:39:11]

Awsome use of gmock!

http://codereview.chromium.org/126306/diff/3003/3004
File media/base/mock_ffmpeg.cc (right):

http://codereview.chromium.org/126306/diff/3003/3004#newcode28
Line 28: DCHECK_GT(outstanding_packets_, 0);
I think this should be CHECK_GT because you can catch this problem in release
build.

http://codereview.chromium.org/126306/diff/3003/3005
File media/base/mock_ffmpeg.h (right):

http://codereview.chromium.org/126306/diff/3003/3005#newcode47
Line 47: static void DestructPacket(AVPacket* packet);
Is this a function pointer?

http://codereview.chromium.org/126306/diff/3003/3005#newcode96
Line 96: // Call the destructor if present, such as in NewPacket().
such as the destructor prepared in NewPacket() ?

http://codereview.chromium.org/126306/diff/3003/3008
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/126306/diff/3003/3008#newcode267
Line 267: // Other stream should be audio and support FFmpegDemuxerStream
interface.
nit: s/support/supports/

http://codereview.chromium.org/126306/diff/3003/3008#newcode281
Line 281: //      reading until it finds a packet it cares about
nit: period at the end.

http://codereview.chromium.org/126306/diff/3003/3008#newcode324
Line 324: // must call av_free_packet().
should mention the end of stream packet reaches both the audio and video demuxer
stream, thus there must be two av_free_packet().

http://codereview.chromium.org/126306/diff/3003/3008#newcode404
Line 404: //   1) The demuxer frees all queued packets when it receives a Seek()
period at the end.

[scherkus (not reviewing), 2009-06-18 22:50:12]

kablamo!

http://codereview.chromium.org/126306/diff/1/6
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/126306/diff/1/6#newcode149
Line 149: EXPECT_TRUE(demuxer_->Initialize(data_source_.get()));
On 2009/06/18 00:38:08, awong wrote:
> On 2009/06/18 00:29:28, scherkus wrote:
> > On 2009/06/17 23:54:49, awong wrote:
> > > For these functions, can you verify that on a test failure, the log
message
> > > gives enough informaiton to know which test it came from?
> > 
> > these are called at the top of tests, so I can make these ASSERTs so the
test
> > will start and immediately terminate
> > 
> > the mocks would be trickier :(
> 
> EXPECTs are fine still I think.  Infact, ASSERTs don't have the right
behavior;
> inside a subfunction, they don't actually abort the test.
> 
> The issue is that I think the macro reports the failure in the "current"
> function.  I guess InitializeDemuxer() is only called once per test so it's
not
> a big deal. However, if you called it more than once per test, you wouln't
know
> which instance failed.

Right.. true we only call it once per test.  And I forgot that ASSERT simply
calls return :)

http://codereview.chromium.org/126306/diff/3003/3004
File media/base/mock_ffmpeg.cc (right):

http://codereview.chromium.org/126306/diff/3003/3004#newcode28
Line 28: DCHECK_GT(outstanding_packets_, 0);
On 2009/06/18 18:39:12, Alpha wrote:
> I think this should be CHECK_GT because you can catch this problem in release
> build.

Done.

The CHECK_* macros don't exist unfortunately :\

http://codereview.chromium.org/126306/diff/3003/3005
File media/base/mock_ffmpeg.h (right):

http://codereview.chromium.org/126306/diff/3003/3005#newcode47
Line 47: static void DestructPacket(AVPacket* packet);
On 2009/06/18 18:39:12, Alpha wrote:
> Is this a function pointer?

Done.

http://codereview.chromium.org/126306/diff/3003/3005#newcode96
Line 96: // Call the destructor if present, such as in NewPacket().
On 2009/06/18 18:39:12, Alpha wrote:
> such as the destructor prepared in NewPacket() ?

Done.

http://codereview.chromium.org/126306/diff/3003/3008
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/126306/diff/3003/3008#newcode267
Line 267: // Other stream should be audio and support FFmpegDemuxerStream
interface.
On 2009/06/18 18:39:12, Alpha wrote:
> nit: s/support/supports/

Done.

http://codereview.chromium.org/126306/diff/3003/3008#newcode279
Line 279: // We're testing the following:
On 2009/06/18 00:38:08, awong wrote:
> I usually put these kinda of comments inside the TEST_F function since the
> declaration is kinda funky already.
> 
> Oh, and newline after the colon. :)

Done.

http://codereview.chromium.org/126306/diff/3003/3008#newcode281
Line 281: //      reading until it finds a packet it cares about
On 2009/06/18 18:39:12, Alpha wrote:
> nit: period at the end.

Done.

http://codereview.chromium.org/126306/diff/3003/3008#newcode324
Line 324: // must call av_free_packet().
On 2009/06/18 18:39:12, Alpha wrote:
> should mention the end of stream packet reaches both the audio and video
demuxer
> stream, thus there must be two av_free_packet().

Done.

http://codereview.chromium.org/126306/diff/3003/3008#newcode403
Line 403: // We're testing the following:
On 2009/06/18 00:38:08, awong wrote:
> Newline after the colon.

Done.

http://codereview.chromium.org/126306/diff/3003/3008#newcode404
Line 404: //   1) The demuxer frees all queued packets when it receives a Seek()
On 2009/06/18 18:39:12, Alpha wrote:
> period at the end.

Done.

[scherkus (not reviewing), 2009-06-19 02:22:11]

Added SCOPED_TRACE

[awong, 2009-06-19 17:51:04]

LGTM