Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
17125003:
First unit test for Google Now extension (Closed)
|
Created:
7 years, 6 months ago by vadimt Modified:
7 years, 5 months ago CC:
chromium-reviews, arv+watch_chromium.org, Jeffrey Yasskin, Sheridan Rawlins, aboxhall, rgustafson, skare_ Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFirst unit test for Google Now extension.
The test is based upon gtestjs technology.
The test check only one function in one script of Google Now extension. Other tests will follow.
The test doesn't verify (and doesn't want to verify) how the extension' js code behaves in the context of a real extension. It's just a unit test that checks function outputs based on inputs, and future tests will also mock callees and check how they are called.
The main goal of this CL is to get feedback from the reviewers regarding the general direction chosen for unit-testing scripts in component extensions.
BUG=164227
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209016
Patch Set 1 #
Total comments: 2
Patch Set 2 : jyasskin@ comment #
Total comments: 15
Patch Set 3 : arv@'s comments #Patch Set 4 : More arv@ notes #Patch Set 5 : Modifying .isolate dependencies #
Messages
Total messages: 24 (0 generated)
Dear CC line reviewers! Please take a look, and comment if needed. If there are no answers, I'll wait some time, and send this to OWNERs.
I'm very happy with this approach. I don't know what the conventions are for putting files in the resources directory or arranging .gtestjs tests, but the rest lgtm. https://codereview.chromium.org/17125003/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/unit_test/background.gtestjs (right): https://codereview.chromium.org/17125003/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/unit_test/background.gtestjs:30: testTaskPair(UPDATE_CARDS_TASK_NAME, UPDATE_CARDS_TASK_NAME, true); These should be indented 2 spaces, not 1, right?
https://codereview.chromium.org/17125003/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/unit_test/background.gtestjs (right): https://codereview.chromium.org/17125003/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/unit_test/background.gtestjs:30: testTaskPair(UPDATE_CARDS_TASK_NAME, UPDATE_CARDS_TASK_NAME, true); On 2013/06/16 21:36:50, Jeffrey Yasskin wrote: > These should be indented 2 spaces, not 1, right? Done.
+rgustafson, skare I've decided to add you to CC, so no obligation to review, but still feel free to add comments. IF YOU WISH :-|
OWNERs, please provide approvals: sky@: chrome/chrome_tests_unit.gypi arv@: the rest
LGTM
https://codereview.chromium.org/17125003/diff/5001/chrome/browser/resources/g... File chrome/browser/resources/google_now/unit_test/background_globals.js (right): https://codereview.chromium.org/17125003/diff/5001/chrome/browser/resources/g... chrome/browser/resources/google_now/unit_test/background_globals.js:7: function EMPTY() {} WHY ALL CAPS? https://codereview.chromium.org/17125003/diff/5001/chrome/browser/resources/g... chrome/browser/resources/google_now/unit_test/background_globals.js:7: function EMPTY() {} Do we need this to be a global? https://codereview.chromium.org/17125003/diff/5001/chrome/browser/resources/g... chrome/browser/resources/google_now/unit_test/background_globals.js:8: function buildTaskManager() { return {instrumentApiFunction: EMPTY}; } line break after { https://codereview.chromium.org/17125003/diff/5001/chrome/browser/resources/g... chrome/browser/resources/google_now/unit_test/background_globals.js:12: chrome['location'] = {onLocationUpdate: EMPTY_LISTENER}; Why not chrome.location? https://codereview.chromium.org/17125003/diff/5001/chrome/browser/resources/g... chrome/browser/resources/google_now/unit_test/background_globals.js:23: storage = {}; missing var? https://codereview.chromium.org/17125003/diff/5001/chrome/chrome_tests_unit.gypi File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/17125003/diff/5001/chrome/chrome_tests_unit.g... chrome/chrome_tests_unit.gypi:1073: 'browser/resources/google_now/unit_test/background.gtestjs', No other tests uses a unit_test dir. How about 'browser/resources/google_now/background_unittest.gtestjs',
https://codereview.chromium.org/17125003/diff/5001/chrome/browser/resources/g... File chrome/browser/resources/google_now/unit_test/background_globals.js (right): https://codereview.chromium.org/17125003/diff/5001/chrome/browser/resources/g... chrome/browser/resources/google_now/unit_test/background_globals.js:7: function EMPTY() {} On 2013/06/18 17:47:10, arv wrote: > WHY ALL CAPS? DONE https://codereview.chromium.org/17125003/diff/5001/chrome/browser/resources/g... chrome/browser/resources/google_now/unit_test/background_globals.js:7: function EMPTY() {} On 2013/06/18 17:47:10, arv wrote: > Do we need this to be a global? I assume you are asking about emptyMock only. buildTaskManager() should be global, since background.js depends on it. buildTaskManager() uses emptyMock, so it's convenient to make emptyMock global too. But yes, emptyMock is not used directly by background.js https://codereview.chromium.org/17125003/diff/5001/chrome/browser/resources/g... chrome/browser/resources/google_now/unit_test/background_globals.js:8: function buildTaskManager() { return {instrumentApiFunction: EMPTY}; } On 2013/06/18 17:47:10, arv wrote: > line break after { Done. https://codereview.chromium.org/17125003/diff/5001/chrome/browser/resources/g... chrome/browser/resources/google_now/unit_test/background_globals.js:12: chrome['location'] = {onLocationUpdate: EMPTY_LISTENER}; On 2013/06/18 17:47:10, arv wrote: > Why not chrome.location? This was causing presubmit check failures: line 14: E0220: No docs found for member 'chrome.location' https://codereview.chromium.org/17125003/diff/5001/chrome/browser/resources/g... chrome/browser/resources/google_now/unit_test/background_globals.js:23: storage = {}; On 2013/06/18 17:47:10, arv wrote: > missing var? Done. https://codereview.chromium.org/17125003/diff/5001/chrome/chrome_tests_unit.gypi File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/17125003/diff/5001/chrome/chrome_tests_unit.g... chrome/chrome_tests_unit.gypi:1073: 'browser/resources/google_now/unit_test/background.gtestjs', On 2013/06/18 17:47:10, arv wrote: > No other tests uses a unit_test dir. How about > > 'browser/resources/google_now/background_unittest.gtestjs', Here, we have choice between uniformity (and benefits of this uniformity are not so obvious, while it's messy to keep everything in one directory) and clarity (with a separate directory for tests). In my eyes, the benefits of clarity overweight. But if you insist, I'll put everything in one folder.
https://codereview.chromium.org/17125003/diff/5001/chrome/chrome_tests_unit.gypi File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/17125003/diff/5001/chrome/chrome_tests_unit.g... chrome/chrome_tests_unit.gypi:1073: 'browser/resources/google_now/unit_test/background.gtestjs', On 2013/06/18 23:12:25, vadimt wrote: > On 2013/06/18 17:47:10, arv wrote: > > No other tests uses a unit_test dir. How about > > > > 'browser/resources/google_now/background_unittest.gtestjs', > > Here, we have choice between uniformity (and benefits of this uniformity are not > so obvious, while it's messy to keep everything in one directory) and clarity > (with a separate directory for tests). In my eyes, the benefits of clarity > overweight. > But if you insist, I'll put everything in one folder. All other chrome tests go next to the file they are testing. https://codereview.chromium.org/17125003/diff/5001/chrome/chrome_tests_unit.g... chrome/chrome_tests_unit.gypi:1074: 'browser/resources/google_now/unit_test/background_globals.js', background_test_util.js maybe?
https://codereview.chromium.org/17125003/diff/5001/chrome/chrome_tests_unit.gypi File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/17125003/diff/5001/chrome/chrome_tests_unit.g... chrome/chrome_tests_unit.gypi:1074: 'browser/resources/google_now/unit_test/background_globals.js', On 2013/06/19 13:31:01, arv wrote: > background_test_util.js maybe? All done.
On Wed, Jun 19, 2013 at 6:31 AM, <arv@chromium.org> wrote: > > https://codereview.chromium.org/17125003/diff/5001/chrome/chrome_tests_unit.gypi > File chrome/chrome_tests_unit.gypi (right): > > https://codereview.chromium.org/17125003/diff/5001/chrome/chrome_tests_unit.g... > chrome/chrome_tests_unit.gypi:1073: > 'browser/resources/google_now/unit_test/background.gtestjs', > On 2013/06/18 23:12:25, vadimt wrote: >> >> On 2013/06/18 17:47:10, arv wrote: >> > No other tests uses a unit_test dir. How about >> > >> > 'browser/resources/google_now/background_unittest.gtestjs', > > >> Here, we have choice between uniformity (and benefits of this > > uniformity are not >> >> so obvious, while it's messy to keep everything in one directory) and > > clarity >> >> (with a separate directory for tests). In my eyes, the benefits of > > clarity >> >> overweight. >> But if you insist, I'll put everything in one folder. > > > All other chrome tests go next to the file they are testing. Except for a pile of extension tests, e.g. https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/test/data/.... These are awful, but they exist that way because a single test needs several helper files, and it's confusing to put all of those test helper files next to implementation code. (And some of the helper files need the same name across multiple tests.) > https://codereview.chromium.org/17125003/diff/5001/chrome/chrome_tests_unit.g... > chrome/chrome_tests_unit.gypi:1074: > 'browser/resources/google_now/unit_test/background_globals.js', > background_test_util.js maybe? > > https://codereview.chromium.org/17125003/
LGTM
On 2013/06/19 18:17:30, Jeffrey Yasskin wrote: > On Wed, Jun 19, 2013 at 6:31 AM, <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=arv@chromium.org> wrote: > > > > > https://codereview.chromium.org/17125003/diff/5001/chrome/chrome_tests_unit.gypi > > File chrome/chrome_tests_unit.gypi (right): > > > > > https://codereview.chromium.org/17125003/diff/5001/chrome/chrome_tests_unit.g... > > chrome/chrome_tests_unit.gypi:1073: > > 'browser/resources/google_now/unit_test/background.gtestjs', > > On 2013/06/18 23:12:25, vadimt wrote: > >> > >> On 2013/06/18 17:47:10, arv wrote: > >> > No other tests uses a unit_test dir. How about > >> > > >> > 'browser/resources/google_now/background_unittest.gtestjs', > > > > > >> Here, we have choice between uniformity (and benefits of this > > > > uniformity are not > >> > >> so obvious, while it's messy to keep everything in one directory) and > > > > clarity > >> > >> (with a separate directory for tests). In my eyes, the benefits of > > > > clarity > >> > >> overweight. > >> But if you insist, I'll put everything in one folder. > > > > > > All other chrome tests go next to the file they are testing. > > Except for a pile of extension tests, e.g. > https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/test/data/.... > These are awful, but they exist that way because a single test needs > several helper files, and it's confusing to put all of those test > helper files next to implementation code. (And some of the helper > files need the same name across multiple tests.) > > > > https://codereview.chromium.org/17125003/diff/5001/chrome/chrome_tests_unit.g... > > chrome/chrome_tests_unit.gypi:1074: > > 'browser/resources/google_now/unit_test/background_globals.js', > > background_test_util.js maybe? > > > > https://codereview.chromium.org/17125003/ Ack
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/17125003/23001
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/17125003/23001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/17125003/23001
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/17125003/57001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/17125003/57001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/17125003/57001
Message was sent while issue was closed.
Change committed as 209016 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
