Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2808583002:
RELAND: Media Remoting end to end integration tests. (Closed)
|
Created:
3 years, 8 months ago by xjz Modified:
3 years, 8 months ago CC:
apacible+watch_chromium.org, chromium-reviews, erickung+watch_chromium.org, feature-media-reviews_chromium.org, miu+watch_chromium.org, xjz+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRELAND: Media Remoting end to end integration tests.
This is a re-land of https://codereview.chromium.org/2692593002/.
Moved tests for media remoting pipeline out of general
PipelineIntegrationTest.
-------Description of original change follows-------
Media Remoting: End to end integration tests.
Add end to end integration tests for Media Remoting. Refactors
PipelineIntegrationTest to test both media and media remoting pipeline.
Re-use current tests. No new tests are added in this CL.
BUG=684065
Review-Url: https://codereview.chromium.org/2808583002
Cr-Commit-Position: refs/heads/master@{#466216}
Committed: https://chromium.googlesource.com/chromium/src/+/722e67b4d30548006c685f81bfcd6a88858168e1
Patch Set 1 : Final patch set from original change #Patch Set 2 : Fix compile error. #
Total comments: 4
Patch Set 3 : Reduced the number of test cases for media remoting. #Patch Set 4 : Rebased. #Patch Set 5 : Separate media remoting tests. #Patch Set 6 : Rebased. #
Total comments: 30
Patch Set 7 : Addressed DaleCurtis's comments. #
Total comments: 12
Patch Set 8 : Addressed DaleCurtis's comments from PS 7. #Patch Set 9 : Rebased. #
Messages
Total messages: 62 (40 generated)
Description was changed from ========== RELAND: Media Remoting end to end integration tests. This is a re-land of https://codereview.chromium.org/2692593002/ with fix of compilor errors. -------Description of original change follows------- Media Remoting: End to end integration tests. Add end to end integration tests for Media Remoting. Refactors PipelineIntegrationTest to test both media and media remoting pipeline. Re-use current tests. No new tests are added in this CL. BUG=684065 ========== to ========== RELAND: Media Remoting end to end integration tests. This is a re-land of https://codereview.chromium.org/2692593002/ with fix of compile warnings. -------Description of original change follows------- Media Remoting: End to end integration tests. Add end to end integration tests for Media Remoting. Refactors PipelineIntegrationTest to test both media and media remoting pipeline. Re-use current tests. No new tests are added in this CL. BUG=684065 ==========
xjz@chromium.org changed reviewers: + miu@chromium.org, sandersd@chromium.org
The CQ bit was checked by xjz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Fixed compile "unreachable code" error. PTAL. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2808583002/diff/20001/media/test/pipeline_int... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2808583002/diff/20001/media/test/pipeline_int... media/test/pipeline_integration_test.cc:1153: #if !defined(DISABLE_CLOCKLESS_TESTS) This is prone to errors, since it's easy for future developers to add tests in between the #if...#endif without realizing it's there. How about you revert back to using the MAYBE_CLOCKLESS() macro, as in the original code? This would also make things more in-line with how conditional tests are supposed to be defined, according to the style guide.
https://codereview.chromium.org/2808583002/diff/20001/media/test/pipeline_int... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2808583002/diff/20001/media/test/pipeline_int... media/test/pipeline_integration_test.cc:1153: #if !defined(DISABLE_CLOCKLESS_TESTS) On 2017/04/09 03:25:53, miu wrote: > This is prone to errors, since it's easy for future developers to add tests in > between the #if...#endif without realizing it's there. > > How about you revert back to using the MAYBE_CLOCKLESS() macro, as in the > original code? This would also make things more in-line with how conditional > tests are supposed to be defined, according to the style guide. Is there a way to make the MAYBE_CLOCKLESS() macro work for parameterized tests(TEST_P). For example, for this test, with the macro, the test name became "CommonPipelineIntegrationTest.MAYBE_CLOCKLESS(BasicPlaybackOpusOggTrimmingHashed)/0", which didn't apply the macro.
https://codereview.chromium.org/2808583002/diff/20001/media/test/pipeline_int... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2808583002/diff/20001/media/test/pipeline_int... media/test/pipeline_integration_test.cc:1153: #if !defined(DISABLE_CLOCKLESS_TESTS) On 2017/04/10 17:14:52, xjz wrote: > On 2017/04/09 03:25:53, miu wrote: > > This is prone to errors, since it's easy for future developers to add tests in > > between the #if...#endif without realizing it's there. > > > > How about you revert back to using the MAYBE_CLOCKLESS() macro, as in the > > original code? This would also make things more in-line with how conditional > > tests are supposed to be defined, according to the style guide. > > Is there a way to make the MAYBE_CLOCKLESS() macro work for parameterized > tests(TEST_P). For example, for this test, with the macro, the test name became > "CommonPipelineIntegrationTest.MAYBE_CLOCKLESS(BasicPlaybackOpusOggTrimmingHashed)/0", > which didn't apply the macro. Dale has requested that the remoting tests be limited to a smaller portion of these tests, to improve testing performance. If the tests affected by clockless playback are not included, then this won't need to be solved. Is that a potential solution here?
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
Yeah, this is too big and these tests take quite a bit of time to run (compare --single-process-tests before and after). Instead please have remoting_unittests instantiate it's own subclass of media::PipelineIntegrationTestBase with some cherry-picked test cases. There's no reason to run all of these through the remoting path.
On 2017/04/10 22:31:43, DaleCurtis wrote: > Yeah, this is too big and these tests take quite a bit of time to run (compare > --single-process-tests before and after). Instead please have remoting_unittests > instantiate it's own subclass of media::PipelineIntegrationTestBase with some > cherry-picked test cases. There's no reason to run all of these through the > remoting path. Our intention was to test the media remoting path thoroughly to make sure the RPCs/Controls work properly. Tried compare --single-process-tests before and after on my local work station, the difference to run all media_unittests is 194s vs 295s for release, and 310s vs 424s for debug. If this is a concern, I'll try remove some tests for the remoting path. WDYT? +miu@
That's a fair goal, but most of these tests are for testing codec specific functionality that you're not worried about; i.e. end trimming in opus, mp3 toc issues, etc. I think all you really want are ~3-4 basic mse and src= playback cases. These belong in a custom remoting subclass of PipelineIntegrationTestBase instead of as a parameterized version of our pipeline tests.
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by xjz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by xjz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Reduced the number of test cases for media remoting pipeline. Now doesn't test codec specific cases, but only some basic playback, track status change, config change, and seeking cases. Still use the parameterized tests to avoid duplicating the test cases. PTAL. Thanks! https://codereview.chromium.org/2808583002/diff/20001/media/test/pipeline_int... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2808583002/diff/20001/media/test/pipeline_int... media/test/pipeline_integration_test.cc:1153: #if !defined(DISABLE_CLOCKLESS_TESTS) On 2017/04/10 22:26:24, sandersd wrote: > On 2017/04/10 17:14:52, xjz wrote: > > On 2017/04/09 03:25:53, miu wrote: > > > This is prone to errors, since it's easy for future developers to add tests > in > > > between the #if...#endif without realizing it's there. > > > > > > How about you revert back to using the MAYBE_CLOCKLESS() macro, as in the > > > original code? This would also make things more in-line with how conditional > > > tests are supposed to be defined, according to the style guide. > > > > Is there a way to make the MAYBE_CLOCKLESS() macro work for parameterized > > tests(TEST_P). For example, for this test, with the macro, the test name > became > > > "CommonPipelineIntegrationTest.MAYBE_CLOCKLESS(BasicPlaybackOpusOggTrimmingHashed)/0", > > which didn't apply the macro. > > Dale has requested that the remoting tests be limited to a smaller portion of > these tests, to improve testing performance. If the tests affected by clockless > playback are not included, then this won't need to be solved. Is that a > potential solution here? Done. Now don't test these for media remoting pipeline.
Sorry if I wasn't clear, I meant you should not put these in
PipelineIntegrationTest, they should live in some remoting specific test
package. You should add a new remoting/remoting_integration_test.{cc,h} with
your tests in it.
On 2017/04/14 23:59:59, DaleCurtis wrote:
> Sorry if I wasn't clear, I meant you should not put these in
> PipelineIntegrationTest, they should live in some remoting specific test
> package. You should add a new remoting/remoting_integration_test.{cc,h} with
> your tests in it.
Thanks. You were clear. :)
As I mentioned in the message, I still put the tests in PipelineIntegrationTest
just to avoid duplicating the test cases for easier maintenance.
Do you think I'd better duplicate cases instead of the current approach?
+miu@
And also, if adding a new test class for media remoting as a subclass of the
PipelineIntegrationTestBase, I need to move some methods from
PipelineIntegrationTest to PipelineIntegrationTestBase, e.g.
StartPipelineWithMediaSource(), etc. Does that sgty?
On 2017/04/15 at 00:09:40, xjz wrote:
> On 2017/04/14 23:59:59, DaleCurtis wrote:
> > Sorry if I wasn't clear, I meant you should not put these in
> > PipelineIntegrationTest, they should live in some remoting specific test
> > package. You should add a new remoting/remoting_integration_test.{cc,h} with
> > your tests in it.
>
> Thanks. You were clear. :)
> As I mentioned in the message, I still put the tests in
PipelineIntegrationTest just to avoid duplicating the test cases for easier
maintenance.
> Do you think I'd better duplicate cases instead of the current approach?
> +miu@
I think you should duplicate the 3-4 tests you need; again the vast majority of
the tests in PIT are not necessary for you.
>
> And also, if adding a new test class for media remoting as a subclass of the
PipelineIntegrationTestBase, I need to move some methods from
PipelineIntegrationTest to PipelineIntegrationTestBase, e.g.
StartPipelineWithMediaSource(), etc. Does that sgty?
That sgtm.
On 2017/04/15 00:09:40, xjz wrote:
> On 2017/04/14 23:59:59, DaleCurtis wrote:
> > Sorry if I wasn't clear, I meant you should not put these in
> > PipelineIntegrationTest, they should live in some remoting specific test
> > package. You should add a new remoting/remoting_integration_test.{cc,h} with
> > your tests in it.
>
> Thanks. You were clear. :)
> As I mentioned in the message, I still put the tests in
PipelineIntegrationTest
> just to avoid duplicating the test cases for easier maintenance.
> Do you think I'd better duplicate cases instead of the current approach?
> +miu@
Dale: I think xjz@ was concerned about duplicating the test fixture code itself.
This would need to a large maintenance burden. For example, we don't want to
force everyone to maintain two copies of this:
1406 TEST_P(CommonPipelineIntegrationTest, BasicPlayback_MediaSource) {
Therefore, we really don't want a separate remoting_integration_test.{cc,h}.
Though, I could see a case for all of the code in
pipeline_integration_test.{cc,h} being refactored so that all three of normal,
remoting, and mojo pipelines share common core testing implementation. That
said, this would be a huge undertaking.
I think xjz@'s on the right track: Meaning, picking a small set of the existing
integration tests and adding a parameterized variant for running them through
the remoting pipeline, with all else the same.
Does this help clear things up? Or, do you still have alternative ideas in mind?
:)
On 2017/04/15 at 00:40:00, miu wrote:
> On 2017/04/15 00:09:40, xjz wrote:
> > On 2017/04/14 23:59:59, DaleCurtis wrote:
> > > Sorry if I wasn't clear, I meant you should not put these in
> > > PipelineIntegrationTest, they should live in some remoting specific test
> > > package. You should add a new remoting/remoting_integration_test.{cc,h}
with
> > > your tests in it.
> >
> > Thanks. You were clear. :)
> > As I mentioned in the message, I still put the tests in
PipelineIntegrationTest
> > just to avoid duplicating the test cases for easier maintenance.
> > Do you think I'd better duplicate cases instead of the current approach?
> > +miu@
>
> Dale: I think xjz@ was concerned about duplicating the test fixture code
itself. This would need to a large maintenance burden. For example, we don't
want to force everyone to maintain two copies of this:
>
> 1406 TEST_P(CommonPipelineIntegrationTest, BasicPlayback_MediaSource) {
Can you elaborate? I do indeed think you should have a file with copies of these
tests. These are mostly 3-4 lines each, and thus no big deal to copy. You
shouldn't need more than 3-4 tests for coverage at this layer. The benefit is
that we don't have every new test going into PIT try to figure out which
category they need to go in. That cognitive load and code complexity would now
be only in the place that needs it.
>
> Therefore, we really don't want a separate remoting_integration_test.{cc,h}.
Though, I could see a case for all of the code in
pipeline_integration_test.{cc,h} being refactored so that all three of normal,
remoting, and mojo pipelines share common core testing implementation. That
said, this would be a huge undertaking.
>
> I think xjz@'s on the right track: Meaning, picking a small set of the
existing integration tests and adding a parameterized variant for running them
through the remoting pipeline, with all else the same.
>
> Does this help clear things up? Or, do you still have alternative ideas in
mind? :)
On 2017/04/15 00:49:30, DaleCurtis wrote:
> On 2017/04/15 at 00:40:00, miu wrote:
> > On 2017/04/15 00:09:40, xjz wrote:
> This would need to a large maintenance burden. For example, we don't
> want to force everyone to maintain two copies of this:
> >
> > 1406 TEST_P(CommonPipelineIntegrationTest, BasicPlayback_MediaSource) {
>
> Can you elaborate? I do indeed think you should have a file with copies of
these
> tests. These are mostly 3-4 lines each, and thus no big deal to copy. You
Most of them are, but the one in my example was about 20 LOC...
> The benefit is
> that we don't have every new test going into PIT try to figure out which
> category they need to go in. That cognitive load and code complexity would now
> be only in the place that needs it.
Actually, we kinda *do* want developers to think about whether each new test
applies to each of the three media pipelines (normal, remoting, or mojo
service), right? ;) Meaning, if a new test is confirming some new interface
behavior that affects control behavior or data flow (i.e., not codec-specific
behavior), then we'd want a developer to think about adding coverage for
remoting and the mojo service also to ensure the interface change is compatible.
I want to be respectful about the code complexity and cognitive load here. I
think our goals should be to make sure changes that affect multiple pipelines
don't break anything, while changes that don't do not add a burden of
maintaining these tests. I'll leave it up to you and xjz@ how to best accomplish
this. :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/15 at 01:15:39, miu wrote:
> On 2017/04/15 00:49:30, DaleCurtis wrote:
> > On 2017/04/15 at 00:40:00, miu wrote:
> > > On 2017/04/15 00:09:40, xjz wrote:
> > This would need to a large maintenance burden. For example, we don't
> > want to force everyone to maintain two copies of this:
> > >
> > > 1406 TEST_P(CommonPipelineIntegrationTest, BasicPlayback_MediaSource) {
> >
> > Can you elaborate? I do indeed think you should have a file with copies of
these
> > tests. These are mostly 3-4 lines each, and thus no big deal to copy. You
>
> Most of them are, but the one in my example was about 20 LOC...
I think there's some confusion about what most of these tests are actually
testing. They're primarily "whole pipeline" tests, which means a lot of the
stuff in the interior is for testing the demuxers. Whereas what your project
should be looking to test is the renderer, so of that 20 LOC in there the only
part you need is:
MockMediaSource source("bear-320x240.webm", kWebM, 219229);
EXPECT_EQ(PIPELINE_OK, StartPipelineWithMediaSource(&source));
source.EndOfStream();
Play();
ASSERT_TRUE(WaitUntilOnEnded());
source.Shutdown();
Stop();
Of which we could definitely extract into a helper tested method to get that
down even further.
>
> > The benefit is
> > that we don't have every new test going into PIT try to figure out which
> > category they need to go in. That cognitive load and code complexity would
now
> > be only in the place that needs it.
>
> Actually, we kinda *do* want developers to think about whether each new test
applies to each of the three media pipelines (normal, remoting, or mojo
service), right? ;) Meaning, if a new test is confirming some new interface
behavior that affects control behavior or data flow (i.e., not codec-specific
behavior), then we'd want a developer to think about adding coverage for
remoting and the mojo service also to ensure the interface change is compatible.
As per above, these are primarily "whole pipeline" tests, not renderer tests.
What you want is just a couple basic playback tests and possibly some new
remoting renderer specific ones which test properties about the renderer. We
cheat sometimes and put renderer tests in here, but it's not the primary goal
nor does it feel like the right place for a whole suite of renderer tests to me.
>
> I want to be respectful about the code complexity and cognitive load here. I
think our goals should be to make sure changes that affect multiple pipelines
don't break anything, while changes that don't do not add a burden of
maintaining these tests. I'll leave it up to you and xjz@ how to best accomplish
this. :)
Happy to chat more about this, just trying to find the solution that's easiest
for everyone to maintain.
The CQ bit was checked by xjz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by xjz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Separate the media remoting integration tests from PipelineIntegrationTest. Moved some common codes from pipeline_integration_test.cc to pipeline_integration_test_base.*. No functional changes. PTAL. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Just looked at the media/ side of things. Thanks for moving things around! I think there's still a bunch more you don't need though. https://codereview.chromium.org/2808583002/diff/120001/media/remoting/integra... File media/remoting/integration_test.cc (right): https://codereview.chromium.org/2808583002/diff/120001/media/remoting/integra... media/remoting/integration_test.cc:29: CreateVideoDecodersCB prepend_video_decoders_cb = CreateVideoDecodersCB(), Style for unused arguments is to put the parameter name in /* .. */ and then in CreateRenderer() call below use the CreateXXXCB() mechanism. https://codereview.chromium.org/2808583002/diff/120001/media/remoting/integra... media/remoting/integration_test.cc:65: TEST_F(MediaRemotingIntegrationTest, BasicPlaybackHashed) { I think you can remove this or replace the BasicPlayback one with this. https://codereview.chromium.org/2808583002/diff/120001/media/remoting/integra... media/remoting/integration_test.cc:72: EXPECT_TRUE(demuxer_->GetTimelineOffset().is_null()); Delete; not relevant to your testing. https://codereview.chromium.org/2808583002/diff/120001/media/remoting/integra... media/remoting/integration_test.cc:82: EXPECT_TRUE(demuxer_->GetTimelineOffset().is_null()); Delete. https://codereview.chromium.org/2808583002/diff/120001/media/remoting/integra... media/remoting/integration_test.cc:107: TEST_F(MediaRemotingIntegrationTest, SeekWhilePaused) { Probably only need one of these seek tests since that's mostly a property of the pipeline and demuxers (renderers are just always paused then flushed). https://codereview.chromium.org/2808583002/diff/120001/media/remoting/integra... media/remoting/integration_test.cc:149: TEST_F(MediaRemotingIntegrationTest, SuspendWhilePaused) { Delete, not necessary here. https://codereview.chromium.org/2808583002/diff/120001/media/remoting/integra... media/remoting/integration_test.cc:174: TEST_F(MediaRemotingIntegrationTest, SuspendWhilePlaying) { Delete. https://codereview.chromium.org/2808583002/diff/120001/media/remoting/integra... media/remoting/integration_test.cc:195: // Verify audio decoder & renderer can handle aborted demuxer reads. Delete. https://codereview.chromium.org/2808583002/diff/120001/media/remoting/integra... media/remoting/integration_test.cc:203: // Verify video decoder & renderer can handle aborted demuxer reads. Delete. https://codereview.chromium.org/2808583002/diff/120001/media/remoting/integra... media/remoting/integration_test.cc:211: // Tests that we signal ended even when audio runs longer than video track. Probably delete. https://codereview.chromium.org/2808583002/diff/120001/media/test/pipeline_in... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2808583002/diff/120001/media/test/pipeline_in... media/test/pipeline_integration_test.cc:94: namespace { We don't generally use anonymous namespace in the media code; it doesn't really matter for unittests anyways. https://codereview.chromium.org/2808583002/diff/120001/media/test/pipeline_in... File media/test/pipeline_integration_test_base.cc (right): https://codereview.chromium.org/2808583002/diff/120001/media/test/pipeline_in... media/test/pipeline_integration_test_base.cc:89: namespace { Again drop. https://codereview.chromium.org/2808583002/diff/120001/media/test/pipeline_in... File media/test/pipeline_integration_test_base.h (right): https://codereview.chromium.org/2808583002/diff/120001/media/test/pipeline_in... media/test/pipeline_integration_test_base.h:64: // Helper class that emulates calls made on the ChunkDemuxer by the Should be extracted to a mock_media_source.{h,cc} file instead if we're going to move it. https://codereview.chromium.org/2808583002/diff/120001/media/test/pipeline_in... media/test/pipeline_integration_test_base.h:119: // Note: Tests using this class only exercise the DecryptingDemuxerStream path. Ditto. https://codereview.chromium.org/2808583002/diff/120001/media/test/pipeline_in... media/test/pipeline_integration_test_base.h:332: bool TestSeekDuringRead(const std::string& filename, This should be on the normal PIT since you can drop these tests from your side.
Patchset #7 (id:140001) has been deleted
The CQ bit was checked by xjz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
DaleCurtis: Thanks for the reviewing! Addressed your comments. PTAL again. Thanks! https://codereview.chromium.org/2808583002/diff/120001/media/remoting/integra... File media/remoting/integration_test.cc (right): https://codereview.chromium.org/2808583002/diff/120001/media/remoting/integra... media/remoting/integration_test.cc:29: CreateVideoDecodersCB prepend_video_decoders_cb = CreateVideoDecodersCB(), On 2017/04/20 18:24:50, DaleCurtis wrote: > Style for unused arguments is to put the parameter name in /* .. */ and then in > CreateRenderer() call below use the CreateXXXCB() mechanism. Done. https://codereview.chromium.org/2808583002/diff/120001/media/remoting/integra... media/remoting/integration_test.cc:65: TEST_F(MediaRemotingIntegrationTest, BasicPlaybackHashed) { On 2017/04/20 18:24:51, DaleCurtis wrote: > I think you can remove this or replace the BasicPlayback one with this. Done. https://codereview.chromium.org/2808583002/diff/120001/media/remoting/integra... media/remoting/integration_test.cc:72: EXPECT_TRUE(demuxer_->GetTimelineOffset().is_null()); On 2017/04/20 18:24:50, DaleCurtis wrote: > Delete; not relevant to your testing. Done. https://codereview.chromium.org/2808583002/diff/120001/media/remoting/integra... media/remoting/integration_test.cc:82: EXPECT_TRUE(demuxer_->GetTimelineOffset().is_null()); On 2017/04/20 18:24:51, DaleCurtis wrote: > Delete. Done. https://codereview.chromium.org/2808583002/diff/120001/media/remoting/integra... media/remoting/integration_test.cc:107: TEST_F(MediaRemotingIntegrationTest, SeekWhilePaused) { On 2017/04/20 18:24:50, DaleCurtis wrote: > Probably only need one of these seek tests since that's mostly a property of the > pipeline and demuxers (renderers are just always paused then flushed). Done. https://codereview.chromium.org/2808583002/diff/120001/media/remoting/integra... media/remoting/integration_test.cc:149: TEST_F(MediaRemotingIntegrationTest, SuspendWhilePaused) { On 2017/04/20 18:24:51, DaleCurtis wrote: > Delete, not necessary here. Done. https://codereview.chromium.org/2808583002/diff/120001/media/remoting/integra... media/remoting/integration_test.cc:174: TEST_F(MediaRemotingIntegrationTest, SuspendWhilePlaying) { On 2017/04/20 18:24:50, DaleCurtis wrote: > Delete. Done. https://codereview.chromium.org/2808583002/diff/120001/media/remoting/integra... media/remoting/integration_test.cc:195: // Verify audio decoder & renderer can handle aborted demuxer reads. On 2017/04/20 18:24:50, DaleCurtis wrote: > Delete. Done. https://codereview.chromium.org/2808583002/diff/120001/media/remoting/integra... media/remoting/integration_test.cc:203: // Verify video decoder & renderer can handle aborted demuxer reads. On 2017/04/20 18:24:50, DaleCurtis wrote: > Delete. Done. https://codereview.chromium.org/2808583002/diff/120001/media/remoting/integra... media/remoting/integration_test.cc:211: // Tests that we signal ended even when audio runs longer than video track. On 2017/04/20 18:24:50, DaleCurtis wrote: > Probably delete. Done. https://codereview.chromium.org/2808583002/diff/120001/media/test/pipeline_in... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2808583002/diff/120001/media/test/pipeline_in... media/test/pipeline_integration_test.cc:94: namespace { On 2017/04/20 18:24:51, DaleCurtis wrote: > We don't generally use anonymous namespace in the media code; it doesn't really > matter for unittests anyways. Done. https://codereview.chromium.org/2808583002/diff/120001/media/test/pipeline_in... File media/test/pipeline_integration_test_base.cc (right): https://codereview.chromium.org/2808583002/diff/120001/media/test/pipeline_in... media/test/pipeline_integration_test_base.cc:89: namespace { On 2017/04/20 18:24:51, DaleCurtis wrote: > Again drop. Done. https://codereview.chromium.org/2808583002/diff/120001/media/test/pipeline_in... File media/test/pipeline_integration_test_base.h (right): https://codereview.chromium.org/2808583002/diff/120001/media/test/pipeline_in... media/test/pipeline_integration_test_base.h:64: // Helper class that emulates calls made on the ChunkDemuxer by the On 2017/04/20 18:24:51, DaleCurtis wrote: > Should be extracted to a mock_media_source.{h,cc} file instead if we're going to > move it. Done. https://codereview.chromium.org/2808583002/diff/120001/media/test/pipeline_in... media/test/pipeline_integration_test_base.h:119: // Note: Tests using this class only exercise the DecryptingDemuxerStream path. On 2017/04/20 18:24:51, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/2808583002/diff/120001/media/test/pipeline_in... media/test/pipeline_integration_test_base.h:332: bool TestSeekDuringRead(const std::string& filename, On 2017/04/20 18:24:51, DaleCurtis wrote: > This should be on the normal PIT since you can drop these tests from your side. Done.
lgtm % a few nits. Thanks for bearing with me! https://codereview.chromium.org/2808583002/diff/160001/media/remoting/integra... File media/remoting/integration_test.cc (right): https://codereview.chromium.org/2808583002/diff/160001/media/remoting/integra... media/remoting/integration_test.cc:116: TEST_F(MediaRemotingIntegrationTest, BasicPlaybackPositiveStartTime) { Actually probably this one can go too. It's only verifying the demuxer expectations. https://codereview.chromium.org/2808583002/diff/160001/media/test/fake_encryp... File media/test/fake_encrypted_media.cc (right): https://codereview.chromium.org/2808583002/diff/160001/media/test/fake_encryp... media/test/fake_encrypted_media.cc:19: int FakeEncryptedMedia::TestCdmContext::GetCdmId() const { Line. https://codereview.chromium.org/2808583002/diff/160001/media/test/fake_encryp... File media/test/fake_encrypted_media.h (right): https://codereview.chromium.org/2808583002/diff/160001/media/test/fake_encryp... media/test/fake_encrypted_media.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. No (c). https://codereview.chromium.org/2808583002/diff/160001/media/test/mock_media_... File media/test/mock_media_source.cc (right): https://codereview.chromium.org/2808583002/diff/160001/media/test/mock_media_... media/test/mock_media_source.cc:43: void MockMediaSource::set_encrypted_media_init_data_cb( hacker_style() must be inline so keep this in the .h https://codereview.chromium.org/2808583002/diff/160001/media/test/mock_media_... media/test/mock_media_source.cc:48: void MockMediaSource::set_demuxer_failure_cb( Ditto. https://codereview.chromium.org/2808583002/diff/160001/media/test/mock_media_... File media/test/mock_media_source.h (right): https://codereview.chromium.org/2808583002/diff/160001/media/test/mock_media_... media/test/mock_media_source.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. Ditto. Probably all new files in this cl need this fix?
Description was changed from ========== RELAND: Media Remoting end to end integration tests. This is a re-land of https://codereview.chromium.org/2692593002/ with fix of compile warnings. -------Description of original change follows------- Media Remoting: End to end integration tests. Add end to end integration tests for Media Remoting. Refactors PipelineIntegrationTest to test both media and media remoting pipeline. Re-use current tests. No new tests are added in this CL. BUG=684065 ========== to ========== RELAND: Media Remoting end to end integration tests. This is a re-land of https://codereview.chromium.org/2692593002/. Moved tests for media remoting pipeline out of general PipelineIntegrationTest. -------Description of original change follows------- Media Remoting: End to end integration tests. Add end to end integration tests for Media Remoting. Refactors PipelineIntegrationTest to test both media and media remoting pipeline. Re-use current tests. No new tests are added in this CL. BUG=684065 ==========
Thanks Dale! Addressed the comments in PS# 8. https://codereview.chromium.org/2808583002/diff/160001/media/remoting/integra... File media/remoting/integration_test.cc (right): https://codereview.chromium.org/2808583002/diff/160001/media/remoting/integra... media/remoting/integration_test.cc:116: TEST_F(MediaRemotingIntegrationTest, BasicPlaybackPositiveStartTime) { On 2017/04/20 21:40:25, DaleCurtis wrote: > Actually probably this one can go too. It's only verifying the demuxer > expectations. Done. https://codereview.chromium.org/2808583002/diff/160001/media/test/fake_encryp... File media/test/fake_encrypted_media.cc (right): https://codereview.chromium.org/2808583002/diff/160001/media/test/fake_encryp... media/test/fake_encrypted_media.cc:19: int FakeEncryptedMedia::TestCdmContext::GetCdmId() const { On 2017/04/20 21:40:25, DaleCurtis wrote: > Line. Done. https://codereview.chromium.org/2808583002/diff/160001/media/test/fake_encryp... File media/test/fake_encrypted_media.h (right): https://codereview.chromium.org/2808583002/diff/160001/media/test/fake_encryp... media/test/fake_encrypted_media.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/04/20 21:40:25, DaleCurtis wrote: > No (c). Done. https://codereview.chromium.org/2808583002/diff/160001/media/test/mock_media_... File media/test/mock_media_source.cc (right): https://codereview.chromium.org/2808583002/diff/160001/media/test/mock_media_... media/test/mock_media_source.cc:43: void MockMediaSource::set_encrypted_media_init_data_cb( On 2017/04/20 21:40:25, DaleCurtis wrote: > hacker_style() must be inline so keep this in the .h Done. https://codereview.chromium.org/2808583002/diff/160001/media/test/mock_media_... media/test/mock_media_source.cc:48: void MockMediaSource::set_demuxer_failure_cb( On 2017/04/20 21:40:25, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/2808583002/diff/160001/media/test/mock_media_... File media/test/mock_media_source.h (right): https://codereview.chromium.org/2808583002/diff/160001/media/test/mock_media_... media/test/mock_media_source.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/04/20 21:40:25, DaleCurtis wrote: > Ditto. Probably all new files in this cl need this fix? Done. Thanks for catching this.
The CQ bit was checked by xjz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by xjz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xjz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2808583002/#ps200001 (title: "Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1492738251840480,
"parent_rev": "60e0ebaed452659c1274773c8e1869a379a656fa", "commit_rev":
"722e67b4d30548006c685f81bfcd6a88858168e1"}
Message was sent while issue was closed.
Description was changed from ========== RELAND: Media Remoting end to end integration tests. This is a re-land of https://codereview.chromium.org/2692593002/. Moved tests for media remoting pipeline out of general PipelineIntegrationTest. -------Description of original change follows------- Media Remoting: End to end integration tests. Add end to end integration tests for Media Remoting. Refactors PipelineIntegrationTest to test both media and media remoting pipeline. Re-use current tests. No new tests are added in this CL. BUG=684065 ========== to ========== RELAND: Media Remoting end to end integration tests. This is a re-land of https://codereview.chromium.org/2692593002/. Moved tests for media remoting pipeline out of general PipelineIntegrationTest. -------Description of original change follows------- Media Remoting: End to end integration tests. Add end to end integration tests for Media Remoting. Refactors PipelineIntegrationTest to test both media and media remoting pipeline. Re-use current tests. No new tests are added in this CL. BUG=684065 Review-Url: https://codereview.chromium.org/2808583002 Cr-Commit-Position: refs/heads/master@{#466216} Committed: https://chromium.googlesource.com/chromium/src/+/722e67b4d30548006c685f81bfcd... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:200001) as https://chromium.googlesource.com/chromium/src/+/722e67b4d30548006c685f81bfcd... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
