Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1514633006:
Add HTMLVideoElementTest (Closed)
|
Created:
5 years ago by sandersd (OOO until July 31) Modified:
4 years, 11 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, mlamouri+watch-blink_chromium.org, philipj_slow Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd HTMLVideoElementTest
Committed: https://crrev.com/e714c6542eb7b4ec7ac91a5d52cc66d27b9d51a0
Cr-Commit-Position: refs/heads/master@{#368465}
Patch Set 1 #Patch Set 2 : #
Total comments: 7
Patch Set 3 : Rebase #Patch Set 4 : Fixes #
Messages
Total messages: 23 (10 generated)
Description was changed from ========== HTMLVideoElementTest ========== to ========== Add HTMLVideoElementTest ==========
sandersd@chromium.org changed reviewers: + philipj@opera.com
sandersd@chromium.org changed required reviewers: + philipj@opera.com
https://codereview.chromium.org/1514633006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp (right): https://codereview.chromium.org/1514633006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp:23: class EmptyWebMediaPlayer : public WebMediaPlayer { It seems like this should be in a separate file, but I don't see empty implementations of public classes. Is there a convention for this? https://codereview.chromium.org/1514633006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp:75: class HTMLVideoElementTest : public ::testing::Test { The test cases here are better on HTMLMediaElement, but there is (identical) logic in HTMLAudioElement::create and HTMLVideoElement::create that would need to be copied again.
lgtm https://codereview.chromium.org/1514633006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp (right): https://codereview.chromium.org/1514633006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp:23: class EmptyWebMediaPlayer : public WebMediaPlayer { On 2015/12/17 19:05:56, sandersd wrote: > It seems like this should be in a separate file, but I don't see empty > implementations of public classes. Is there a convention for this? Not that I'm aware of, until now we haven't had any use for an empty implementation of this. Perhaps someone with more experience writing unit tests inside Blink would know, maybe mlamouri@ https://codereview.chromium.org/1514633006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp:75: class HTMLVideoElementTest : public ::testing::Test { On 2015/12/17 19:05:56, sandersd wrote: > The test cases here are better on HTMLMediaElement, but there is (identical) > logic in HTMLAudioElement::create and HTMLVideoElement::create that would need > to be copied again. Just HTMLVideoElementTest seems fine, if unit tests for HTMLAudioElement are later also added I'm sure it can be refactored somehow.
sandersd@chromium.org changed reviewers: + mlamouri@chromium.org
mlamouri@: Where should EmptyWebMediaPlayer go?
https://codereview.chromium.org/1514633006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp (right): https://codereview.chromium.org/1514633006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp:23: class EmptyWebMediaPlayer : public WebMediaPlayer { On 2015/12/18 at 15:02:27, philipj wrote: > On 2015/12/17 19:05:56, sandersd wrote: > > It seems like this should be in a separate file, but I don't see empty > > implementations of public classes. Is there a convention for this? > > Not that I'm aware of, until now we haven't had any use for an empty implementation of this. Perhaps someone with more experience writing unit tests inside Blink would know, maybe mlamouri@ What about doing something similar to: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... You could basically add a MockWebMediaPlayer that will mock all the methods. WDYT?
On 2016/01/05 15:00:07, Mounir Lamouri wrote: > https://codereview.chromium.org/1514633006/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp (right): > > https://codereview.chromium.org/1514633006/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp:23: class > EmptyWebMediaPlayer : public WebMediaPlayer { > On 2015/12/18 at 15:02:27, philipj wrote: > > On 2015/12/17 19:05:56, sandersd wrote: > > > It seems like this should be in a separate file, but I don't see empty > > > implementations of public classes. Is there a convention for this? > > > > Not that I'm aware of, until now we haven't had any use for an empty > implementation of this. Perhaps someone with more experience writing unit tests > inside Blink would know, maybe mlamouri@ > > What about doing something similar to: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > You could basically add a MockWebMediaPlayer that will mock all the methods. > WDYT? I specifically implemented a empty version (similar to the core/loader/EmptyClients.h classes) so that I would only have to mock methods relevant to each test. I oppose strongly having to program a large mock, as it results in brittle tests. (In some cases a Nice mock can work, but that's not obviously true in this case.) I suppose the relevant question though is, if there was a MockWebMediaPlayer, where would the code be?
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
https://codereview.chromium.org/1514633006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp (right): https://codereview.chromium.org/1514633006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp:97: RefPtrWillBeMember<HTMLVideoElement> m_video; RefPtrWillBePersistent<>
On 2016/01/05 at 18:45:53, sandersd wrote: > On 2016/01/05 15:00:07, Mounir Lamouri wrote: > > https://codereview.chromium.org/1514633006/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp (right): > > > > https://codereview.chromium.org/1514633006/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp:23: class > > EmptyWebMediaPlayer : public WebMediaPlayer { > > On 2015/12/18 at 15:02:27, philipj wrote: > > > On 2015/12/17 19:05:56, sandersd wrote: > > > > It seems like this should be in a separate file, but I don't see empty > > > > implementations of public classes. Is there a convention for this? > > > > > > Not that I'm aware of, until now we haven't had any use for an empty > > implementation of this. Perhaps someone with more experience writing unit tests > > inside Blink would know, maybe mlamouri@ > > > > What about doing something similar to: > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > You could basically add a MockWebMediaPlayer that will mock all the methods. > > WDYT? > > I specifically implemented a empty version (similar to the core/loader/EmptyClients.h classes) so that I would only have to mock methods relevant to each test. I oppose strongly having to program a large mock, as it results in brittle tests. (In some cases a Nice mock can work, but that's not obviously true in this case.) > > I suppose the relevant question though is, if there was a MockWebMediaPlayer, where would the code be? I guess you can create an {Empty,Mock}WebMediaPlayer and have it in the same dir but put it under the core_unittest_files target.
> I guess you can create an {Empty,Mock}WebMediaPlayer and have it in the same
dir
> but put it under the core_unittest_files target.
Given that option, I'll prefer to leave it where it is until such a time as
another test also needs it.
Thanks for your help!
https://codereview.chromium.org/1514633006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp (right):
https://codereview.chromium.org/1514633006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp:97:
RefPtrWillBeMember<HTMLVideoElement> m_video;
On 2016/01/05 18:51:48, sof wrote:
> RefPtrWillBePersistent<>
Done.
The CQ bit was checked by sandersd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1514633006/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1514633006/60001
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 sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com Link to the patchset: https://codereview.chromium.org/1514633006/#ps60001 (title: "Fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1514633006/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1514633006/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add HTMLVideoElementTest ========== to ========== Add HTMLVideoElementTest Committed: https://crrev.com/e714c6542eb7b4ec7ac91a5d52cc66d27b9d51a0 Cr-Commit-Position: refs/heads/master@{#368465} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e714c6542eb7b4ec7ac91a5d52cc66d27b9d51a0 Cr-Commit-Position: refs/heads/master@{#368465} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
