Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1204943002:
Tests for Media Router media-router-container Polymer element. (Closed)
DescriptionTests for Media Router media-router-container Polymer element.
BUG=501146
Committed: https://crrev.com/98a5676d4bfc93e9e2d92f3ee386205904cdf163
Cr-Commit-Position: refs/heads/master@{#338430}
Patch Set 1 : #
Total comments: 25
Patch Set 2 : Changes per michaelpg@ and dbeam@'s comments. #
Total comments: 4
Patch Set 3 : Changes per kmarshall@'s comments. Rebase. #
Total comments: 25
Patch Set 4 : Rebase #Patch Set 5 : Changes per dbeam@'s comments. #
Total comments: 12
Patch Set 6 : Changes per dbeam@'s comments. #Patch Set 7 : Rebase #Patch Set 8 : Changes per dbeam@'s comments. #
Messages
Total messages: 47 (29 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
Patchset #1 (id:180001) has been deleted
Patchset #1 (id:200001) has been deleted
Patchset #1 (id:220001) has been deleted
Patchset #1 (id:240001) has been deleted
Patchset #1 (id:260001) has been deleted
Patchset #2 (id:300001) has been deleted
Patchset #1 (id:280001) has been deleted
Patchset #1 (id:320001) has been deleted
Patchset #1 (id:340001) has been deleted
Patchset #1 (id:340001) has been deleted
Patchset #1 (id:360001) has been deleted
Patchset #1 (id:380001) has been deleted
apacible@chromium.org changed reviewers: + dbeam@chromium.org, kmarshall@chromium.org, michaelpg@chromium.org
PTAL, thanks! dbeam@ for chrome/test/data/webui/. kmarshall@ for chrome/browser/resources/media_router/. michaelpg@ for everything.
Dan, is this kosher in closure? https://codereview.chromium.org/1204943002/diff/60002/chrome/test/data/webui/... Also, curious to hear your thoughts on the enum situation. https://codereview.chromium.org/1204943002/diff/60002/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1204943002/diff/60002/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:33: value: function() { add readOnly: true and then value: may as well be set to the object literal instead of using the wrapping function https://codereview.chromium.org/1204943002/diff/60002/chrome/test/data/webui/... File chrome/test/data/webui/media_router/media_router_container_tests.js (right): https://codereview.chromium.org/1204943002/diff/60002/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_tests.js:25: var fakeCastModeList; dbeam: technically this starts out as undefined. does closure care? should this be {!Array<..>|undefined} (I hope not) https://codereview.chromium.org/1204943002/diff/60002/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_tests.js:48: container.computeArrowDropIcon_(view)); You should also test the result of computeArrowDropIcon_ itself somewhere, since this is only checking that the data binding is working. https://codereview.chromium.org/1204943002/diff/60002/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_tests.js:55: // Checks whether element |elementId| is hidden. |expected| is the nit: linebreak above https://codereview.chromium.org/1204943002/diff/60002/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_tests.js:56: // expected visibility. |expected| is true if the item should be hidden, so it's the expected hidden-ness. Rename to |hidden|? https://codereview.chromium.org/1204943002/diff/60002/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_tests.js:94: new media_router.CastMode(1, 'Cast Mode 1', 'Description 1'), nit: 2-space indent inside array/object literals https://codereview.chromium.org/1204943002/diff/60002/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_tests.js:128: container.addEventListener('close-button-click', function() { We should add a PolymerTest.awaitEvent[s] method to simplify this: PolymerTest.awaitEvent(container, 'close-button-click', done); Not a requisite for this CL, just a thought. https://codereview.chromium.org/1204943002/diff/60002/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_tests.js:142: var sinkArray = Array.prototype.slice.call(sinkList); why slice this if you're not going to use Array methods? NodeList supports [indexing] (like all JS objects -- take a look Object.getOwnPropertyNames(foo) for all kinds of variables.) https://codereview.chromium.org/1204943002/diff/60002/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_tests.js:147: data.detail.selectedCastModeValue); This is asynchronous, so include a `done` callback in this test and call it here -- otherwise the test will pass immediately, and nothing happens if the listener is never triggered. https://codereview.chromium.org/1204943002/diff/60002/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_tests.js:161: setTimeout(function() { Same -- call done() at the end of this function, otherwise Mocha registers the test as a pass before this function is executed. https://codereview.chromium.org/1204943002/diff/60002/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_tests.js:242: checkElementText(fakeCastModeList[i].description, castModeArray[i]); can you combine this with the previous test? https://codereview.chromium.org/1204943002/diff/60002/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_tests.js:292: checkElementHiddenWithId(false, 'container-header'); A thought: could you make a checkElementsHidden function, which takes an array of elements, tests that those elements are hidden, and tests that the elements you care about that are *not* in that array are *not* hidden? It would clean up these walls of checkElementHiddenWithId and make the tests easier to read, e.g.: checkElementsHidden(['sink-list-header-text', 'issue-banner', 'route-details', 'sink-list']); IDK, maybe that's getting too fancy.
https://codereview.chromium.org/1204943002/diff/60002/chrome/test/data/webui/... File chrome/test/data/webui/media_router/media_router_container_tests.js (right): https://codereview.chromium.org/1204943002/diff/60002/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_tests.js:25: var fakeCastModeList; On 2015/07/06 20:55:46, michaelpg wrote: > dbeam: technically this starts out as undefined. does closure care? should this > be {!Array<..>|undefined} (I hope not) it'd be arguably better to start as var fakeCastModeList = []; but closure doesn't care, no :(
Patchset #2 (id:410001) has been deleted
Patchset #2 (id:430001) has been deleted
https://codereview.chromium.org/1204943002/diff/60002/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1204943002/diff/60002/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:33: value: function() { On 2015/07/06 20:55:46, michaelpg wrote: > add readOnly: true > > and then value: may as well be set to the object literal instead of using the > wrapping function Done. https://codereview.chromium.org/1204943002/diff/60002/chrome/test/data/webui/... File chrome/test/data/webui/media_router/media_router_container_tests.js (right): https://codereview.chromium.org/1204943002/diff/60002/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_tests.js:25: var fakeCastModeList; On 2015/07/06 21:08:06, Dan Beam wrote: > On 2015/07/06 20:55:46, michaelpg wrote: > > dbeam: technically this starts out as undefined. does closure care? should > this > > be {!Array<..>|undefined} (I hope not) > > it'd be arguably better to start as > > var fakeCastModeList = []; > > but closure doesn't care, no :( Updated to start as var fakeCastModeList = []; here and below. https://codereview.chromium.org/1204943002/diff/60002/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_tests.js:48: container.computeArrowDropIcon_(view)); On 2015/07/06 20:55:46, michaelpg wrote: > You should also test the result of computeArrowDropIcon_ itself somewhere, since > this is only checking that the data binding is working. Done. https://codereview.chromium.org/1204943002/diff/60002/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_tests.js:55: // Checks whether element |elementId| is hidden. |expected| is the On 2015/07/06 20:55:46, michaelpg wrote: > nit: linebreak above Done. https://codereview.chromium.org/1204943002/diff/60002/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_tests.js:56: // expected visibility. On 2015/07/06 20:55:46, michaelpg wrote: > |expected| is true if the item should be hidden, so it's the expected > hidden-ness. Rename to |hidden|? Done. https://codereview.chromium.org/1204943002/diff/60002/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_tests.js:94: new media_router.CastMode(1, 'Cast Mode 1', 'Description 1'), On 2015/07/06 20:55:46, michaelpg wrote: > nit: 2-space indent inside array/object literals Done. https://codereview.chromium.org/1204943002/diff/60002/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_tests.js:128: container.addEventListener('close-button-click', function() { On 2015/07/06 20:55:46, michaelpg wrote: > We should add a PolymerTest.awaitEvent[s] method to simplify this: > > PolymerTest.awaitEvent(container, 'close-button-click', done); > > Not a requisite for this CL, just a thought. Acknowledged. https://codereview.chromium.org/1204943002/diff/60002/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_tests.js:142: var sinkArray = Array.prototype.slice.call(sinkList); On 2015/07/06 20:55:46, michaelpg wrote: > why slice this if you're not going to use Array methods? > > NodeList supports [indexing] (like all JS objects -- take a look > Object.getOwnPropertyNames(foo) for all kinds of variables.) Fixed. https://codereview.chromium.org/1204943002/diff/60002/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_tests.js:147: data.detail.selectedCastModeValue); On 2015/07/06 20:55:46, michaelpg wrote: > This is asynchronous, so include a `done` callback in this test and call it here > -- otherwise the test will pass immediately, and nothing happens if the listener > is never triggered. Done. https://codereview.chromium.org/1204943002/diff/60002/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_tests.js:161: setTimeout(function() { On 2015/07/06 20:55:46, michaelpg wrote: > Same -- call done() at the end of this function, otherwise Mocha registers the > test as a pass before this function is executed. Done. https://codereview.chromium.org/1204943002/diff/60002/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_tests.js:242: checkElementText(fakeCastModeList[i].description, castModeArray[i]); On 2015/07/06 20:55:46, michaelpg wrote: > can you combine this with the previous test? Done. https://codereview.chromium.org/1204943002/diff/60002/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_tests.js:292: checkElementHiddenWithId(false, 'container-header'); On 2015/07/06 20:55:46, michaelpg wrote: > A thought: could you make a checkElementsHidden function, which takes an array > of elements, tests that those elements are hidden, and tests that the elements > you care about that are *not* in that array are *not* hidden? > > It would clean up these walls of checkElementHiddenWithId and make the tests > easier to read, e.g.: > > checkElementsHidden(['sink-list-header-text', > 'issue-banner', > 'route-details', > 'sink-list']); > > IDK, maybe that's getting too fancy. Done.
https://codereview.chromium.org/1204943002/diff/450001/chrome/test/data/webui... File chrome/test/data/webui/media_router/media_router_container_tests.js (right): https://codereview.chromium.org/1204943002/diff/450001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:62: assertEquals(container.$['arrow-drop-icon'].icon, Expected values precede actual values (important to get right for test logging), ditto elsewhere in the code. https://codereview.chromium.org/1204943002/diff/450001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:329: checkElementsHiddenWithId(['issue-banner', Might it be easier to express visibility checks in terms of what is shown, rather than what is hidden?
https://codereview.chromium.org/1204943002/diff/450001/chrome/test/data/webui... File chrome/test/data/webui/media_router/media_router_container_tests.js (right): https://codereview.chromium.org/1204943002/diff/450001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:62: assertEquals(container.$['arrow-drop-icon'].icon, On 2015/07/08 23:02:21, Kevin M wrote: > Expected values precede actual values (important to get right for test logging), > ditto elsewhere in the code. Done. https://codereview.chromium.org/1204943002/diff/450001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:329: checkElementsHiddenWithId(['issue-banner', On 2015/07/08 23:02:21, Kevin M wrote: > Might it be easier to express visibility checks in terms of what is shown, > rather than what is hidden? Makes sense, done.
lgtm https://codereview.chromium.org/1204943002/diff/470001/chrome/test/data/webui... File chrome/test/data/webui/media_router/media_router_container_tests.js (right): https://codereview.chromium.org/1204943002/diff/470001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:330: 'cast-mode-list', Fix indents here and elsewhere w/multi line array literals
lgtm https://codereview.chromium.org/1204943002/diff/470001/chrome/test/data/webui... File chrome/test/data/webui/media_router/media_router_container_tests.js (right): https://codereview.chromium.org/1204943002/diff/470001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:322: checkElementHidden(false, routeList[2]); add done()
https://codereview.chromium.org/1204943002/diff/470001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1204943002/diff/470001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:17: value: function() { value: [], https://codereview.chromium.org/1204943002/diff/470001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:31: containerView_: { I don't really see how this is an improvement, but whatevs... https://codereview.chromium.org/1204943002/diff/470001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:31: containerView_: { CONTAINER_VIEW_ https://codereview.chromium.org/1204943002/diff/470001/chrome/test/data/webui... File chrome/test/data/webui/media_router/media_router_container_tests.js (right): https://codereview.chromium.org/1204943002/diff/470001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:11: * @type {media_router_container} this is not a valid type https://codereview.chromium.org/1204943002/diff/470001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:48: * @const @const {!Array<string>} https://codereview.chromium.org/1204943002/diff/470001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:64: } }; https://codereview.chromium.org/1204943002/diff/470001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:69: } }; https://codereview.chromium.org/1204943002/diff/470001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:95: } }; https://codereview.chromium.org/1204943002/diff/470001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:266: } done();? https://codereview.chromium.org/1204943002/diff/470001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:280: } done();? https://codereview.chromium.org/1204943002/diff/470001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:304: }); are any of these actually running? any setTimeout() will defer this from executing, and if test() runs sychronously and says "OK, didn't get an errors!" and doesn't wait for a done() of some sort none of these tests are useful/valid
you could also actually closure compile this code instead of writing doc that's not checked...
Patchset #5 (id:510001) has been deleted
Patchset #4 (id:490001) has been deleted
Patchset #5 (id:550001) has been deleted
> closure compile this code Thanks for chatting yesterday about this! I'll set up continuous checking for our resources directory and the tests in a separate CL. I removed parts of two tests, pending a discussion here[1] to determine how to best handle queueing element/DOM updates. I'll add them back afterwards. [1] https://groups.google.com/forum/#!topic/polymer-dev/RiZDqHtFWC8 https://codereview.chromium.org/1204943002/diff/470001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1204943002/diff/470001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:17: value: function() { On 2015/07/10 01:42:45, Dan Beam wrote: > value: [], Done here and below. michaelpg@: What was the change where Arrays/Objects don't have to be in a function anymore? Was it switching to 1.0 with synchronous loading? https://codereview.chromium.org/1204943002/diff/470001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:31: containerView_: { On 2015/07/10 01:42:45, Dan Beam wrote: > CONTAINER_VIEW_ Done. https://codereview.chromium.org/1204943002/diff/470001/chrome/test/data/webui... File chrome/test/data/webui/media_router/media_router_container_tests.js (right): https://codereview.chromium.org/1204943002/diff/470001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:11: * @type {media_router_container} On 2015/07/10 01:42:45, Dan Beam wrote: > this is not a valid type Done. https://codereview.chromium.org/1204943002/diff/470001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:48: * @const On 2015/07/10 01:42:45, Dan Beam wrote: > @const {!Array<string>} Done. https://codereview.chromium.org/1204943002/diff/470001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:64: } On 2015/07/10 01:42:45, Dan Beam wrote: > }; Done. https://codereview.chromium.org/1204943002/diff/470001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:69: } On 2015/07/10 01:42:45, Dan Beam wrote: > }; Done. https://codereview.chromium.org/1204943002/diff/470001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:95: } On 2015/07/10 01:42:45, Dan Beam wrote: > }; Done. https://codereview.chromium.org/1204943002/diff/470001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:266: } On 2015/07/10 01:42:45, Dan Beam wrote: > done();? Done. https://codereview.chromium.org/1204943002/diff/470001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:280: } On 2015/07/10 01:42:45, Dan Beam wrote: > done();? Done. https://codereview.chromium.org/1204943002/diff/470001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:304: }); On 2015/07/10 01:42:45, Dan Beam wrote: > are any of these actually running? any setTimeout() will defer this from > executing, and if test() runs sychronously and says "OK, didn't get an errors!" > and doesn't wait for a done() of some sort none of these tests are useful/valid Good call, fixed. https://codereview.chromium.org/1204943002/diff/470001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:322: checkElementHidden(false, routeList[2]); On 2015/07/10 01:26:39, michaelpg wrote: > add done() Done. https://codereview.chromium.org/1204943002/diff/470001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:330: 'cast-mode-list', On 2015/07/09 23:40:12, Kevin M wrote: > Fix indents here and elsewhere w/multi line array literals Done.
https://codereview.chromium.org/1204943002/diff/570001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (left): https://codereview.chromium.org/1204943002/diff/570001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:177: * @param {!MediaRouterContainerView} view The current view. I'm not sure this is a good change https://codereview.chromium.org/1204943002/diff/570001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1204943002/diff/570001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:405: } nit: no curlies https://codereview.chromium.org/1204943002/diff/570001/chrome/test/data/webui... File chrome/test/data/webui/media_router/media_router_container_tests.js (right): https://codereview.chromium.org/1204943002/diff/570001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:240: }); so what happens when you don't take a |done| callback? https://codereview.chromium.org/1204943002/diff/570001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:392: 'sink-list']); probably fits on one line
https://codereview.chromium.org/1204943002/diff/570001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (left): https://codereview.chromium.org/1204943002/diff/570001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:177: * @param {!MediaRouterContainerView} view The current view. On 2015/07/10 22:29:43, Dan Beam wrote: > I'm not sure this is a good change What do you prefer? https://codereview.chromium.org/1204943002/diff/570001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/1204943002/diff/570001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:405: } On 2015/07/10 22:29:43, Dan Beam wrote: > nit: no curlies Done. https://codereview.chromium.org/1204943002/diff/570001/chrome/test/data/webui... File chrome/test/data/webui/media_router/media_router_container_tests.js (right): https://codereview.chromium.org/1204943002/diff/570001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:240: }); On 2015/07/10 22:29:43, Dan Beam wrote: > so what happens when you don't take a |done| callback? It depends on the test. Here are the different types of tests and the resulting behavior with no |done| callback: - setTimeout: Test passes even if asserts fail. - Just asserts, including if |done| should be within addEventListener: Test fails if any of the asserts fail. https://codereview.chromium.org/1204943002/diff/570001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:392: 'sink-list']); On 2015/07/10 22:29:43, Dan Beam wrote: > probably fits on one line Done.
lgtm https://codereview.chromium.org/1204943002/diff/570001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (left): https://codereview.chromium.org/1204943002/diff/570001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:177: * @param {!MediaRouterContainerView} view The current view. On 2015/07/10 23:25:28, apacible wrote: > On 2015/07/10 22:29:43, Dan Beam wrote: > > I'm not sure this is a good change > > What do you prefer? using the enum (like you were before) it doesn't matter, you're not compiling any of this anyways https://codereview.chromium.org/1204943002/diff/570001/chrome/test/data/webui... File chrome/test/data/webui/media_router/media_router_container_tests.js (right): https://codereview.chromium.org/1204943002/diff/570001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:240: }); On 2015/07/10 23:25:29, apacible wrote: > On 2015/07/10 22:29:43, Dan Beam wrote: > > so what happens when you don't take a |done| callback? > > It depends on the test. Here are the different types of tests and the resulting > behavior with no |done| callback: > - setTimeout: Test passes even if asserts fail. > - Just asserts, including if |done| should be within addEventListener: Test > fails if any of the asserts fail. tl;dr - mocha uses Function.prototype.length to determine how many args you want (and whether to make the test async). this is dark magic.
https://codereview.chromium.org/1204943002/diff/570001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (left): https://codereview.chromium.org/1204943002/diff/570001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:177: * @param {!MediaRouterContainerView} view The current view. On 2015/07/10 23:56:13, Dan Beam wrote: > On 2015/07/10 23:25:28, apacible wrote: > > On 2015/07/10 22:29:43, Dan Beam wrote: > > > I'm not sure this is a good change > > > > What do you prefer? > > using the enum (like you were before) > > it doesn't matter, you're not compiling any of this anyways Acknowledged. https://codereview.chromium.org/1204943002/diff/570001/chrome/test/data/webui... File chrome/test/data/webui/media_router/media_router_container_tests.js (right): https://codereview.chromium.org/1204943002/diff/570001/chrome/test/data/webui... chrome/test/data/webui/media_router/media_router_container_tests.js:240: }); On 2015/07/10 23:56:13, Dan Beam wrote: > On 2015/07/10 23:25:29, apacible wrote: > > On 2015/07/10 22:29:43, Dan Beam wrote: > > > so what happens when you don't take a |done| callback? > > > > It depends on the test. Here are the different types of tests and the > resulting > > behavior with no |done| callback: > > - setTimeout: Test passes even if asserts fail. > > - Just asserts, including if |done| should be within addEventListener: Test > > fails if any of the asserts fail. > > tl;dr - mocha uses Function.prototype.length to determine how many args you want > (and whether to make the test async). this is dark magic. Acknowledged dark magic.
The CQ bit was checked by apacible@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kmarshall@chromium.org, michaelpg@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1204943002/#ps630001 (title: "Changes per dbeam@'s comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1204943002/630001
Message was sent while issue was closed.
Committed patchset #8 (id:630001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/98a5676d4bfc93e9e2d92f3ee386205904cdf163 Cr-Commit-Position: refs/heads/master@{#338430} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
