Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
642943002:
Add tests for sync custom wallpaper feature (Closed)
|
Created:
6 years, 2 months ago by Ran Modified:
6 years, 1 month ago CC:
chromium-reviews, extensions-reviews_chromium.org, nkostylev+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd tests for sync custom wallpaper feature
BUG=421864
Committed: https://crrev.com/41aa884211d2d8d2be9923e6a75fb5a40238cd5b
Cr-Commit-Position: refs/heads/master@{#302659}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add sync custom wallpaper tests in EventPageTest #
Total comments: 10
Patch Set 3 : Move mock functions to api_mock.js #
Total comments: 14
Patch Set 4 : Fix issues from 2nd code review #
Total comments: 10
Patch Set 5 : Fix issues from last code review #
Total comments: 10
Patch Set 6 : Fix issues #Patch Set 7 : Merge to resolve conflict #
Messages
Total messages: 23 (6 generated)
ranj@chromium.org changed reviewers: + bshe@chromium.org
Hi Biao, I created some tests for sync custom wallpaper feature following "event_page_unittest". Could you take a look to see if it's the right way to write test?
https://codereview.chromium.org/642943002/diff/1/chrome/test/data/chromeos/wa... File chrome/test/data/chromeos/wallpaper_manager/unit_tests/sync_custom_wallpaper_add_unittest.js (right): https://codereview.chromium.org/642943002/diff/1/chrome/test/data/chromeos/wa... chrome/test/data/chromeos/wallpaper_manager/unit_tests/sync_custom_wallpaper_add_unittest.js:24: function testSyncCustomAddWallpaper() { you can move this test to event_page_unittest.js so you dont need to duplicate some of the setup teardown code here. And you dont need to add a html page for every test. https://codereview.chromium.org/642943002/diff/1/chrome/test/data/chromeos/wa... chrome/test/data/chromeos/wallpaper_manager/unit_tests/sync_custom_wallpaper_add_unittest.js:32: var mockStoreWallpaperFromSyncFSToLocalFS = mockController.createFunctionMock( It would be more useful if you could mock chrome.* api instead of WallpaperUtil.* function. Eventually, we want to verify if chrome.* api (such as chrome.wallpaperPrivate.setCustomWallpaper, chrome.syncFileSystem etc.) were called with the correct parameters. https://codereview.chromium.org/642943002/diff/1/chrome/test/data/chromeos/wa... File chrome/test/data/chromeos/wallpaper_manager/unit_tests/sync_custom_wallpaper_delete_unittest.js (right): https://codereview.chromium.org/642943002/diff/1/chrome/test/data/chromeos/wa... chrome/test/data/chromeos/wallpaper_manager/unit_tests/sync_custom_wallpaper_delete_unittest.js:37: function testSyncCustomDeleteWallpaper() { Please try to consolidate this test to event_page.js if possible. https://codereview.chromium.org/642943002/diff/1/chrome/test/data/chromeos/wa... File chrome/test/data/chromeos/wallpaper_manager/unit_tests/sync_custom_wallpaper_set_unittest.js (right): https://codereview.chromium.org/642943002/diff/1/chrome/test/data/chromeos/wa... chrome/test/data/chromeos/wallpaper_manager/unit_tests/sync_custom_wallpaper_set_unittest.js:37: function testSyncCustomSetWallpaper() { ditto
bshe@chromium.org changed reviewers: + kevers@chromium.org
+kevers as reviewer too
Move tests into EventPageTest and add mock FS APIs
I haven't look into details but just some high level feedback to unblock you. https://codereview.chromium.org/642943002/diff/170001/chrome/test/data/chrome... File chrome/test/data/chromeos/wallpaper_manager/unit_tests/event_page_unittest.js (right): https://codereview.chromium.org/642943002/diff/170001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/event_page_unittest.js:13: var currentWallpaperName = 'currentwallpapername'; nit: all the variables are defined in global namespace. It would be better to move them into each individual test function if possible, given that the variable is only used in the test. If it is used by multiple tests, I would prefer to define them in api_mock.js or a test_util.js file, unless you have specific reason that we shouldnt do it. https://codereview.chromium.org/642943002/diff/170001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/event_page_unittest.js:103: chrome.storage.local.get = function(key, callback) { this overrides the globe stub implementation of chrome.storage.local.get in api_mock.js. I think it is better to directly change the stub implementation instead of override it here. https://codereview.chromium.org/642943002/diff/170001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/event_page_unittest.js:115: I think FileEntry and FileReader could move to api_mock.js as it is used a few times. https://codereview.chromium.org/642943002/diff/170001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/event_page_unittest.js:178: mockWrite.addExpectation(new Blob([new Int8Array(fileString)])); Is it possible to move this to previous test? This way we can reduce some code duplication. https://codereview.chromium.org/642943002/diff/170001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/event_page_unittest.js:183: // the local wallpaper info, wallpaper should set to the synced one. nit: update the comment.
Move mock functions to api_mock.js https://codereview.chromium.org/642943002/diff/170001/chrome/test/data/chrome... File chrome/test/data/chromeos/wallpaper_manager/unit_tests/event_page_unittest.js (right): https://codereview.chromium.org/642943002/diff/170001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/event_page_unittest.js:13: var currentWallpaperName = 'currentwallpapername'; On 2014/10/20 21:01:00, bshe wrote: > nit: all the variables are defined in global namespace. It would be better to > move them into each individual test function if possible, given that the > variable is only used in the test. If it is used by multiple tests, I would > prefer to define them in api_mock.js or a test_util.js file, unless you have > specific reason that we shouldnt do it. Done. https://codereview.chromium.org/642943002/diff/170001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/event_page_unittest.js:103: chrome.storage.local.get = function(key, callback) { On 2014/10/20 21:00:59, bshe wrote: > this overrides the globe stub implementation of chrome.storage.local.get in > api_mock.js. I think it is better to directly change the stub implementation > instead of override it here. Done. https://codereview.chromium.org/642943002/diff/170001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/event_page_unittest.js:115: On 2014/10/20 21:00:59, bshe wrote: > I think FileEntry and FileReader could move to api_mock.js as it is used a few > times. Done. https://codereview.chromium.org/642943002/diff/170001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/event_page_unittest.js:178: mockWrite.addExpectation(new Blob([new Int8Array(fileString)])); On 2014/10/20 21:00:59, bshe wrote: > Is it possible to move this to previous test? This way we can reduce some code > duplication. I think it might be clear to separate them since one test is for setting the custom wallpaper and another is storing the historical wallpaper. https://codereview.chromium.org/642943002/diff/170001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/event_page_unittest.js:183: // the local wallpaper info, wallpaper should set to the synced one. On 2014/10/20 21:00:59, bshe wrote: > nit: update the comment. Done.
Sorry for the delay. Some high level feedback for now. https://codereview.chromium.org/642943002/diff/190001/chrome/test/data/chrome... File chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js (right): https://codereview.chromium.org/642943002/diff/190001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js:36: if (isCreate.create == false) { nit: it is probably more robust to use: if (!isCreate.create) https://codereview.chromium.org/642943002/diff/190001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js:41: else { nit: move 'else' to previous line. The same for all the 'else' in this file. https://codereview.chromium.org/642943002/diff/190001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js:56: if (isCreate.create == false) { nit: if (!isCreate.create) https://codereview.chromium.org/642943002/diff/190001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js:65: else if (fileName.split('/').length == 2) { nit: move else if to previous line https://codereview.chromium.org/642943002/diff/190001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js:79: addFile: function(fileName) { nit: perhaps rename it to mockTestFile to be more clear? Same for syncFS? https://codereview.chromium.org/642943002/diff/190001/chrome/test/data/chrome... File chrome/test/data/chromeos/wallpaper_manager/unit_tests/event_page_unittest.js (right): https://codereview.chromium.org/642943002/diff/190001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/event_page_unittest.js:46: chrome.syncFileSystem.onFileStatusChanged.dispatch(syncFSChanges); I think it is probably easier to make "addFile" return a FileEntry. So you could do syncFSChanged.fileEntry = mockSyncFS.addFile('dummy'); https://codereview.chromium.org/642943002/diff/190001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/event_page_unittest.js:64: mockWrite.addExpectation(new Blob([new Int8Array(TestConstants.fileString)])); Blob probably dont have == operator. would it be possible to mock the blob with string?
Patchset #4 (id:210001) has been deleted
Fix issues from last code review https://codereview.chromium.org/642943002/diff/190001/chrome/test/data/chrome... File chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js (right): https://codereview.chromium.org/642943002/diff/190001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js:36: if (isCreate.create == false) { On 2014/10/27 20:14:51, bshe wrote: > nit: it is probably more robust to use: > if (!isCreate.create) Done. https://codereview.chromium.org/642943002/diff/190001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js:41: else { On 2014/10/27 20:14:51, bshe wrote: > nit: move 'else' to previous line. The same for all the 'else' in this file. Done. https://codereview.chromium.org/642943002/diff/190001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js:56: if (isCreate.create == false) { On 2014/10/27 20:14:51, bshe wrote: > nit: if (!isCreate.create) Done. https://codereview.chromium.org/642943002/diff/190001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js:65: else if (fileName.split('/').length == 2) { On 2014/10/27 20:14:51, bshe wrote: > nit: move else if to previous line Done. https://codereview.chromium.org/642943002/diff/190001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js:79: addFile: function(fileName) { On 2014/10/27 20:14:51, bshe wrote: > nit: perhaps rename it to mockTestFile to be more clear? Same for syncFS? Done. https://codereview.chromium.org/642943002/diff/190001/chrome/test/data/chrome... File chrome/test/data/chromeos/wallpaper_manager/unit_tests/event_page_unittest.js (right): https://codereview.chromium.org/642943002/diff/190001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/event_page_unittest.js:46: chrome.syncFileSystem.onFileStatusChanged.dispatch(syncFSChanges); On 2014/10/27 20:14:51, bshe wrote: > I think it is probably easier to make "addFile" return a FileEntry. So you could > do > syncFSChanged.fileEntry = mockSyncFS.addFile('dummy'); Done. https://codereview.chromium.org/642943002/diff/190001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/event_page_unittest.js:64: mockWrite.addExpectation(new Blob([new Int8Array(TestConstants.fileString)])); On 2014/10/27 20:14:51, bshe wrote: > Blob probably dont have == operator. would it be possible to mock the blob with > string? Done.
https://codereview.chromium.org/642943002/diff/230001/chrome/test/data/chrome... File chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js (right): https://codereview.chromium.org/642943002/diff/230001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js:37: if (failure) { nit: perhaps easier to read this way: if (!isCreate.create && failure) { failure('DIR_NOT_FOUND'); } else { ... } And the same for the rest in this file. https://codereview.chromium.org/642943002/diff/230001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js:71: console.error('Only support one-level fileSystem mock') nit: Only support one level deep subdirectory. https://codereview.chromium.org/642943002/diff/230001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js:76: var ret; nit: ret/mockFile https://codereview.chromium.org/642943002/diff/230001/chrome/test/data/chrome... File chrome/test/data/chromeos/wallpaper_manager/unit_tests/event_page_unittest.js (right): https://codereview.chromium.org/642943002/diff/230001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/event_page_unittest.js:83: Is it possible to add a test that a new custom wallpaper is indeed uploaded to sync server?
https://codereview.chromium.org/642943002/diff/230001/chrome/test/data/chrome... File chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js (right): https://codereview.chromium.org/642943002/diff/230001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js:37: if (failure) { On 2014/10/28 22:34:57, bshe wrote: > nit: perhaps easier to read this way: > if (!isCreate.create && failure) { > failure('DIR_NOT_FOUND'); > } else { > ... > } > > And the same for the rest in this file. I don't think they are equivalent. As if we pass in create=false and failure=null, the previous one will do nothing but the new code will create a directory https://codereview.chromium.org/642943002/diff/230001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js:71: console.error('Only support one-level fileSystem mock') On 2014/10/28 22:34:57, bshe wrote: > nit: Only support one level deep subdirectory. Done. https://codereview.chromium.org/642943002/diff/230001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js:76: var ret; On 2014/10/28 22:34:57, bshe wrote: > nit: ret/mockFile Done. https://codereview.chromium.org/642943002/diff/230001/chrome/test/data/chrome... File chrome/test/data/chromeos/wallpaper_manager/unit_tests/event_page_unittest.js (right): https://codereview.chromium.org/642943002/diff/230001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/event_page_unittest.js:83: On 2014/10/28 22:34:57, bshe wrote: > Is it possible to add a test that a new custom wallpaper is indeed uploaded to > sync server? Not possible for now as the FS operations takes two callbacks success and failure. Should I can update the test framework to support two callbacks?
lgtm please wait for kevin's review before you submit. https://codereview.chromium.org/642943002/diff/230001/chrome/test/data/chrome... File chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js (right): https://codereview.chromium.org/642943002/diff/230001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js:37: if (failure) { On 2014/10/29 19:13:13, Ran wrote: > On 2014/10/28 22:34:57, bshe wrote: > > nit: perhaps easier to read this way: > > if (!isCreate.create && failure) { > > failure('DIR_NOT_FOUND'); > > } else { > > ... > > } > > > > And the same for the rest in this file. > > I don't think they are equivalent. As if we pass in create=false and > failure=null, the previous one will do nothing but the new code will create a > directory dooh. My mistake. you are right. https://codereview.chromium.org/642943002/diff/230001/chrome/test/data/chrome... File chrome/test/data/chromeos/wallpaper_manager/unit_tests/event_page_unittest.js (right): https://codereview.chromium.org/642943002/diff/230001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/event_page_unittest.js:83: On 2014/10/29 19:13:13, Ran wrote: > On 2014/10/28 22:34:57, bshe wrote: > > Is it possible to add a test that a new custom wallpaper is indeed uploaded to > > sync server? > > Not possible for now as the FS operations takes two callbacks success and > failure. Should I can update the test framework to support two callbacks? > never mind. you can do it in a separate CL.
https://codereview.chromium.org/642943002/diff/250001/chrome/test/data/chrome... File chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js (right): https://codereview.chromium.org/642943002/diff/250001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js:9: image: '*#*@#&', Constants should be in all caps. var TestContants { IMAGE: ... FILE_STRING: ... }; https://codereview.chromium.org/642943002/diff/250001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js:14: function FileReader() { Please add jsdoc for all methods that are not API overrides. Refer to https://developers.google.com/closure/compiler/docs/js-for-compiler for guidelines. https://codereview.chromium.org/642943002/diff/250001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js:37: if (failure) { No need for brackets on single line if. https://codereview.chromium.org/642943002/diff/250001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js:112: if (failure) { Remove brackets on single line if. https://codereview.chromium.org/642943002/diff/250001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js:129: if (typeof data == 'string') { Remove brackets on single line if-else.
Patchset #6 (id:270001) has been deleted
https://codereview.chromium.org/642943002/diff/250001/chrome/test/data/chrome... File chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js (right): https://codereview.chromium.org/642943002/diff/250001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js:9: image: '*#*@#&', On 2014/10/30 14:17:36, kevers wrote: > Constants should be in all caps. > > var TestContants { > IMAGE: ... > FILE_STRING: ... > }; Done. https://codereview.chromium.org/642943002/diff/250001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js:14: function FileReader() { On 2014/10/30 14:17:36, kevers wrote: > Please add jsdoc for all methods that are not API overrides. Refer to > https://developers.google.com/closure/compiler/docs/js-for-compiler for > guidelines. Done. https://codereview.chromium.org/642943002/diff/250001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js:37: if (failure) { On 2014/10/30 14:17:36, kevers wrote: > No need for brackets on single line if. Done. https://codereview.chromium.org/642943002/diff/250001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js:112: if (failure) { On 2014/10/30 14:17:36, kevers wrote: > Remove brackets on single line if. Done. https://codereview.chromium.org/642943002/diff/250001/chrome/test/data/chrome... chrome/test/data/chromeos/wallpaper_manager/unit_tests/api_mock.js:129: if (typeof data == 'string') { On 2014/10/30 14:17:36, kevers wrote: > Remove brackets on single line if-else. Done.
Sorry for the delay. LGTM.
Patchset #7 (id:310001) has been deleted
The CQ bit was checked by ranj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/642943002/330001
Message was sent while issue was closed.
Committed patchset #7 (id:330001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/41aa884211d2d8d2be9923e6a75fb5a40238cd5b Cr-Commit-Position: refs/heads/master@{#302659} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
