Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1065733004:
Added partial unit tests for host_controller.js. More to come. (Closed)
|
Created:
5 years, 8 months ago by John Williams Modified:
5 years, 8 months ago CC:
chromium-reviews, chromoting-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@hdf-unittest Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded partial unit tests for host_controller.js. More to come.
BUG=471928
Committed: https://crrev.com/f326e5ad2b86b9e9c66a2076526c1045f264f25a
Cr-Commit-Position: refs/heads/master@{#324495}
Patch Set 1 #
Total comments: 33
Patch Set 2 : for review #
Total comments: 14
Patch Set 3 : for review #Patch Set 4 : #Patch Set 5 : fix for broken merge #Patch Set 6 : added licenses to satisfy presubmit #
Messages
Total messages: 26 (11 generated)
jrw@chromium.org changed reviewers: + jamiewalch@chromium.org, kelvinp@chromium.org
https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/base/js/bas... File remoting/webapp/base/js/base.js (right): https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/base/js/bas... remoting/webapp/base/js/base.js:63: base.reinterpret_cast = function(value) { You don't seem to be using this. https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/base/js/bas... remoting/webapp/base/js/base.js:750: base.generateUuid = function() { Move this up below generateXsrfToken, since it's conceptually similar. https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/host... File remoting/webapp/crd/js/host_controller_unittest.js (right): https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/host... remoting/webapp/crd/js/host_controller_unittest.js:95: afterEach: function() { Blank line before afterEach for readability. https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/host... remoting/webapp/crd/js/host_controller_unittest.js:110: controller = new remoting.HostController(); Can this be moved into beforeEach (for symmetry with it being nulled in afterEach)? Alternatively, can this be a local variable within in each test (ie, remove the global and the nulling in afterEach)? https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/host... remoting/webapp/crd/js/host_controller_unittest.js:134: // Prepare the to execute HostController.start up to the point where Prepare the what? (here and below) https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/host... remoting/webapp/crd/js/host_controller_unittest.js:136: function initThroughHostName() { Using "through" to mean "until" is idiomatic to US English and not generally understood elsewhere. I suggest initFieldsUpToHostName as an alternative. However, can you just initialize everything (maybe in the ctor) and set to undefined the field where you want to simulate failure? That has the added advantage of being robust to the order in which the various properties are requested. https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/host... remoting/webapp/crd/js/host_controller_unittest.js:150: assert.equal(e.getDetail(), 'generateKeyPair'); Check the tag as well? https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/host... remoting/webapp/crd/js/host_controller_unittest.js:169: remoting.HostController.Feature.OAUTH_CLIENT Is there a reason why you don't want this Feature to be supported for the other tests? https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/host... File remoting/webapp/crd/js/host_daemon_facade.js (right): https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/host... remoting/webapp/crd/js/host_daemon_facade.js:205: reply.type + ', got: ' + type; No need for this change. https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/mock... File remoting/webapp/crd/js/mock_host_daemon_facade.js (right): https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/mock... remoting/webapp/crd/js/mock_host_daemon_facade.js:85: Promise.resolve().then(function() { Is this to run the callback asynchronously? Elsewhere, I've used requestAnimationFrame for that. Since we don't use requestAnimationFrame for anything else, it seems like it might be a better option since there's little chance of it interacting badly with SpyPromise, for example. https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/mock... remoting/webapp/crd/js/mock_host_daemon_facade.js:213: that.consentSupported, Nit: Indentation. https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/mock... File remoting/webapp/crd/js/mock_xhr.js (right): https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/mock... remoting/webapp/crd/js/mock_xhr.js:183: * Calls |setResponseFor| to add a 204 (no content) response. 204 is now just the default. However, it's not clear from this CL why you've added this parameter. Is it for future tests? If so, then it would be better to save it until the CL where you need it. https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/mock... remoting/webapp/crd/js/mock_xhr.js:194: xhr.setEmptyResponse(opt_status == null ? 204 : opt_status); For the most common invocation, opt_status will be undefined, not null. This will work, but I think it would be clearer if you compare to undefined. == is still probably preferable to === though.
https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/base/js/bas... File remoting/webapp/base/js/base.js (right): https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/base/js/bas... remoting/webapp/base/js/base.js:63: base.reinterpret_cast = function(value) { On 2015/04/07 01:13:11, Jamie wrote: > You don't seem to be using this. Yeah, I thought this would make certain casts less painful but it turned out not to work. Removed. https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/base/js/bas... remoting/webapp/base/js/base.js:750: base.generateUuid = function() { On 2015/04/07 01:13:12, Jamie wrote: > Move this up below generateXsrfToken, since it's conceptually similar. Done. https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/host... File remoting/webapp/crd/js/host_controller_unittest.js (right): https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/host... remoting/webapp/crd/js/host_controller_unittest.js:95: afterEach: function() { On 2015/04/07 01:13:12, Jamie wrote: > Blank line before afterEach for readability. Done. https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/host... remoting/webapp/crd/js/host_controller_unittest.js:110: controller = new remoting.HostController(); On 2015/04/07 01:13:12, Jamie wrote: > Can this be moved into beforeEach (for symmetry with it being nulled in > afterEach)? Alternatively, can this be a local variable within in each test (ie, > remove the global and the nulling in afterEach)? Made local. https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/host... remoting/webapp/crd/js/host_controller_unittest.js:134: // Prepare the to execute HostController.start up to the point where On 2015/04/07 01:13:12, Jamie wrote: > Prepare the what? (here and below) The word "the" wasn't supposed to be there. https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/host... remoting/webapp/crd/js/host_controller_unittest.js:136: function initThroughHostName() { On 2015/04/07 01:13:12, Jamie wrote: > Using "through" to mean "until" is idiomatic to US English and not generally > understood elsewhere. I suggest initFieldsUpToHostName as an alternative. News to me. I'll keep that in mind. > However, can you just initialize everything (maybe in the ctor) and set to > undefined the field where you want to simulate failure? That has the added > advantage of being robust to the order in which the various properties are > requested. The idea was to do the minimal initialization required for each test so that, for instance, getting the daemon version can't depend creating a PIN hash. Anyway, I rewrote it to initialize everything in beforeEach. I don't want to do it in the MockHostDaemonFacade constructor because I want all the specific values used in the test to be in one place. Since HostDaemonFacade is only used by HostController, this won't result in any duplication of code. https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/host... remoting/webapp/crd/js/host_controller_unittest.js:150: assert.equal(e.getDetail(), 'generateKeyPair'); On 2015/04/07 01:13:12, Jamie wrote: > Check the tag as well? Seems redundant since it's always UNEXPECTED and the detail field uniquely identifies which throw statement the exception came from. https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/host... remoting/webapp/crd/js/host_controller_unittest.js:169: remoting.HostController.Feature.OAUTH_CLIENT On 2015/04/07 01:13:12, Jamie wrote: > Is there a reason why you don't want this Feature to be supported for the other > tests? Minimalism again. I want the tests to demonstrate that everything works without this feature. I changed things to include that feature my default, since it seems to be the default case in real life. https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/mock... File remoting/webapp/crd/js/mock_host_daemon_facade.js (right): https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/mock... remoting/webapp/crd/js/mock_host_daemon_facade.js:85: Promise.resolve().then(function() { On 2015/04/07 01:13:12, Jamie wrote: > Is this to run the callback asynchronously? Elsewhere, I've used > requestAnimationFrame for that. Since we don't use requestAnimationFrame for > anything else, it seems like it might be a better option since there's little > chance of it interacting badly with SpyPromise, for example. I'm deliberately using promises to allow unit tests to interact with SpyPromise (cf. lines 2-4). My intention in writing SpyPromise.run was to create a way to say, essentially "wait until all the async stuff has finished", so using requestAnimationFrame actually breaks one of my design goals for SpyPromise. I also prefer to use promises because Chrome can re-construct the stack trace for async calls through promises, and I don't know if it does the same thing with requestAnimationFrame. I'm open to changing it since I don't know if the interaction with SpyPromise is something I'll actually want to use, but window.requestAnimationFrame just seems like a weirdly specialized API with a lot of cognitive baggage attached to it, whereas the semantics of the promise version are predictable just from knowing how promises work. I've said my peace so let me know if you still want to me change it. https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/mock... remoting/webapp/crd/js/mock_host_daemon_facade.js:213: that.consentSupported, On 2015/04/07 01:13:12, Jamie wrote: > Nit: Indentation. I don't understand. I thought 4 spaces was the standard for continued arguments of a function call. https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/mock... File remoting/webapp/crd/js/mock_xhr.js (right): https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/mock... remoting/webapp/crd/js/mock_xhr.js:183: * Calls |setResponseFor| to add a 204 (no content) response. On 2015/04/07 01:13:12, Jamie wrote: > 204 is now just the default. However, it's not clear from this CL why you've > added this parameter. Is it for future tests? If so, then it would be better to > save it until the CL where you need it. I added this so I could use this method for a 500 response in host_controller_unittest.js (line 188 in the first patch). https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/mock... remoting/webapp/crd/js/mock_xhr.js:194: xhr.setEmptyResponse(opt_status == null ? 204 : opt_status); On 2015/04/07 01:13:12, Jamie wrote: > For the most common invocation, opt_status will be undefined, not null. This > will work, but I think it would be clearer if you compare to undefined. == is > still probably preferable to === though. In google3 I'd use goog.isDef, but here I've been using "== null" to test for undefined in cases where null is either not allowed or it should be treated the same as undefined. For future reference, would you prefer "=== undefined" in those cases?
https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/host... File remoting/webapp/crd/js/host_controller_unittest.js (right): https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/host... remoting/webapp/crd/js/host_controller_unittest.js:150: assert.equal(e.getDetail(), 'generateKeyPair'); On 2015/04/07 20:10:26, John Williams wrote: > On 2015/04/07 01:13:12, Jamie wrote: > > Check the tag as well? > > Seems redundant since it's always UNEXPECTED and the detail field uniquely > identifies which throw statement the exception came from. It's only UNEXPECTED if the code is bug-free :) If this code were to start returning a different error code, it would result in confusing UI being displayed to the user, so I think it's worth checking. https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/mock... File remoting/webapp/crd/js/mock_host_daemon_facade.js (right): https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/mock... remoting/webapp/crd/js/mock_host_daemon_facade.js:85: Promise.resolve().then(function() { On 2015/04/07 20:10:26, John Williams wrote: > On 2015/04/07 01:13:12, Jamie wrote: > > Is this to run the callback asynchronously? Elsewhere, I've used > > requestAnimationFrame for that. Since we don't use requestAnimationFrame for > > anything else, it seems like it might be a better option since there's little > > chance of it interacting badly with SpyPromise, for example. > > I'm deliberately using promises to allow unit tests to interact with SpyPromise > (cf. lines 2-4). My intention in writing SpyPromise.run was to create a way to > say, essentially "wait until all the async stuff has finished", so using > requestAnimationFrame actually breaks one of my design goals for SpyPromise. I > also prefer to use promises because Chrome can re-construct the stack trace for > async calls through promises, and I don't know if it does the same thing with > requestAnimationFrame. > > I'm open to changing it since I don't know if the interaction with SpyPromise is > something I'll actually want to use, but window.requestAnimationFrame just seems > like a weirdly specialized API with a lot of cognitive baggage attached to it, > whereas the semantics of the promise version are predictable just from knowing > how promises work. I've said my peace so let me know if you still want to me > change it. No, those are all well-reasoned arguments. I only used requestAnimationFrame because setTimeout doesn't generally work in unit tests; it never occurred to me to use a Promise, but I think that would have been better. https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/mock... remoting/webapp/crd/js/mock_host_daemon_facade.js:213: that.consentSupported, On 2015/04/07 20:10:26, John Williams wrote: > On 2015/04/07 01:13:12, Jamie wrote: > > Nit: Indentation. > > I don't understand. I thought 4 spaces was the standard for continued arguments > of a function call. You can either do that, or align them at the open-paren. In this case, it looks strange to me this way (I think because there's so little overlap between the two lines) and I've have put the first parameter on the previous line. That said, this is style-guide conformant, so I'm not going to insist. https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/mock... File remoting/webapp/crd/js/mock_xhr.js (right): https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/mock... remoting/webapp/crd/js/mock_xhr.js:183: * Calls |setResponseFor| to add a 204 (no content) response. On 2015/04/07 20:10:26, John Williams wrote: > On 2015/04/07 01:13:12, Jamie wrote: > > 204 is now just the default. However, it's not clear from this CL why you've > > added this parameter. Is it for future tests? If so, then it would be better > to > > save it until the CL where you need it. > > I added this so I could use this method for a 500 response in > host_controller_unittest.js (line 188 in the first patch). Okay, but please update the comment, since it doesn't necessarily use 204. You can add that detail as documentation for the opt_status parameter. https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/mock... remoting/webapp/crd/js/mock_xhr.js:194: xhr.setEmptyResponse(opt_status == null ? 204 : opt_status); On 2015/04/07 20:10:26, John Williams wrote: > On 2015/04/07 01:13:12, Jamie wrote: > > For the most common invocation, opt_status will be undefined, not null. This > > will work, but I think it would be clearer if you compare to undefined. == is > > still probably preferable to === though. > > In google3 I'd use goog.isDef, but here I've been using "== null" to test for > undefined in cases where null is either not allowed or it should be treated the > same as undefined. For future reference, would you prefer "=== undefined" in > those cases? I guess the question here is whether or not we want to explicitly allow: setEmptyResponseFor(method, pattern, null) in place of: setEmptyResponseFor(method, pattern) I can't think of any reason why a caller would want to write that (maybe if they wanted to set opt_reuse but not opt_status, but even then undefined would be more "correct") so I lean towards using '=== undefined' and having the code fail is the caller passes null. If you want to be more lenient, then '== undefined' is acceptable, but I feel that '== null' is misleading as to the expected value of omitted parameters. https://codereview.chromium.org/1065733004/diff/20001/remoting/webapp/crd/js/... File remoting/webapp/crd/js/host_controller_unittest.js (right): https://codereview.chromium.org/1065733004/diff/20001/remoting/webapp/crd/js/... remoting/webapp/crd/js/host_controller_unittest.js:128: * Install The comment doesn't seem to have any relation to the function name. https://codereview.chromium.org/1065733004/diff/20001/remoting/webapp/crd/js/... remoting/webapp/crd/js/host_controller_unittest.js:192: var controller = new remoting.HostController(); Is the code from here on common to all tests? Can it be factored out? https://codereview.chromium.org/1065733004/diff/20001/remoting/webapp/crd/js/... remoting/webapp/crd/js/host_controller_unittest.js:198: assert.equal(getCredentialsFromAuthCodeSpy.callCount, 0); Should it be a test failure if this was called? We definitely want to assert that startDaemon is not called, but be careful that you're not verifying the precise order in which the various attributes are queried (which we should be free to change without breaking tests). https://codereview.chromium.org/1065733004/diff/20001/remoting/webapp/crd/js/... remoting/webapp/crd/js/host_controller_unittest.js:332: mockHostDaemonFacade.startDaemonResult = null; Why not FAILED/FAILED_DIRECTORY?
https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/host... File remoting/webapp/crd/js/host_controller_unittest.js (right): https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/host... remoting/webapp/crd/js/host_controller_unittest.js:150: assert.equal(e.getDetail(), 'generateKeyPair'); On 2015/04/08 00:16:14, Jamie wrote: > On 2015/04/07 20:10:26, John Williams wrote: > > On 2015/04/07 01:13:12, Jamie wrote: > > > Check the tag as well? > > > > Seems redundant since it's always UNEXPECTED and the detail field uniquely > > identifies which throw statement the exception came from. > > It's only UNEXPECTED if the code is bug-free :) If this code were to start > returning a different error code, it would result in confusing UI being > displayed to the user, so I think it's worth checking. Done. https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/mock... File remoting/webapp/crd/js/mock_host_daemon_facade.js (right): https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/mock... remoting/webapp/crd/js/mock_host_daemon_facade.js:85: Promise.resolve().then(function() { On 2015/04/08 00:16:14, Jamie wrote: > On 2015/04/07 20:10:26, John Williams wrote: > > On 2015/04/07 01:13:12, Jamie wrote: > > > Is this to run the callback asynchronously? Elsewhere, I've used > > > requestAnimationFrame for that. Since we don't use requestAnimationFrame for > > > anything else, it seems like it might be a better option since there's > little > > > chance of it interacting badly with SpyPromise, for example. > > > > I'm deliberately using promises to allow unit tests to interact with > SpyPromise > > (cf. lines 2-4). My intention in writing SpyPromise.run was to create a way > to > > say, essentially "wait until all the async stuff has finished", so using > > requestAnimationFrame actually breaks one of my design goals for SpyPromise. > I > > also prefer to use promises because Chrome can re-construct the stack trace > for > > async calls through promises, and I don't know if it does the same thing with > > requestAnimationFrame. > > > > I'm open to changing it since I don't know if the interaction with SpyPromise > is > > something I'll actually want to use, but window.requestAnimationFrame just > seems > > like a weirdly specialized API with a lot of cognitive baggage attached to it, > > whereas the semantics of the promise version are predictable just from knowing > > how promises work. I've said my peace so let me know if you still want to me > > change it. > > No, those are all well-reasoned arguments. I only used requestAnimationFrame > because setTimeout doesn't generally work in unit tests; it never occurred to me > to use a Promise, but I think that would have been better. Acknowledged. https://codereview.chromium.org/1065733004/diff/1/remoting/webapp/crd/js/mock... remoting/webapp/crd/js/mock_host_daemon_facade.js:213: that.consentSupported, On 2015/04/08 00:16:14, Jamie wrote: > On 2015/04/07 20:10:26, John Williams wrote: > > On 2015/04/07 01:13:12, Jamie wrote: > > > Nit: Indentation. > > > > I don't understand. I thought 4 spaces was the standard for continued > arguments > > of a function call. > > You can either do that, or align them at the open-paren. In this case, it looks > strange to me this way (I think because there's so little overlap between the > two lines) and I've have put the first parameter on the previous line. That > said, this is style-guide conformant, so I'm not going to insist. I'm generally not a fan of aligning to the first parameter, because it breaks if a refactoring changes the name of the function. IIRC it's even against some of our other style guides. https://codereview.chromium.org/1065733004/diff/20001/remoting/webapp/crd/js/... File remoting/webapp/crd/js/host_controller_unittest.js (right): https://codereview.chromium.org/1065733004/diff/20001/remoting/webapp/crd/js/... remoting/webapp/crd/js/host_controller_unittest.js:128: * Install On 2015/04/08 00:16:15, Jamie wrote: > The comment doesn't seem to have any relation to the function name. Must've typed errant ^K while I was thinking about something else. https://codereview.chromium.org/1065733004/diff/20001/remoting/webapp/crd/js/... remoting/webapp/crd/js/host_controller_unittest.js:192: var controller = new remoting.HostController(); On 2015/04/08 00:16:14, Jamie wrote: > Is the code from here on common to all tests? Can it be factored out? Done. https://codereview.chromium.org/1065733004/diff/20001/remoting/webapp/crd/js/... remoting/webapp/crd/js/host_controller_unittest.js:198: assert.equal(getCredentialsFromAuthCodeSpy.callCount, 0); On 2015/04/08 00:16:14, Jamie wrote: > Should it be a test failure if this was called? We definitely want to assert > that startDaemon is not called, but be careful that you're not verifying the > precise order in which the various attributes are queried (which we should be > free to change without breaking tests). Good point. Changed. https://codereview.chromium.org/1065733004/diff/20001/remoting/webapp/crd/js/... remoting/webapp/crd/js/host_controller_unittest.js:332: mockHostDaemonFacade.startDaemonResult = null; On 2015/04/08 00:16:14, Jamie wrote: > Why not FAILED/FAILED_DIRECTORY? Added this as a separate case, but there are two distinct ways this method can "fail". Either it calls onDone with a failure code, or it calls onError.
https://codereview.chromium.org/1065733004/diff/20001/remoting/webapp/crd/js/... File remoting/webapp/crd/js/host_controller_unittest.js (right): https://codereview.chromium.org/1065733004/diff/20001/remoting/webapp/crd/js/... remoting/webapp/crd/js/host_controller_unittest.js:192: var controller = new remoting.HostController(); On 2015/04/08 20:20:09, John Williams wrote: > On 2015/04/08 00:16:14, Jamie wrote: > > Is the code from here on common to all tests? Can it be factored out? > > Done. IIRC, the controller used to be global and you opted to make it local in an earlier revision. FWIW, I prefer it global, but my comment was actually referring to the code following this line that starts the controller and asserts various things, and which seems to be largely copy/pasted between functions. https://codereview.chromium.org/1065733004/diff/20001/remoting/webapp/crd/js/... remoting/webapp/crd/js/host_controller_unittest.js:332: mockHostDaemonFacade.startDaemonResult = null; On 2015/04/08 20:20:09, John Williams wrote: > On 2015/04/08 00:16:14, Jamie wrote: > > Why not FAILED/FAILED_DIRECTORY? > > Added this as a separate case, but there are two distinct ways this method can > "fail". Either it calls onDone with a failure code, or it calls onError. Yuck. That sounds like something worth cleaning up as part of your refactoring (ie, not this CL), if it's not too much additional work.
https://codereview.chromium.org/1065733004/diff/20001/remoting/webapp/crd/js/... File remoting/webapp/crd/js/host_controller_unittest.js (right): https://codereview.chromium.org/1065733004/diff/20001/remoting/webapp/crd/js/... remoting/webapp/crd/js/host_controller_unittest.js:192: var controller = new remoting.HostController(); On 2015/04/08 22:29:29, Jamie wrote: > On 2015/04/08 20:20:09, John Williams wrote: > > On 2015/04/08 00:16:14, Jamie wrote: > > > Is the code from here on common to all tests? Can it be factored out? > > > > Done. > > IIRC, the controller used to be global and you opted to make it local in an > earlier revision. FWIW, I prefer it global, but my comment was actually > referring to the code following this line that starts the controller and asserts > various things, and which seems to be largely copy/pasted between functions. Right, I did. I misunderstood the comment, but I think having it be global is probably the right way to do now that I've added the mockGetClientBaseJid function...OTOH, that function is going away in my next CL, so I guess I'll make it local again in that CL :-/ Anyway, the assertions are such that they're the same in a lot of test cases, but none of them are the same in *every* case; I think I've moved everything I can into the beforeEach function. The failure test cases just happen to be very similar to each other. It's possible to factor out some more common functionality into helper functions, but I think you quickly hit the point of diminishing returns when you do that in unit tests. IMHO that what's desirable in test code is often exactly the opposite of what's desirable in production code. In this case, I think some copypasta is OK because being obvious is more important than being concise or future-proof. https://codereview.chromium.org/1065733004/diff/20001/remoting/webapp/crd/js/... remoting/webapp/crd/js/host_controller_unittest.js:332: mockHostDaemonFacade.startDaemonResult = null; On 2015/04/08 22:29:29, Jamie wrote: > On 2015/04/08 20:20:09, John Williams wrote: > > On 2015/04/08 00:16:14, Jamie wrote: > > > Why not FAILED/FAILED_DIRECTORY? > > > > Added this as a separate case, but there are two distinct ways this method can > > "fail". Either it calls onDone with a failure code, or it calls onError. > > Yuck. That sounds like something worth cleaning up as part of your refactoring > (ie, not this CL), if it's not too much additional work. SGTM. When I work on legacy code I usually end up itching to do various refactorings, so I'm happy to do it, provided it doesn't adversely affect me come perf time.
lgtm https://codereview.chromium.org/1065733004/diff/20001/remoting/webapp/crd/js/... File remoting/webapp/crd/js/host_controller_unittest.js (right): https://codereview.chromium.org/1065733004/diff/20001/remoting/webapp/crd/js/... remoting/webapp/crd/js/host_controller_unittest.js:192: var controller = new remoting.HostController(); On 2015/04/08 23:19:55, John Williams wrote: > On 2015/04/08 22:29:29, Jamie wrote: > > On 2015/04/08 20:20:09, John Williams wrote: > > > On 2015/04/08 00:16:14, Jamie wrote: > > > > Is the code from here on common to all tests? Can it be factored out? > > > > > > Done. > > > > IIRC, the controller used to be global and you opted to make it local in an > > earlier revision. FWIW, I prefer it global, but my comment was actually > > referring to the code following this line that starts the controller and > asserts > > various things, and which seems to be largely copy/pasted between functions. > > Right, I did. I misunderstood the comment, but I think having it be global is > probably the right way to do now that I've added the mockGetClientBaseJid > function...OTOH, that function is going away in my next CL, so I guess I'll make > it local again in that CL :-/ > > Anyway, the assertions are such that they're the same in a lot of test cases, > but none of them are the same in *every* case; I think I've moved everything I > can into the beforeEach function. The failure test cases just happen to be very > similar to each other. > > It's possible to factor out some more common functionality into helper > functions, but I think you quickly hit the point of diminishing returns when you > do that in unit tests. IMHO that what's desirable in test code is often exactly > the opposite of what's desirable in production code. In this case, I think some > copypasta is OK because being obvious is more important than being concise or > future-proof. Acknowledged. https://codereview.chromium.org/1065733004/diff/20001/remoting/webapp/crd/js/... remoting/webapp/crd/js/host_controller_unittest.js:332: mockHostDaemonFacade.startDaemonResult = null; On 2015/04/08 23:19:55, John Williams wrote: > On 2015/04/08 22:29:29, Jamie wrote: > > On 2015/04/08 20:20:09, John Williams wrote: > > > On 2015/04/08 00:16:14, Jamie wrote: > > > > Why not FAILED/FAILED_DIRECTORY? > > > > > > Added this as a separate case, but there are two distinct ways this method > can > > > "fail". Either it calls onDone with a failure code, or it calls onError. > > > > Yuck. That sounds like something worth cleaning up as part of your refactoring > > (ie, not this CL), if it's not too much additional work. > > SGTM. When I work on legacy code I usually end up itching to do various > refactorings, so I'm happy to do it, provided it doesn't adversely affect me > come perf time. Acknowledged.
The CQ bit was checked by jrw@chromium.org
The CQ bit was unchecked by jrw@chromium.org
The CQ bit was checked by jrw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamiewalch@chromium.org Link to the patchset: https://codereview.chromium.org/1065733004/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1065733004/60001
On 2015/04/08 23:58:36, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1065733004/60001 FYI, there's a notification sent out when you modify a CL between its LGTM and landing it. This happens a lot, most commonly for a rebase as is the case here, but it's helpful to include a patch-set title to that effect so that suspicious reviewers like myself don't wonder what you're trying to sneak into the codebase :) In general, informative patch-set titles are good, even if it's just "Reviewer feedback."
The CQ bit was unchecked by jrw@chromium.org
The CQ bit was checked by jrw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamiewalch@chromium.org Link to the patchset: https://codereview.chromium.org/1065733004/#ps80001 (title: "fix for broken merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1065733004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by jrw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamiewalch@chromium.org Link to the patchset: https://codereview.chromium.org/1065733004/#ps100001 (title: "added licenses to satisfy presubmit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1065733004/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/f326e5ad2b86b9e9c66a2076526c1045f264f25a Cr-Commit-Position: refs/heads/master@{#324495} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
