Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
5151418134560768:
Adding first unit tests with mocks (Closed)
|
Created:
7 years, 5 months ago by vadimt Modified:
7 years, 5 months ago CC:
chromium-reviews, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdding first unit tests to Google Now extension with mocks for global functions, chrome APIs and local callbacks.
test_api.js framework was extended to support mocking non-global functions and APIs with dot-separated names.
BUG=164227
TEST=No
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=211514
Patch Set 1 #
Total comments: 12
Patch Set 2 : arv@ notes #
Total comments: 22
Patch Set 3 : More arv@ notes #
Total comments: 2
Patch Set 4 : More arv@ notes #
Messages
Total messages: 18 (0 generated)
sky@, please provide OWNER's approval for chrome/test/data/webui/test_api.js arv@, please provide OWNER's approval for chrome/browser/resources/google_now/*
https://codereview.chromium.org/5151418134560768/diff/5629499534213120/chrome... File chrome/browser/resources/google_now/background_test_util.js (right): https://codereview.chromium.org/5151418134560768/diff/5629499534213120/chrome... chrome/browser/resources/google_now/background_test_util.js:53: this._expectedValue = expectedValue; Google3 style is to use trailing underscore. https://codereview.chromium.org/5151418134560768/diff/5629499534213120/chrome... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/5151418134560768/diff/5629499534213120/chrome... chrome/browser/resources/google_now/background_unittest.gtestjs:72: var storage_get_SavedArgs = new SaveMockArguments(); no underscores https://codereview.chromium.org/5151418134560768/diff/5629499534213120/chrome... chrome/browser/resources/google_now/background_unittest.gtestjs:74: storage_get( no underscores https://codereview.chromium.org/5151418134560768/diff/5629499534213120/chrome... chrome/browser/resources/google_now/background_unittest.gtestjs:110: chrome_identity_getAuthToken_SavedArgs.match( incorrect indentation https://codereview.chromium.org/5151418134560768/diff/5629499534213120/chrome... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/5151418134560768/diff/5629499534213120/chrome... chrome/test/data/webui/test_api.js:590: mockFunctions._functions = {}; functions_
https://codereview.chromium.org/5151418134560768/diff/5629499534213120/chrome... File chrome/browser/resources/google_now/background_test_util.js (right): https://codereview.chromium.org/5151418134560768/diff/5629499534213120/chrome... chrome/browser/resources/google_now/background_test_util.js:53: this._expectedValue = expectedValue; On 2013/07/11 20:59:44, arv wrote: > Google3 style is to use trailing underscore. Done. https://codereview.chromium.org/5151418134560768/diff/5629499534213120/chrome... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/5151418134560768/diff/5629499534213120/chrome... chrome/browser/resources/google_now/background_unittest.gtestjs:72: var storage_get_SavedArgs = new SaveMockArguments(); On 2013/07/11 20:59:44, arv wrote: > no underscores I believe this is a special case. I mock an API call 'storage.get'. The mock function for it will be storage_get. Saved mock arguments for it will be, correspondingly, storage_get_SavedArgs. https://codereview.chromium.org/5151418134560768/diff/5629499534213120/chrome... chrome/browser/resources/google_now/background_unittest.gtestjs:74: storage_get( On 2013/07/11 20:59:44, arv wrote: > no underscores Same story. storage_get is a name automatically generated from 'storage.get'. I believe, generating underscores is better than generating camel-cased names, for example, because parts of chrome API names can be camel-cased themselves. https://codereview.chromium.org/5151418134560768/diff/5629499534213120/chrome... chrome/browser/resources/google_now/background_unittest.gtestjs:110: chrome_identity_getAuthToken_SavedArgs.match( On 2013/07/11 20:59:44, arv wrote: > incorrect indentation Done. https://codereview.chromium.org/5151418134560768/diff/5629499534213120/chrome... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/5151418134560768/diff/5629499534213120/chrome... chrome/test/data/webui/test_api.js:590: mockFunctions._functions = {}; On 2013/07/11 20:59:44, arv wrote: > functions_ Done.
https://codereview.chromium.org/5151418134560768/diff/5629499534213120/chrome... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/5151418134560768/diff/5629499534213120/chrome... chrome/browser/resources/google_now/background_unittest.gtestjs:72: var storage_get_SavedArgs = new SaveMockArguments(); On 2013/07/11 21:28:10, vadimt wrote: > On 2013/07/11 20:59:44, arv wrote: > > no underscores > > I believe this is a special case. I mock an API call 'storage.get'. The mock > function for it will be storage_get. Saved mock arguments for it will be, > correspondingly, storage_get_SavedArgs. I don't think I understand. How can the api depend on the name of a local variable?
https://codereview.chromium.org/5151418134560768/diff/5629499534213120/chrome... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/5151418134560768/diff/5629499534213120/chrome... chrome/browser/resources/google_now/background_unittest.gtestjs:72: var storage_get_SavedArgs = new SaveMockArguments(); On 2013/07/11 21:56:04, arv wrote: > On 2013/07/11 21:28:10, vadimt wrote: > > On 2013/07/11 20:59:44, arv wrote: > > > no underscores > > > > I believe this is a special case. I mock an API call 'storage.get'. The mock > > function for it will be storage_get. Saved mock arguments for it will be, > > correspondingly, storage_get_SavedArgs. > > I don't think I understand. How can the api depend on the name of a local > variable? Expanded story. This is a test for initialize() method. initialize() calls storage.get(). storage.get is an API provided by utility.js (see "var storage = chrome.storage.local;" there). Since utility.js is not included in this test, we have to mock storage.get API. We mock it in mockInitializeDependencies method (see makeAndRegisterMockApis call). makeAndRegisterMockApis creates this call in the global namespace under its real name 'storage.get'. This is a mocked method that will be called by the tested function. At the same time, it adds it to the mock class for API under the name 'storage_get'. You can see how it's used below to set expectations for this call: this.mockApis.expects(once()). storage_get(.... storage_get is a name generated from 'storage.get'. This is a pseudo-method to set expectations, not to call from the tested function. Now, we want to know values of parameters that the tested method passes to this API call. For this, I have storage_get_SavedArgs. Hopefully, this explains it. Please let me know if I didn't answer the question.
sky@ if OOF jcivelli@, please provide OWNER's approval for chrome/test/data/webui/test_api.js
On 2013/07/11 22:23:28, vadimt wrote: > sky@ if OOF > jcivelli@, please provide OWNER's approval for > chrome/test/data/webui/test_api.js I am not skilled at the JavaScript. Any chance you can find someone who knows JS to review this file first? Jay
arv@ could you also review chrome/test/data/webui/test_api.js? jcivelli@, I assume that after arv@ looks at the style, you are going to look at the logic, and provide you comments/LGTM. Please let me know if my assumption is wrong.
On 2013/07/11 22:18:24, vadimt wrote: > https://codereview.chromium.org/5151418134560768/diff/5629499534213120/chrome... > File chrome/browser/resources/google_now/background_unittest.gtestjs (right): > > https://codereview.chromium.org/5151418134560768/diff/5629499534213120/chrome... > chrome/browser/resources/google_now/background_unittest.gtestjs:72: var > storage_get_SavedArgs = new SaveMockArguments(); > On 2013/07/11 21:56:04, arv wrote: > > On 2013/07/11 21:28:10, vadimt wrote: > > > On 2013/07/11 20:59:44, arv wrote: > > > > no underscores > > > > > > I believe this is a special case. I mock an API call 'storage.get'. The mock > > > function for it will be storage_get. Saved mock arguments for it will be, > > > correspondingly, storage_get_SavedArgs. > > > > I don't think I understand. How can the api depend on the name of a local > > variable? > > Expanded story. > This is a test for initialize() method. > initialize() calls storage.get(). storage.get is an API provided by utility.js > (see "var storage = chrome.storage.local;" there). > Since utility.js is not included in this test, we have to mock storage.get API. > We mock it in mockInitializeDependencies method (see makeAndRegisterMockApis > call). > makeAndRegisterMockApis creates this call in the global namespace under its real > name 'storage.get'. This is a mocked method that will be called by the tested > function. > At the same time, it adds it to the mock class for API under the name > 'storage_get'. You can see how it's used below to set expectations for this > call: Why doesn't it mock out global.storage.get? If you call it global.storage_get then internal code that calls global.storage.get will not tricker your mock and your test cannot test that the arguments are what you expected. > this.mockApis.expects(once()). > storage_get(.... > > storage_get is a name generated from 'storage.get'. This is a pseudo-method to > set expectations, not to call from the tested function. > > Now, we want to know values of parameters that the tested method passes to this > API call. For this, I have storage_get_SavedArgs. > > Hopefully, this explains it. Please let me know if I didn't answer the question.
https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... File chrome/browser/resources/google_now/background_test_util.js (right): https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... chrome/browser/resources/google_now/background_test_util.js:64: return ( Or return JSON.stringify(this.expectedValue_) === JSON.stringify(actualArgument); https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... chrome/browser/resources/google_now/background_unittest.gtestjs:72: var storage_get_SavedArgs = new SaveMockArguments(); This still makes no sense. storage_get_SavedArgs is a local variable. Are you saying that there is some preprocessor involved? https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... chrome/browser/resources/google_now/background_unittest.gtestjs:74: storage_get( this one I understand why it is using storage_get. I think it is a bad design but I think it is out of scope for this CL. https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... chrome/test/data/webui/test_api.js:209: var apiMockNames = []; Maybe: var apiMockNames = apiNames.map(function(name) { return name.replace(/\./g, '_'); }); https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... chrome/test/data/webui/test_api.js:211: apiMockNames.push(apiNames [i].replace(/\./g, '_')); Another option is to keep the '.' in the name. Then the caller would look like: this.mockApis.expects(once())['chrome.tabs.create'](...) https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... chrome/test/data/webui/test_api.js:222: * @return {Mock} Mock handler class. @param {!Array.<string>} functionNames https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... chrome/test/data/webui/test_api.js:547: for(var i = 0; i < path.length-1; ++i) { missing whitespaces around bin op https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... chrome/test/data/webui/test_api.js:547: for(var i = 0; i < path.length-1; ++i) { i++ https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... chrome/test/data/webui/test_api.js:552: namespace = namespace[fieldName]; Once again I'm confused. It seems like you are building the right object structure here. So, why don't we allow calling things using: this.mockApis.expects(once()).chrome.tabs.create(...) https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... chrome/test/data/webui/test_api.js:593: if (typeof MockClass.prototype[func] === 'function') What about accessors? Maybe add a todo? https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... chrome/test/data/webui/test_api.js:598: return this.functions_; Is this the correct this? If so, maybe just rename all functions_ to function and you remove this statement.
arv@, thanks for also reviewing test_api.js! >> Why doesn't it mock out global.storage.get?... (I assume that by ‘global’ you mean a pseudo-identifier for global namespace) In fact, I mock it exactly as you describe it: as global.storage.get. There are no problems here. (But I also have to create corresponding a pseudo-method that is used to set expectations. I need to find a name for this method. I prefer names that are valid JS identifiers. So I’ve just replaced dots with underscores.) https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... File chrome/browser/resources/google_now/background_test_util.js (right): https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... chrome/browser/resources/google_now/background_test_util.js:64: return ( On 2013/07/12 14:18:34, arv wrote: > Or > > return JSON.stringify(this.expectedValue_) === > JSON.stringify(actualArgument); Done. https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... chrome/browser/resources/google_now/background_unittest.gtestjs:72: var storage_get_SavedArgs = new SaveMockArguments(); On 2013/07/12 14:18:34, arv wrote: > This still makes no sense. storage_get_SavedArgs is a local variable. > > Are you saying that there is some preprocessor involved? No preprocessing. The reason is that I want this variable name to match the call that it's getting arguments for. The call itself is automatically generated and includes underscores. I could camel-case it as storageGetSavedArgs, but then it would be harder to read the code. Also, consider a call chrome.identity.getAuthToken. If we call the corresponding saved args 'chromeIdentityGetAuthTokenSavedArgs', all parts will lump together, and it will be hard to comprehend the code. Better it will be chrome_identity_getAuthToken_SavedArgs. https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... chrome/browser/resources/google_now/background_unittest.gtestjs:74: storage_get( On 2013/07/12 14:18:34, arv wrote: > this one I understand why it is using storage_get. I think it is a bad design > but I think it is out of scope for this CL. Alternatively, it could be this.mockApis.expects(once()) ['storage.get']( but this is harder to write. Also, in this case, how to call corresponding saved args? We cannot include dots there. Another alternative in theory could be: this.mockApis.expects(once()). storage.get( But unfortunately, the mocking itself is done by a 3rd party library mock4js.js. (test_api.js is build as a sugar on top of it). It generates only a flat object with a list of same-level methods. I could probably hack around it, but I feel that this would create more problems than solve. https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... chrome/test/data/webui/test_api.js:209: var apiMockNames = []; On 2013/07/12 14:18:34, arv wrote: > Maybe: > > var apiMockNames = apiNames.map(function(name) { > return name.replace(/\./g, '_'); > }); Cool https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... chrome/test/data/webui/test_api.js:211: apiMockNames.push(apiNames [i].replace(/\./g, '_')); On 2013/07/12 14:18:34, arv wrote: > Another option is to keep the '.' in the name. Then the caller would look like: > > this.mockApis.expects(once())['chrome.tabs.create'](...) I considered this. I feel that the using code would become somewhat less clear, mostly because I cannot include dots in names of saved args. So, the using code would suffer from inconsistency between expectation method names and saved args names. https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... chrome/test/data/webui/test_api.js:222: * @return {Mock} Mock handler class. On 2013/07/12 14:18:34, arv wrote: > @param {!Array.<string>} functionNames Done. https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... chrome/test/data/webui/test_api.js:547: for(var i = 0; i < path.length-1; ++i) { On 2013/07/12 14:18:34, arv wrote: > missing whitespaces around bin op Done. https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... chrome/test/data/webui/test_api.js:547: for(var i = 0; i < path.length-1; ++i) { On 2013/07/12 14:18:34, arv wrote: > missing whitespaces around bin op Done. https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... chrome/test/data/webui/test_api.js:552: namespace = namespace[fieldName]; On 2013/07/12 14:18:34, arv wrote: > Once again I'm confused. It seems like you are building the right object > structure here. So, why don't we allow calling things using: > > this.mockApis.expects(once()).chrome.tabs.create(...) See above about external mock4js that generates the flat list of methods. I have no control over it, therefore, it remains flat. However, I have freedom to take these methods and create a hierarchical structure of global overrides based on them. https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... chrome/test/data/webui/test_api.js:593: if (typeof MockClass.prototype[func] === 'function') On 2013/07/12 14:18:34, arv wrote: > What about accessors? Maybe add a todo? This method is used to generate: 1. Local functions that are used as (callback etc.) params to the tested functions. 2. chrome APIs Both are unlikely to be getters. So, probably, we'll never need them. But I guess whoever will need them first, will be able to add them here. So, I'd not create a TODO item, if you don't mind. https://codereview.chromium.org/5151418134560768/diff/5865619656278016/chrome... chrome/test/data/webui/test_api.js:598: return this.functions_; On 2013/07/12 14:18:34, arv wrote: > Is this the correct this? If so, maybe just rename all functions_ to function > and you remove this statement. Yes, this is a correct 'this': the one of mockFunctions. I've introduced a method to match another method: proxy() which returns the generated proxied class.
https://codereview.chromium.org/5151418134560768/diff/18001/chrome/browser/re... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/5151418134560768/diff/18001/chrome/browser/re... chrome/browser/resources/google_now/background_unittest.gtestjs:72: var storage_get_SavedArgs = new SaveMockArguments(); please fix local variable names at least.
https://codereview.chromium.org/5151418134560768/diff/18001/chrome/browser/re... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/5151418134560768/diff/18001/chrome/browser/re... chrome/browser/resources/google_now/background_unittest.gtestjs:72: var storage_get_SavedArgs = new SaveMockArguments(); On 2013/07/12 18:36:52, arv wrote: > please fix local variable names at least. Done.
LGTM
Yey!!! Jay, could you review your part now?
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/5151418134560768/2...
Message was sent while issue was closed.
Change committed as 211514 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
