Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23477006:
Unit tests for utility.js (Closed)
|
Created:
7 years, 3 months ago by vadimt Modified:
7 years, 3 months ago CC:
chromium-reviews, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUnit tests for utility.js.
Adding tests for error reporting and wrapper, the rest will follow.
BUG=164227
TEST=No
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221239
Patch Set 1 #
Total comments: 16
Patch Set 2 : robliao@ comments #
Total comments: 18
Patch Set 3 : skare@ comments #Patch Set 4 : rgustafson's comments #Patch Set 5 : More comments #
Total comments: 13
Patch Set 6 : arv@ notes #Patch Set 7 : Fixing unit test + more arv@ comments #
Messages
Total messages: 27 (0 generated)
https://codereview.chromium.org/23477006/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23477006/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:26: // Test sending report for an error with a message that can be sent to server. Where is the test that checks for first line removals? https://codereview.chromium.org/23477006/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:156: // Expectations. Incomplete comment? (and all others) https://codereview.chromium.org/23477006/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:163: // Invoking tested function. Spacing. (and all others) https://codereview.chromium.org/23477006/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:167: // Step 3. Checking that after the callback we are again in non-instrumented Let's not enumerate the steps. At the point you need to add something in between, you have to renumber the entire sequence.
https://codereview.chromium.org/23477006/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23477006/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:26: // Test sending report for an error with a message that can be sent to server. On 2013/08/28 22:05:29, Robert Liao wrote: > Where is the test that checks for first line removals? Line 94. https://codereview.chromium.org/23477006/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:156: // Expectations. On 2013/08/28 22:05:29, Robert Liao wrote: > Incomplete comment? (and all others) No, simply Step 2 is divided into Expectations and Invocation. They are indented because they are sub-sections of the step. When steps don't include expectations, I collapse comments. https://codereview.chromium.org/23477006/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:163: // Invoking tested function. On 2013/08/28 22:05:29, Robert Liao wrote: > Spacing. (and all others) See above. https://codereview.chromium.org/23477006/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:167: // Step 3. Checking that after the callback we are again in non-instrumented On 2013/08/28 22:05:29, Robert Liao wrote: > Let's not enumerate the steps. At the point you need to add something in > between, you have to renumber the entire sequence. Numbering really helps to read the code. You want to find the next step in the code, and you don't want to skip a // Step comment in between. Having numbers really helps.
https://codereview.chromium.org/23477006/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23477006/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:72: // Test sending report for an error with a message that cannot be sent to cannot --> should not https://codereview.chromium.org/23477006/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:156: // Expectations. No other comments that I've seen thus far do this. You're better off not indenting because further comments considered in isolation (as will appear in diffs) appear to be violating the commenting convention. On 2013/08/28 22:29:35, vadimt wrote: > On 2013/08/28 22:05:29, Robert Liao wrote: > > Incomplete comment? (and all others) > > No, simply Step 2 is divided into Expectations and Invocation. > They are indented because they are sub-sections of the step. > > When steps don't include expectations, I collapse comments. https://codereview.chromium.org/23477006/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:167: // Step 3. Checking that after the callback we are again in non-instrumented Your choice, as this is test code. All further insertions will cause a reenumeration of the entire list. You may be better off summarizing the steps in one block comment above and sign posting in the middle of the code. On 2013/08/28 22:29:35, vadimt wrote: > On 2013/08/28 22:05:29, Robert Liao wrote: > > Let's not enumerate the steps. At the point you need to add something in > > between, you have to renumber the entire sequence. > > Numbering really helps to read the code. > You want to find the next step in the code, and you don't want to skip a // Step > comment in between. Having numbers really helps.
https://codereview.chromium.org/23477006/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23477006/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:72: // Test sending report for an error with a message that cannot be sent to On 2013/08/28 22:42:31, Robert Liao wrote: > cannot --> should not Done. https://codereview.chromium.org/23477006/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:156: // Expectations. On 2013/08/28 22:42:31, Robert Liao wrote: > No other comments that I've seen thus far do this. You're better off not > indenting because further comments considered in isolation (as will appear in > diffs) appear to be violating the commenting convention. > On 2013/08/28 22:29:35, vadimt wrote: > > On 2013/08/28 22:05:29, Robert Liao wrote: > > > Incomplete comment? (and all others) > > > > No, simply Step 2 is divided into Expectations and Invocation. > > They are indented because they are sub-sections of the step. > > > > When steps don't include expectations, I collapse comments. > Done. https://codereview.chromium.org/23477006/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:167: // Step 3. Checking that after the callback we are again in non-instrumented On 2013/08/28 22:42:31, Robert Liao wrote: > Your choice, as this is test code. > > All further insertions will cause a reenumeration of the entire list. You may be > better off summarizing the steps in one block comment above and sign posting in > the middle of the code. > > On 2013/08/28 22:29:35, vadimt wrote: > > On 2013/08/28 22:05:29, Robert Liao wrote: > > > Let's not enumerate the steps. At the point you need to add something in > > > between, you have to renumber the entire sequence. > > > > Numbering really helps to read the code. > > You want to find the next step in the code, and you don't want to skip a // > Step > > comment in between. Having numbers really helps. > I feel that the current way is the best :) Thanks!
lgtm
minor stuff on one file, need to look at the actual test onward. https://codereview.chromium.org/23477006/diff/8001/chrome/browser/resources/g... File chrome/browser/resources/google_now/common_test_util.js (right): https://codereview.chromium.org/23477006/diff/8001/chrome/browser/resources/g... chrome/browser/resources/google_now/common_test_util.js:7: function emptyMock() {} the extension is assembled with sourcefile concatenation right? Need to guard against naming collisions in the global namespace? if so, self-execing anonymouse function? https://codereview.chromium.org/23477006/diff/8001/chrome/browser/resources/g... chrome/browser/resources/google_now/common_test_util.js:14: * handlers to arrays stored inside mockEventHandlers. hide implementation details? e.g. "mocked function will keep track of handlers?" (or not) https://codereview.chromium.org/23477006/diff/8001/chrome/browser/resources/g... chrome/browser/resources/google_now/common_test_util.js:16: * function. Can be either chrome or instrumented. nit: maybe 'chrome' or 'instrumented'. for consistency with 'addListener' and 'runtime.onSuspend' elsewhere in comment. https://codereview.chromium.org/23477006/diff/8001/chrome/browser/resources/g... chrome/browser/resources/google_now/common_test_util.js:22: var eventName = eventIdentifierParts.pop(); maybe check that (eventName != undefined) or (eventIdentifierParts.length > 0) https://codereview.chromium.org/23477006/diff/8001/chrome/browser/resources/g... chrome/browser/resources/google_now/common_test_util.js:25: eventIdentifierParts.forEach(function(fragment) { js styleguide suggests using normal iteration over for-in with arrays; not sure if that extends to array.foreach. Seems to avoid the underlying problem (expandos aren't included with forEach) though, so probably no action required. https://codereview.chromium.org/23477006/diff/8001/chrome/browser/resources/g... chrome/browser/resources/google_now/common_test_util.js:27: originalMethodContainer[fragment] = quick check on this and 29-31 -- intended to be (x = x[prop] = x[prop] || {})? can chat quickly in person
the one comment I had so far so it doesn't get missed. https://codereview.chromium.org/23477006/diff/1/chrome/test/data/webui/test_a... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/23477006/diff/1/chrome/test/data/webui/test_a... chrome/test/data/webui/test_api.js:1671: * Mock4JS matcher object that matches the actual agrument and the expected argument
one more, still need to check the test https://codereview.chromium.org/23477006/diff/8001/chrome/test/data/webui/tes... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/23477006/diff/8001/chrome/test/data/webui/tes... chrome/test/data/webui/test_api.js:1683: * Checks that the string representation of the actual argument, and the nit: no comma before 'and'
https://codereview.chromium.org/23477006/diff/8001/chrome/browser/resources/g... File chrome/browser/resources/google_now/common_test_util.js (right): https://codereview.chromium.org/23477006/diff/8001/chrome/browser/resources/g... chrome/browser/resources/google_now/common_test_util.js:7: function emptyMock() {} On 2013/08/29 00:03:37, Travis Skare wrote: > the extension is assembled with sourcefile concatenation right? Need to guard > against naming collisions in the global namespace? if so, self-execing > anonymouse function? This is not a part of the extension. There are 3 independent gtestjs files. Each of them is assembled independently, like: extraLibraries: [ 'common_test_util.js', 'utility_test_util.js', 'utility.js' ] and we don't have collisions. https://codereview.chromium.org/23477006/diff/8001/chrome/browser/resources/g... chrome/browser/resources/google_now/common_test_util.js:14: * handlers to arrays stored inside mockEventHandlers. On 2013/08/29 00:03:37, Travis Skare wrote: > hide implementation details? e.g. "mocked function will keep track of handlers?" > (or not) Done. https://codereview.chromium.org/23477006/diff/8001/chrome/browser/resources/g... chrome/browser/resources/google_now/common_test_util.js:16: * function. Can be either chrome or instrumented. On 2013/08/29 00:03:37, Travis Skare wrote: > nit: maybe > 'chrome' or 'instrumented'. > > for consistency with 'addListener' and 'runtime.onSuspend' elsewhere in comment. Done. https://codereview.chromium.org/23477006/diff/8001/chrome/browser/resources/g... chrome/browser/resources/google_now/common_test_util.js:22: var eventName = eventIdentifierParts.pop(); On 2013/08/29 00:03:37, Travis Skare wrote: > maybe check that (eventName != undefined) or (eventIdentifierParts.length > 0) Then it will crash. This is just a test anyways. https://codereview.chromium.org/23477006/diff/8001/chrome/browser/resources/g... chrome/browser/resources/google_now/common_test_util.js:25: eventIdentifierParts.forEach(function(fragment) { On 2013/08/29 00:03:37, Travis Skare wrote: > js styleguide suggests using normal iteration over for-in with arrays; not sure > if that extends to array.foreach. Seems to avoid the underlying problem > (expandos aren't included with forEach) though, so probably no action required. This is because for-in can lead to errors, not because of stylistic preferences. forEach works correctly, therefore, there are no reasons to not use it. arv@, one of style guide authors, suggested using it (as a better alternative to map()). https://codereview.chromium.org/23477006/diff/8001/chrome/browser/resources/g... chrome/browser/resources/google_now/common_test_util.js:27: originalMethodContainer[fragment] = On 2013/08/29 00:03:37, Travis Skare wrote: > quick check on this and 29-31 -- intended to be (x = x[prop] = x[prop] || {})? > can chat quickly in person This is exactly what happens: originalMethodContainer = originalMethodContainer[fragment] = originalMethodContainer[fragment] || {};
https://codereview.chromium.org/23477006/diff/1/chrome/test/data/webui/test_a... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/23477006/diff/1/chrome/test/data/webui/test_a... chrome/test/data/webui/test_api.js:1671: * Mock4JS matcher object that matches the actual agrument and the expected On 2013/08/29 00:07:14, rgustafson wrote: > argument Done. https://codereview.chromium.org/23477006/diff/8001/chrome/test/data/webui/tes... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/23477006/diff/8001/chrome/test/data/webui/tes... chrome/test/data/webui/test_api.js:1683: * Checks that the string representation of the actual argument, and the On 2013/08/29 00:26:14, Travis Skare wrote: > nit: no comma before 'and' I did this to avoid this interpretation: the string representation of (the actual argument and the expected value) are same We get string representation only for the actual argument, and use the expected value as is. Still insisting?
https://codereview.chromium.org/23477006/diff/8001/chrome/test/data/webui/tes... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/23477006/diff/8001/chrome/test/data/webui/tes... chrome/test/data/webui/test_api.js:1683: * Checks that the string representation of the actual argument, and the On 2013/08/29 00:35:32, vadimt wrote: > On 2013/08/29 00:26:14, Travis Skare wrote: > > nit: no comma before 'and' > > I did this to avoid this interpretation: > the string representation of (the actual argument and the expected value) are > same > We get string representation only for the actual argument, and use the expected > value as is. > > Still insisting? sort of, but I'm not witholding an lg over it or anything :) I think without the comma it's pretty clear as (check (isSame (stringRep argument) expected)) someone else can confirm. Whatever you prefer. Or you can rewrite, e.g.: Checks that the string representation of the actual argument matches the expected value. https://codereview.chromium.org/23477006/diff/8001/chrome/test/data/webui/tes... chrome/test/data/webui/test_api.js:1684: * expected value are same. though if not changing the above, s/are same/are the same
https://codereview.chromium.org/23477006/diff/8001/chrome/test/data/webui/tes... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/23477006/diff/8001/chrome/test/data/webui/tes... chrome/test/data/webui/test_api.js:1683: * Checks that the string representation of the actual argument, and the On 2013/08/29 00:48:09, Travis Skare wrote: > On 2013/08/29 00:35:32, vadimt wrote: > > On 2013/08/29 00:26:14, Travis Skare wrote: > > > nit: no comma before 'and' > > > > I did this to avoid this interpretation: > > the string representation of (the actual argument and the expected value) are > > same > > We get string representation only for the actual argument, and use the > expected > > value as is. > > > > Still insisting? > > sort of, but I'm not witholding an lg over it or anything :) > I think without the comma it's pretty clear as > (check (isSame (stringRep argument) expected)) > someone else can confirm. Whatever you prefer. Or you can rewrite, e.g.: > > Checks that the string representation of the actual argument matches the > expected value. Done. https://codereview.chromium.org/23477006/diff/8001/chrome/test/data/webui/tes... chrome/test/data/webui/test_api.js:1684: * expected value are same. On 2013/08/29 00:48:09, Travis Skare wrote: > though if not changing the above, s/are same/are the same Done.
https://codereview.chromium.org/23477006/diff/25001/chrome/browser/resources/... File chrome/browser/resources/google_now/common_test_util.js (right): https://codereview.chromium.org/23477006/diff/25001/chrome/browser/resources/... chrome/browser/resources/google_now/common_test_util.js:26: originalMethodContainer = if I'm reading this right (and I might not be) I think you can just do originalMethodContainer = originalMethodContainer[fragment] || {}. and skip the middle assignment?
https://codereview.chromium.org/23477006/diff/25001/chrome/browser/resources/... File chrome/browser/resources/google_now/common_test_util.js (right): https://codereview.chromium.org/23477006/diff/25001/chrome/browser/resources/... chrome/browser/resources/google_now/common_test_util.js:26: originalMethodContainer = On 2013/08/29 21:28:22, Travis Skare wrote: > if I'm reading this right (and I might not be) I think you can just do > originalMethodContainer = originalMethodContainer[fragment] || {}. > and skip the middle assignment? never mind -- this can already exist and you need to create so the check is needed.
https://codereview.chromium.org/23477006/diff/25001/chrome/browser/resources/... File chrome/browser/resources/google_now/common_test_util.js (right): https://codereview.chromium.org/23477006/diff/25001/chrome/browser/resources/... chrome/browser/resources/google_now/common_test_util.js:26: originalMethodContainer = On 2013/08/29 21:28:22, Travis Skare wrote: > if I'm reading this right (and I might not be) I think you can just do > originalMethodContainer = originalMethodContainer[fragment] || {}. > and skip the middle assignment? Discussed in person. Needed to create intermediate levels.
lgtm
arv@, please provide OWNER's approval
LGTM https://codereview.chromium.org/23477006/diff/25001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility_test_util.js (right): https://codereview.chromium.org/23477006/diff/25001/chrome/browser/resources/... chrome/browser/resources/google_now/utility_test_util.js:9: chrome['alarms'] = { why not chrome.alarms? https://codereview.chromium.org/23477006/diff/25001/chrome/test/data/webui/te... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/23477006/diff/25001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:1674: * @param {string} expectedValue Expected value. useless description https://codereview.chromium.org/23477006/diff/25001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:1685: * @param {Object} actualArgument The argument to match. {*} https://codereview.chromium.org/23477006/diff/25001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:1689: return this.expectedValue_ === actualArgument.toString(); String(actualArgument)
https://codereview.chromium.org/23477006/diff/25001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility_test_util.js (right): https://codereview.chromium.org/23477006/diff/25001/chrome/browser/resources/... chrome/browser/resources/google_now/utility_test_util.js:9: chrome['alarms'] = { On 2013/09/03 22:54:50, arv wrote: > why not chrome.alarms? This would cause presubmit errors. https://codereview.chromium.org/23477006/diff/25001/chrome/test/data/webui/te... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/23477006/diff/25001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:1674: * @param {string} expectedValue Expected value. On 2013/09/03 22:54:50, arv wrote: > useless description Done. https://codereview.chromium.org/23477006/diff/25001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:1685: * @param {Object} actualArgument The argument to match. On 2013/09/03 22:54:50, arv wrote: > {*} Done. https://codereview.chromium.org/23477006/diff/25001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:1689: return this.expectedValue_ === actualArgument.toString(); On 2013/09/03 22:54:50, arv wrote: > String(actualArgument) The rest of the file uses toString(). Unless I'm missing something toString() is equivalent to String(), so I'd prefer to leave it here for consistency.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/23477006/36001
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
https://codereview.chromium.org/23477006/diff/25001/chrome/test/data/webui/te... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/23477006/diff/25001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:1689: return this.expectedValue_ === actualArgument.toString(); On 2013/09/03 23:17:37, vadimt wrote: > On 2013/09/03 22:54:50, arv wrote: > > String(actualArgument) > > The rest of the file uses toString(). Unless I'm missing something toString() is > equivalent to String(), so I'd prefer to leave it here for consistency. They are not equivalent. value.toString() fails if value is null or undefined, but String(value) works for all values, and returns 'null' and 'undefined' as expected for null and undefined respectively.
https://codereview.chromium.org/23477006/diff/25001/chrome/test/data/webui/te... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/23477006/diff/25001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:1689: return this.expectedValue_ === actualArgument.toString(); On 2013/09/04 15:00:22, arv wrote: > On 2013/09/03 23:17:37, vadimt wrote: > > On 2013/09/03 22:54:50, arv wrote: > > > String(actualArgument) > > > > The rest of the file uses toString(). Unless I'm missing something toString() > is > > equivalent to String(), so I'd prefer to leave it here for consistency. > > They are not equivalent. > > value.toString() > > fails if value is null or undefined, but > > String(value) > > works for all values, and returns 'null' and 'undefined' as expected for null > and undefined respectively. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/23477006/58001
LGTM
Message was sent while issue was closed.
Change committed as 221239 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
