Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1504063003:
MediaRecorder tests for Pause functionality (Closed)
|
Created:
5 years ago by cpaulin (no longer in chrome) Modified:
5 years ago CC:
chromium-reviews, jam, tnakamura+watch_chromium.org, phoglund+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMediaRecorder tests for Pause functionality
BUG=chromium:548914
Committed: https://crrev.com/4be9c46795da0228e51c1ae8f6e5c35152774eb0
Cr-Commit-Position: refs/heads/master@{#365403}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Disabled one test and merged recorder start funcs #Patch Set 3 : Enable Android testing #Patch Set 4 : Check for empty blobs while testing start(slice) #
Total comments: 5
Patch Set 5 : Fixing non sense in testPauseAndNoDataAvailable #
Total comments: 12
Patch Set 6 : Disable android for now and address comments #
Messages
Total messages: 30 (9 generated)
Description was changed from ========== MediaRecorder tests for Pause functionality BUG=chromium:548914 ========== to ========== MediaRecorder tests for Pause functionality BUG=chromium:548914 ==========
cpaulin@chromium.org changed reviewers: + mcasas@chromium.org, phoglund@chromium.org, tommi@chromium.org
Review of content browser test for Media Recorder Pause
bots please. https://codereview.chromium.org/1504063003/diff/1/content/test/data/media/med... File content/test/data/media/mediarecorder_test.html (right): https://codereview.chromium.org/1504063003/diff/1/content/test/data/media/med... content/test/data/media/mediarecorder_test.html:32: function createAndStartWithTimeSliceMediaRecorder(stream, mimeType, slice) { Code duplication??? Please merge this function with the previous.
Bots fired up and addressed comment https://codereview.chromium.org/1504063003/diff/1/content/test/data/media/med... File content/test/data/media/mediarecorder_test.html (right): https://codereview.chromium.org/1504063003/diff/1/content/test/data/media/med... content/test/data/media/mediarecorder_test.html:32: function createAndStartWithTimeSliceMediaRecorder(stream, mimeType, slice) { On 2015/12/08 02:49:05, mcasas wrote: > Code duplication??? > Please merge this function with the previous. Miguel, Patrik explicitly wanted it separate, so there you go. At first I had it combined into one.
cpaulin@chromium.org changed reviewers: + avi@chromium.org - tommi@chromium.org
https://codereview.chromium.org/1504063003/diff/1/content/browser/media/webrt... File content/browser/media/webrtc_media_recorder_browsertest.cc (left): https://codereview.chromium.org/1504063003/diff/1/content/browser/media/webrt... content/browser/media/webrtc_media_recorder_browsertest.cc:72: #endif Hmm all this seems to be dragging from https://codereview.chromium.org/1502693002, I guess you should rebase and re-upload so these changes won't show up. You can do it at the same time you address the comments. https://codereview.chromium.org/1504063003/diff/1/content/browser/media/webrt... File content/browser/media/webrtc_media_recorder_browsertest.cc (right): https://codereview.chromium.org/1504063003/diff/1/content/browser/media/webrt... content/browser/media/webrtc_media_recorder_browsertest.cc:12: #if defined(OS_ANDROID) Can we enable this now? https://codereview.chromium.org/1504063003/diff/1/content/test/data/media/med... File content/test/data/media/mediarecorder_test.html (left): https://codereview.chromium.org/1504063003/diff/1/content/test/data/media/med... content/test/data/media/mediarecorder_test.html:168: }) Why is this part gone? https://codereview.chromium.org/1504063003/diff/1/content/test/data/media/med... File content/test/data/media/mediarecorder_test.html (right): https://codereview.chromium.org/1504063003/diff/1/content/test/data/media/med... content/test/data/media/mediarecorder_test.html:32: function createAndStartWithTimeSliceMediaRecorder(stream, mimeType, slice) { On 2015/12/08 18:29:11, cpaulin wrote: > On 2015/12/08 02:49:05, mcasas wrote: > > Code duplication??? > > Please merge this function with the previous. > > Miguel, > Patrik explicitly wanted it separate, so there you go. At first I had it > combined into one. The only really different line is 37 and you could just quickly check if the |slice| parameter is there by |slice !== 'undefined'| (note that |slice == 0| is not the same as this function being called with no |slice|. I think these 3 methods can be collapsed since they are essentially the same. What about createMediaRecorderAndStart(...) ? https://codereview.chromium.org/1504063003/diff/1/content/test/data/media/med... content/test/data/media/mediarecorder_test.html:382: function testPauseAndPauseEventTriggered() { What about merging testPauseAndPauseEventTriggered() with testPauseAndRecorderState()? I understand the lecture about each test should trigger one thing only, but in particular checking for a particular state and grabbing an Event are sequential and, with the appropriate checking, easily distinguishable in an error log. This also applies to e.g. testStartAndRecorderState() and testStartStopAndStopEventTriggered(). https://codereview.chromium.org/1504063003/diff/1/content/test/data/media/med... content/test/data/media/mediarecorder_test.html:457: reportTestSuccess(); In some tests there is a catch() ending in reportTestSuccess() and that's somehow surprising. WebKit has assert_throws() for this, to make it very explicit [1]. Why not copy the appropriate parts (i.e. until |if (code == null) ... return;|) in our test utilities js? (Also applies to l.286, at least). [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
Enabled Android and addressed comments https://codereview.chromium.org/1504063003/diff/1/content/browser/media/webrt... File content/browser/media/webrtc_media_recorder_browsertest.cc (left): https://codereview.chromium.org/1504063003/diff/1/content/browser/media/webrt... content/browser/media/webrtc_media_recorder_browsertest.cc:72: #endif On 2015/12/11 00:08:56, mcasas wrote: > Hmm all this seems to be dragging from > https://codereview.chromium.org/1502693002, > I guess you should rebase and re-upload so > these changes won't show up. You can do it > at the same time you address the comments. Yes, I took it out https://codereview.chromium.org/1504063003/diff/1/content/browser/media/webrt... File content/browser/media/webrtc_media_recorder_browsertest.cc (right): https://codereview.chromium.org/1504063003/diff/1/content/browser/media/webrt... content/browser/media/webrtc_media_recorder_browsertest.cc:12: #if defined(OS_ANDROID) On 2015/12/11 00:08:56, mcasas wrote: > Can we enable this now? I wanted to do it in a separate CL so that if it fails it does not impact this CL, but sure I can enable it https://codereview.chromium.org/1504063003/diff/1/content/test/data/media/med... File content/test/data/media/mediarecorder_test.html (left): https://codereview.chromium.org/1504063003/diff/1/content/test/data/media/med... content/test/data/media/mediarecorder_test.html:168: }) On 2015/12/11 00:08:57, mcasas wrote: > Why is this part gone? It is a bit flaky, sometimes the time stamps are a little bit late and then it fails. It was a nice to have thing, I am not sure it is worth the added flakiness. https://codereview.chromium.org/1504063003/diff/1/content/test/data/media/med... File content/test/data/media/mediarecorder_test.html (right): https://codereview.chromium.org/1504063003/diff/1/content/test/data/media/med... content/test/data/media/mediarecorder_test.html:382: function testPauseAndPauseEventTriggered() { On 2015/12/11 00:08:57, mcasas wrote: > What about merging testPauseAndPauseEventTriggered() > with testPauseAndRecorderState()? > I understand the lecture about each test should > trigger one thing only, but in particular > checking for a particular state and grabbing an > Event are sequential and, with the appropriate > checking, easily distinguishable in an error log. > > This also applies to e.g. testStartAndRecorderState() > and testStartStopAndStopEventTriggered(). You are preaching to the convert... I am not sure I will go for it since it might mean me changing this 2 more times ;-( https://codereview.chromium.org/1504063003/diff/1/content/test/data/media/med... content/test/data/media/mediarecorder_test.html:457: reportTestSuccess(); On 2015/12/11 00:08:56, mcasas wrote: > In some tests there is a catch() ending in > reportTestSuccess() and that's somehow > surprising. WebKit has assert_throws() for > this, to make it very explicit [1]. Why > not copy the appropriate parts (i.e. until > |if (code == null) ... return;|) in our > test utilities js? (Also applies to l.286, > at least). > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Because catch is a nice feature of promise that really works well here. And this is a negative test: we want it to fail.
mcasas@ PTAL
Working merge with https://codereview.chromium.org/1515323002/
Patch set 4 is out, mcasas@ PTAL
https://codereview.chromium.org/1504063003/diff/60001/content/test/data/media... File content/test/data/media/mediarecorder_test.html (right): https://codereview.chromium.org/1504063003/diff/60001/content/test/data/media... content/test/data/media/mediarecorder_test.html:405: navigator.mediaDevices.getUserMedia(DEFAULT_CONSTRAINTS) Does this test actually test anything? It looks like you're trying to prove that ondataavailable is never called, by waiting for it to be NOT called. You could try deliberately disabling pause in the production code, but I suspect this test would either pass anyway or be flaky. I'd question if this test has any real value. If you need to write it, you need to sleep a second or two after the pause so if the recorder is actually is delivering data, that it has time to arrive. https://codereview.chromium.org/1504063003/diff/60001/content/test/data/media... content/test/data/media/mediarecorder_test.html:416: if (event.data.size > 0) { I think this can be simplified to theRecorder.ondataavailable = function(event) { failTest('Received unexpected data after pause!'); } You could also throw an exception I guess, but I'm not sure if an exception in ondataavailable propagates to the catch(). failTest should make the c++ code fail immediately in any case.
Patch Set 5 ready for review. Addressed comments https://codereview.chromium.org/1504063003/diff/60001/content/test/data/media... File content/test/data/media/mediarecorder_test.html (right): https://codereview.chromium.org/1504063003/diff/60001/content/test/data/media... content/test/data/media/mediarecorder_test.html:405: navigator.mediaDevices.getUserMedia(DEFAULT_CONSTRAINTS) On 2015/12/14 12:54:13, phoglund_chrome wrote: > Does this test actually test anything? It looks like you're trying to prove that > ondataavailable is never called, by waiting for it to be NOT called. You could > try deliberately disabling pause in the production code, but I suspect this test > would either pass anyway or be flaky. > > I'd question if this test has any real value. If you need to write it, you need > to sleep a second or two after the pause so if the recorder is actually is > delivering data, that it has time to arrive. Sometimes a bit of waiting does not hurt. New patch on its way. https://codereview.chromium.org/1504063003/diff/60001/content/test/data/media... content/test/data/media/mediarecorder_test.html:416: if (event.data.size > 0) { On 2015/12/14 12:54:13, phoglund_chrome wrote: > I think this can be simplified to > > theRecorder.ondataavailable = function(event) { > failTest('Received unexpected data after pause!'); > } > > You could also throw an exception I guess, but I'm not sure if an exception in > ondataavailable propagates to the catch(). failTest should make the c++ code > fail immediately in any case. Good idea
Sometimes, if you're unsure if a test actually will catch bugs, try deliberately breaking it in the production code (e.g. make pause do nothing), and run the test and make sure it breaks. https://codereview.chromium.org/1504063003/diff/60001/content/test/data/media... File content/test/data/media/mediarecorder_test.html (right): https://codereview.chromium.org/1504063003/diff/60001/content/test/data/media... content/test/data/media/mediarecorder_test.html:405: navigator.mediaDevices.getUserMedia(DEFAULT_CONSTRAINTS) On 2015/12/14 18:56:50, cpaulin wrote: > On 2015/12/14 12:54:13, phoglund_chrome wrote: > > Does this test actually test anything? It looks like you're trying to prove > that > > ondataavailable is never called, by waiting for it to be NOT called. You could > > try deliberately disabling pause in the production code, but I suspect this > test > > would either pass anyway or be flaky. > > > > I'd question if this test has any real value. If you need to write it, you > need > > to sleep a second or two after the pause so if the recorder is actually is > > delivering data, that it has time to arrive. > > Sometimes a bit of waiting does not hurt. New patch on its way. Yes, I may have said earlier that "never sleep in tests", but an exception is when we're trying to prove that something doesn't happen :)
lgtm
The CQ bit was checked by cpaulin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1504063003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1504063003/80001
The CQ bit was unchecked by mcasas@chromium.org
On 2015/12/14 19:26:23, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1504063003/80001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1504063003/80001 Wait for reviewers' LGTM (mine, in this case)
On 2015/12/14 19:40:15, mcasas wrote: > On 2015/12/14 19:26:23, commit-bot: I haz the power wrote: > > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/patch-status/1504063003/80001 > > View timeline at > > https://chromium-cq-status.appspot.com/patch-timeline/1504063003/80001 > > Wait for reviewers' LGTM (mine, in this case) That's fine, please make clear what you would like to see to obtain the coveted LGTM ;-)
On 2015/12/14 20:51:32, cpaulin wrote: > On 2015/12/14 19:40:15, mcasas wrote: > > On 2015/12/14 19:26:23, commit-bot: I haz the power wrote: > > > CQ is trying da patch. Follow status at > > > https://chromium-cq-status.appspot.com/patch-status/1504063003/80001 > > > View timeline at > > > https://chromium-cq-status.appspot.com/patch-timeline/1504063003/80001 > > > > Wait for reviewers' LGTM (mine, in this case) > > That's fine, please make clear what you would like to see to obtain the coveted > LGTM ;-) mcasas@ PTAL
LGTM nothing is really blocking this CL right now and we can iterate in an immediate follow-up CL. https://codereview.chromium.org/1504063003/diff/80001/content/browser/media/w... File content/browser/media/webrtc_media_recorder_browsertest.cc (left): https://codereview.chromium.org/1504063003/diff/80001/content/browser/media/w... content/browser/media/webrtc_media_recorder_browsertest.cc:13: // TODO(cpaulin): when crbug.com/561068 is fixed, enable this test I didn't mean to enable these tests for Android in this CL. What if we make an Android bot unhappy after landing? You won't know if it's the new tests of the bulk-enable, and this CL would get reverted and then you'll have to split it anyhow. Best to have a single tiny CL to enable for Android, and land the added tests in another CL. CLs are cheap! Keep it in mind for the next time. https://codereview.chromium.org/1504063003/diff/80001/content/test/data/media... File content/test/data/media/mediarecorder_test.html (right): https://codereview.chromium.org/1504063003/diff/80001/content/test/data/media... content/test/data/media/mediarecorder_test.html:27: if (slice != undefined) { !== 'undefined' ? http://stackoverflow.com/questions/411352/how-best-to-determine-if-an-argumen... https://codereview.chromium.org/1504063003/diff/80001/content/test/data/media... content/test/data/media/mediarecorder_test.html:332: DEFAULT_RECORDER_MIME_TYPE); nit: fits in one line, here and elsewhere. https://codereview.chromium.org/1504063003/diff/80001/content/test/data/media... content/test/data/media/mediarecorder_test.html:385: recorder.pause(); I would just bundle here the checking assertEquals('paused', theRecorder.state); and remove the specific test for it, since anyway changing the state should be synchronous. Similar comments about stop() -> stopped state, start() -> started state etc. It's fine to do it in a follow up CL (but don't forget it, e.g. make a tiny bug for yourself to consolidate tests). https://codereview.chromium.org/1504063003/diff/80001/content/test/data/media... content/test/data/media/mediarecorder_test.html:430: function testNoPauseWhileRecorderInactive() { It'd be great to have a one liner description per test, e.g. // Test that MediaRecorder's pause() throws and exception if |state| is not |started|. Again, you can add these micro descriptions in a follow up CL, since I understand you are still planning to work on this file. https://codereview.chromium.org/1504063003/diff/80001/content/test/data/media... content/test/data/media/mediarecorder_test.html:436: recorder.pause(); This test is a great candidate for // Verify that an Exception is thrown when calling pause() on a not started MR assert_throws(recorder.pause());
The CQ bit was checked by cpaulin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from phoglund@chromium.org, mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/1504063003/#ps100001 (title: "Disable android for now and address comments")
New patch set addressing some of the comments, the rest is left for crbug/570074. Android is disabled for now, will be enabled in another CL soon. https://codereview.chromium.org/1504063003/diff/80001/content/browser/media/w... File content/browser/media/webrtc_media_recorder_browsertest.cc (left): https://codereview.chromium.org/1504063003/diff/80001/content/browser/media/w... content/browser/media/webrtc_media_recorder_browsertest.cc:13: // TODO(cpaulin): when crbug.com/561068 is fixed, enable this test On 2015/12/14 23:48:49, mcasas wrote: > I didn't mean to enable these tests for Android in this CL. > What if we make an Android bot unhappy after landing? > You won't know if it's the new tests of the bulk-enable, > and this CL would get reverted and then you'll have to > split it anyhow. Best to have a single tiny CL to enable for > Android, and land the added tests in another CL. CLs are > cheap! Keep it in mind for the next time. Acknowledged. https://codereview.chromium.org/1504063003/diff/80001/content/test/data/media... File content/test/data/media/mediarecorder_test.html (right): https://codereview.chromium.org/1504063003/diff/80001/content/test/data/media... content/test/data/media/mediarecorder_test.html:27: if (slice != undefined) { On 2015/12/14 23:48:49, mcasas wrote: > !== 'undefined' ? > > http://stackoverflow.com/questions/411352/how-best-to-determine-if-an-argumen... |slice != undefined| is correct, more explicit would be typeof(slice) == 'undefined' as per that very stackoverflow link. https://codereview.chromium.org/1504063003/diff/80001/content/test/data/media... content/test/data/media/mediarecorder_test.html:332: DEFAULT_RECORDER_MIME_TYPE); On 2015/12/14 23:48:50, mcasas wrote: > nit: fits in one line, here and elsewhere. Acknowledged. https://codereview.chromium.org/1504063003/diff/80001/content/test/data/media... content/test/data/media/mediarecorder_test.html:385: recorder.pause(); On 2015/12/14 23:48:49, mcasas wrote: > I would just bundle here the checking > assertEquals('paused', theRecorder.state); > and remove the specific test for it, since anyway > changing the state should be synchronous. > Similar comments about stop() -> stopped > state, start() -> started state etc. It's fine to > do it in a follow up CL (but don't forget it, e.g. > make a tiny bug for yourself to consolidate tests). Acknowledged. https://codereview.chromium.org/1504063003/diff/80001/content/test/data/media... content/test/data/media/mediarecorder_test.html:430: function testNoPauseWhileRecorderInactive() { On 2015/12/14 23:48:50, mcasas wrote: > It'd be great to have a one liner description per test, e.g. > // Test that MediaRecorder's pause() throws and exception if |state| is not > |started|. > Again, you can add these micro descriptions in a follow up > CL, since I understand you are still planning to work on > this file. I will address it in: crbug.com/570074 https://codereview.chromium.org/1504063003/diff/80001/content/test/data/media... content/test/data/media/mediarecorder_test.html:436: recorder.pause(); On 2015/12/14 23:48:49, mcasas wrote: > This test is a great candidate for > > // Verify that an Exception is thrown when calling pause() on a not started MR > assert_throws(recorder.pause()); I will address this in crbug/570074
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1504063003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1504063003/100001
Message was sent while issue was closed.
Description was changed from ========== MediaRecorder tests for Pause functionality BUG=chromium:548914 ========== to ========== MediaRecorder tests for Pause functionality BUG=chromium:548914 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== MediaRecorder tests for Pause functionality BUG=chromium:548914 ========== to ========== MediaRecorder tests for Pause functionality BUG=chromium:548914 Committed: https://crrev.com/4be9c46795da0228e51c1ae8f6e5c35152774eb0 Cr-Commit-Position: refs/heads/master@{#365403} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/4be9c46795da0228e51c1ae8f6e5c35152774eb0 Cr-Commit-Position: refs/heads/master@{#365403} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
