Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
320753002:
Support javascript gtests in an extension background page. (Closed)
|
Created:
6 years, 6 months ago by David Tseng Modified:
6 years, 6 months ago CC:
chromium-reviews, oshima+watch_chromium.org, plundblad+watch_chromium.org, aboxhall+watch_chromium.org, arv+watch_chromium.org, yuzo+watch_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSupport javascript gtests in an extension background page.
- abstracts away the minimal amount of logic and data from WebUIBrowserTest to form JavaScriptBrowserTest.
- JavaScriptBrowserTest knows how to manage js libraries, build runnable javascript, setup paths.
- subclasses of JavaScriptBrowserTest are still responsible for js injection and listening to test result IPC's.
- adds a subclass that knows how to inject test assets into an extension and relay pass/fail from the extension's background page as well as run tests in that context. (ExtensionJSBrowserTest).
- ExtensionJSBrowserTest uses DOMAutomation's IPC's (which are available in an extension context); chrome.send is not.
- adds ExtensionLoadWaiter as a helper class to load and wait for an extension.
- eventually, should unify the way in which subclasses communicate to their test js resources:
-- js-end bindings (chrome.send vs window.domAutomation.send)
-- IPC messaging (WebUI specific handler system vs dom automation messages).
-- clean up the hand rolled js string "building".
-- make the subclass provide a getter to the WebContents that hosts its js under test.
These items are not within the scope of this cl.
- obtains friendly error messages from the extension's context while running tests via a json object passed back to the runner ExtensionJSBrowserTest; failures are printed in gtest.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277249
Patch Set 1 #Patch Set 2 : WS #
Total comments: 42
Patch Set 3 : Address comments; rework a fair amount. #Patch Set 4 : Fixup comment #
Total comments: 86
Patch Set 5 : Various improvements; address nits. #Patch Set 6 : More improvements #Patch Set 7 : add maybeGenHeader in call to GEN. #Patch Set 8 : Update comment. #Patch Set 9 : Lint #Patch Set 10 : lint #
Total comments: 4
Patch Set 11 : EXPECT_TRUE and TODO. #Messages
Total messages: 18 (0 generated)
Took a long time to figure out this one, but it cleaned up to be ok I think. First cut, please have a look.
Hi, Exciting step in the right direction I think. I am wondering if we shoud leave the chromevox test base class a little more generic allowing the javascript test fixture to specify what extension to run in to ease reuse. We could then have a chromevox fixture to base all our tests on A few suggestions/nits below. https://codereview.chromium.org/320753002/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/chromevox/chromevox_browsertest.h (right): https://codereview.chromium.org/320753002/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox_browsertest.h:29: // Implicit overrides assumed by js2gtest. I don't know what an implicit override is. Can you jsut say that this method is used by js2gtest to ... etc. https://codereview.chromium.org/320753002/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox_browsertest.h:39: DISALLOW_COPY_AND_ASSIGN(ChromeVoxBrowserTest); nit: blank line before this one. https://codereview.chromium.org/320753002/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi (right): https://codereview.chromium.org/320753002/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi:76: # isolate. nit: extra space. https://codereview.chromium.org/320753002/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi:78: 'extension': 'chromevoxjstest', cvxojstest? Shorter and this rules is local to our target, so there shouldn't be any risk for conflict. https://codereview.chromium.org/320753002/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi:86: '<(chromevox_test_deps_js_file)', Do we need the deps file for these tests? Shouldn't they call code already loaded as part of the extension? https://codereview.chromium.org/320753002/diff/20001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/320753002/diff/20001/chrome/chrome_tests.gypi... chrome/chrome_tests.gypi:1544: 'test/base/javascript_browsertest.cc', Should be called javascript_browser_test.cc for consistency since it is a test framework class and not a test. https://codereview.chromium.org/320753002/diff/20001/chrome/test/base/javascr... File chrome/test/base/javascript_browsertest.cc (right): https://codereview.chromium.org/320753002/diff/20001/chrome/test/base/javascr... chrome/test/base/javascript_browsertest.cc:14: const base::FilePath::CharType Shouldn't we keep this in the webui tests? We should never run the accessibility audit on a chromevox test I think. https://codereview.chromium.org/320753002/diff/20001/chrome/test/base/javascr... File chrome/test/base/javascript_browsertest.h (right): https://codereview.chromium.org/320753002/diff/20001/chrome/test/base/javascr... chrome/test/base/javascript_browsertest.h:13: class JavaScriptBrowserTest : public InProcessBrowserTest { Documentation? https://codereview.chromium.org/320753002/diff/20001/chrome/test/base/javascr... chrome/test/base/javascript_browsertest.h:18: static const base::FilePath::CharType kWebUITestFolder[]; Are these statics used by anything or could they be left as file-private? https://codereview.chromium.org/320753002/diff/20001/chrome/test/base/javascr... chrome/test/base/javascript_browsertest.h:30: void SetUpOnMainThread(); OVERRIDE https://codereview.chromium.org/320753002/diff/20001/chrome/test/base/web_ui_... File chrome/test/base/web_ui_browser_test.h (right): https://codereview.chromium.org/320753002/diff/20001/chrome/test/base/web_ui_... chrome/test/base/web_ui_browser_test.h:32: IN_PROC_BROWSER_TEST_F(x, y) { ASSERT_TRUE(RunJavascriptTest(#y)); } Lots of git cl format churn in this cl... https://codereview.chromium.org/320753002/diff/20001/chrome/test/base/web_ui_... File chrome/test/base/web_ui_browser_test_browsertest.cc (right): https://codereview.chromium.org/320753002/diff/20001/chrome/test/base/web_ui_... chrome/test/base/web_ui_browser_test_browsertest.cc:84: void TestDone() { RunJavascriptFunction("testDone"); } Is this file *just* git cl format or is there any non-style change here? If the former, can we dorp it from the cl to not disttact the rest? https://codereview.chromium.org/320753002/diff/20001/chrome/test/data/webui/t... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/320753002/diff/20001/chrome/test/data/webui/t... chrome/test/data/webui/test_api.js:836: valueResult = { 'result': result[0], message: result[1]}; nit: put a space before the last } to be symmetric.
On Sun, Jun 8, 2014 at 4:27 PM, <plundblad@chromium.org> wrote: > Hi, > > Exciting step in the right direction I think. I am wondering if we shoud > leave the chromevox test base class a little more generic allowing the > javascript test fixture to specify what extension to run in to ease > reuse. I think it is perhaps not in scope for this cl to write a generic ExtensionJavaScriptBrowserTest class; JavaScriptBrowserTest currently holds some common structure for WebUI and ChromeVox related testing. We would basically diverge this code away from WebUI browser test if we went with what I think you're suggesting (and not making WebUIBrowserTest subclass JavaScriptBrowserTest). Re-writing the building of the library assets and the test run call is trivial and probably would be cleaner this time around, but wasn't too important I think for the first cut; we can chat about it more tomorrow. > We could then have a chromevox fixture to base all our tests on > A few suggestions/nits below. > > > > https://codereview.chromium.org/320753002/diff/20001/ > chrome/browser/resources/chromeos/chromevox/chromevox_browsertest.h > File chrome/browser/resources/chromeos/chromevox/chromevox_browsertest.h > (right): > > https://codereview.chromium.org/320753002/diff/20001/ > chrome/browser/resources/chromeos/chromevox/chromevox_ > browsertest.h#newcode29 > chrome/browser/resources/chromeos/chromevox/chromevox_browsertest.h:29: > // Implicit overrides assumed by js2gtest. > I don't know what an implicit override is. Can you jsut say that this > method is used by js2gtest to ... etc. > > https://codereview.chromium.org/320753002/diff/20001/ > chrome/browser/resources/chromeos/chromevox/chromevox_ > browsertest.h#newcode39 > chrome/browser/resources/chromeos/chromevox/chromevox_browsertest.h:39: > DISALLOW_COPY_AND_ASSIGN(ChromeVoxBrowserTest); > nit: blank line before this one. > > https://codereview.chromium.org/320753002/diff/20001/ > chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi > File chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi > (right): > > https://codereview.chromium.org/320753002/diff/20001/ > chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi#newcode76 > chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi:76: # > isolate. > nit: extra space. > > https://codereview.chromium.org/320753002/diff/20001/ > chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi#newcode78 > chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi:78: > 'extension': 'chromevoxjstest', > cvxojstest? Shorter and this rules is local to our target, so there > shouldn't > be any risk for conflict. > > https://codereview.chromium.org/320753002/diff/20001/ > chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi#newcode86 > chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi:86: > '<(chromevox_test_deps_js_file)', > Do we need the deps file for these tests? Shouldn't they call code > already > loaded as part of the extension? > > https://codereview.chromium.org/320753002/diff/20001/ > chrome/chrome_tests.gypi > File chrome/chrome_tests.gypi (right): > > https://codereview.chromium.org/320753002/diff/20001/ > chrome/chrome_tests.gypi#newcode1544 > chrome/chrome_tests.gypi:1544: 'test/base/javascript_browsertest.cc', > Should be called javascript_browser_test.cc for consistency since it is > a test > framework class and not a test. > > https://codereview.chromium.org/320753002/diff/20001/ > chrome/test/base/javascript_browsertest.cc > File chrome/test/base/javascript_browsertest.cc (right): > > https://codereview.chromium.org/320753002/diff/20001/ > chrome/test/base/javascript_browsertest.cc#newcode14 > chrome/test/base/javascript_browsertest.cc:14: const > base::FilePath::CharType > Shouldn't we keep this in the webui tests? We should never run the > accessibility audit on a chromevox test I think. > > https://codereview.chromium.org/320753002/diff/20001/ > chrome/test/base/javascript_browsertest.h > File chrome/test/base/javascript_browsertest.h (right): > > https://codereview.chromium.org/320753002/diff/20001/ > chrome/test/base/javascript_browsertest.h#newcode13 > chrome/test/base/javascript_browsertest.h:13: class > JavaScriptBrowserTest : public InProcessBrowserTest { > Documentation? > > https://codereview.chromium.org/320753002/diff/20001/ > chrome/test/base/javascript_browsertest.h#newcode18 > chrome/test/base/javascript_browsertest.h:18: static const > base::FilePath::CharType kWebUITestFolder[]; > Are these statics used by anything or could they be left as > file-private? > > https://codereview.chromium.org/320753002/diff/20001/ > chrome/test/base/javascript_browsertest.h#newcode30 > chrome/test/base/javascript_browsertest.h:30: void SetUpOnMainThread(); > OVERRIDE > > https://codereview.chromium.org/320753002/diff/20001/ > chrome/test/base/web_ui_browser_test.h > File chrome/test/base/web_ui_browser_test.h (right): > > https://codereview.chromium.org/320753002/diff/20001/ > chrome/test/base/web_ui_browser_test.h#newcode32 > chrome/test/base/web_ui_browser_test.h:32: IN_PROC_BROWSER_TEST_F(x, y) > { ASSERT_TRUE(RunJavascriptTest(#y)); } > Lots of git cl format churn in this cl... > > https://codereview.chromium.org/320753002/diff/20001/ > chrome/test/base/web_ui_browser_test_browsertest.cc > File chrome/test/base/web_ui_browser_test_browsertest.cc (right): > > https://codereview.chromium.org/320753002/diff/20001/ > chrome/test/base/web_ui_browser_test_browsertest.cc#newcode84 > chrome/test/base/web_ui_browser_test_browsertest.cc:84: void TestDone() > { RunJavascriptFunction("testDone"); } > Is this file *just* git cl format or is there any non-style change here? > If the former, can we dorp it from the cl to not disttact the rest? > > https://codereview.chromium.org/320753002/diff/20001/ > chrome/test/data/webui/test_api.js > File chrome/test/data/webui/test_api.js (right): > > https://codereview.chromium.org/320753002/diff/20001/ > chrome/test/data/webui/test_api.js#newcode836 > chrome/test/data/webui/test_api.js:836: valueResult = { 'result': > result[0], message: result[1]}; > nit: put a space before the last } to be symmetric. > > https://codereview.chromium.org/320753002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Love it. This seems like a great first step at abstracting the webui code and really useful for ChromeVox testing! https://codereview.chromium.org/320753002/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/chromevox/chromevox_browsertest.cc (right): https://codereview.chromium.org/320753002/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox_browsertest.cc:32: chromevox_load_looper_->Run(); Optional: it'd be nice to extract a helper class (SpokenFeedbackToggleWaiter, etc.) that enables ChromeVox and waits for it to finish loading, for use by other tests, like spoken_feedback_browsertest, which cheats by just waiting on the spoken feedback enabled speech. https://codereview.chromium.org/320753002/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox_browsertest.cc:58: scoped_ptr<base::Value> valueResult; Nit: value_result https://codereview.chromium.org/320753002/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox_browsertest.cc:61: base::DictionaryValue* dictValue = same - switch to c++ naming style for locals in this function https://codereview.chromium.org/320753002/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi (right): https://codereview.chromium.org/320753002/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi:78: 'extension': 'chromevoxjstest', On 2014/06/08 23:27:21, Peter Lundblad wrote: > cvxojstest? Shorter and this rules is local to our target, so there shouldn't > be any risk for conflict. Yeah, considering the rule is local to the target I don't even think we need cvox/chromevox in the name - how about extjs for running in the extension vs unitjs, etc.? https://codereview.chromium.org/320753002/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi:114: '<(DEPTH)/chrome/test/base/javascrip_browsertest.h', javascrip -> javascript https://codereview.chromium.org/320753002/diff/20001/chrome/test/base/javascr... File chrome/test/base/javascript_browsertest.h (right): https://codereview.chromium.org/320753002/diff/20001/chrome/test/base/javascr... chrome/test/base/javascript_browsertest.h:40: // call. nit: fits on previous line https://codereview.chromium.org/320753002/diff/20001/chrome/test/base/js2gtes... File chrome/test/base/js2gtest.js (right): https://codereview.chromium.org/320753002/diff/20001/chrome/test/base/js2gtes... chrome/test/base/js2gtest.js:7: * tests for WebUI and unit tests. Generates C++ gtest wrappers Update this top comment too https://codereview.chromium.org/320753002/diff/20001/chrome/test/base/js2gtes... chrome/test/base/js2gtest.js:52: * @type {string} ('unit'| 'webui') Update this too https://codereview.chromium.org/320753002/diff/20001/chrome/test/data/webui/t... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/320753002/diff/20001/chrome/test/data/webui/t... chrome/test/data/webui/test_api.js:90: typedefCppFixture: 'WebUIBrowserTest', Should this change or be refactored out? https://codereview.chromium.org/320753002/diff/20001/chrome/test/data/webui/t... chrome/test/data/webui/test_api.js:798: * URL to dummy WebUI page for testing framework. There's so much webui-specific stuff in here, do you think it would be worth having a "shared" test_api.js with just common code, and a subclass for webui and for chromevox? Even if you didn't complete the refactoring now, just starting things in that direction might help clarify things and might encourage future refactoring to make it clearer. https://codereview.chromium.org/320753002/diff/20001/chrome/test/data/webui/t... chrome/test/data/webui/test_api.js:833: if (chrome.send) { I think it'd be more clear if you determined whether this was a webui test or a chromevox test in setup/initialization, and then just switch on that here. But if not, at least clearly document here that chrome.send is for webui tests, and so on...
PTAL; abstracted the extensiony test into a proper class and pushed down the ChromeVox specifics back into the fixture (js side). https://codereview.chromium.org/320753002/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/chromevox/chromevox_browsertest.cc (right): https://codereview.chromium.org/320753002/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox_browsertest.cc:32: chromevox_load_looper_->Run(); On 2014/06/09 06:59:45, dmazzoni wrote: > Optional: it'd be nice to extract a helper class (SpokenFeedbackToggleWaiter, > etc.) that enables ChromeVox and waits for it to finish loading, for use by > other tests, like spoken_feedback_browsertest, which cheats by just waiting on > the spoken feedback enabled speech. > Ok, done. https://codereview.chromium.org/320753002/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox_browsertest.cc:58: scoped_ptr<base::Value> valueResult; On 2014/06/09 06:59:45, dmazzoni wrote: > Nit: value_result Done. https://codereview.chromium.org/320753002/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox_browsertest.cc:61: base::DictionaryValue* dictValue = On 2014/06/09 06:59:45, dmazzoni wrote: > same - switch to c++ naming style for locals in this function Done. https://codereview.chromium.org/320753002/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/chromevox/chromevox_browsertest.h (right): https://codereview.chromium.org/320753002/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox_browsertest.h:29: // Implicit overrides assumed by js2gtest. On 2014/06/08 23:27:21, Peter Lundblad wrote: > I don't know what an implicit override is. Can you jsut say that this > method is used by js2gtest to ... etc. Done. https://codereview.chromium.org/320753002/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox_browsertest.h:39: DISALLOW_COPY_AND_ASSIGN(ChromeVoxBrowserTest); On 2014/06/08 23:27:21, Peter Lundblad wrote: > nit: blank line before this one. Done. https://codereview.chromium.org/320753002/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi (right): https://codereview.chromium.org/320753002/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi:76: # isolate. On 2014/06/08 23:27:21, Peter Lundblad wrote: > nit: extra space. Done. https://codereview.chromium.org/320753002/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi:78: 'extension': 'chromevoxjstest', On 2014/06/09 06:59:46, dmazzoni wrote: > On 2014/06/08 23:27:21, Peter Lundblad wrote: > > cvxojstest? Shorter and this rules is local to our target, so there shouldn't > > be any risk for conflict. > > Yeah, considering the rule is local to the target I don't even think we need > cvox/chromevox in the name - how about extjs for running in the extension vs > unitjs, etc.? > Done. https://codereview.chromium.org/320753002/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi:86: '<(chromevox_test_deps_js_file)', On 2014/06/08 23:27:21, Peter Lundblad wrote: > Do we need the deps file for these tests? Shouldn't they call code already > loaded as part of the extension? Removed. https://codereview.chromium.org/320753002/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi:114: '<(DEPTH)/chrome/test/base/javascrip_browsertest.h', On 2014/06/09 06:59:45, dmazzoni wrote: > javascrip -> javascript Done. https://codereview.chromium.org/320753002/diff/20001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/320753002/diff/20001/chrome/chrome_tests.gypi... chrome/chrome_tests.gypi:1544: 'test/base/javascript_browsertest.cc', On 2014/06/08 23:27:21, Peter Lundblad wrote: > Should be called javascript_browser_test.cc for consistency since it is a test > framework class and not a test. ok https://codereview.chromium.org/320753002/diff/20001/chrome/test/base/javascr... File chrome/test/base/javascript_browsertest.cc (right): https://codereview.chromium.org/320753002/diff/20001/chrome/test/base/javascr... chrome/test/base/javascript_browsertest.cc:14: const base::FilePath::CharType On 2014/06/08 23:27:22, Peter Lundblad wrote: > Shouldn't we keep this in the webui tests? We should never run the > accessibility audit on a chromevox test I think. Requires changing the generator / test api. Punting for future cl. https://codereview.chromium.org/320753002/diff/20001/chrome/test/base/javascr... File chrome/test/base/javascript_browsertest.h (right): https://codereview.chromium.org/320753002/diff/20001/chrome/test/base/javascr... chrome/test/base/javascript_browsertest.h:13: class JavaScriptBrowserTest : public InProcessBrowserTest { On 2014/06/08 23:27:22, Peter Lundblad wrote: > Documentation? Done. https://codereview.chromium.org/320753002/diff/20001/chrome/test/base/javascr... chrome/test/base/javascript_browsertest.h:18: static const base::FilePath::CharType kWebUITestFolder[]; On 2014/06/08 23:27:22, Peter Lundblad wrote: > Are these statics used by anything or could they be left as file-private? They're used by some of the print preview tests I believe. Should clean these up but for a future cl. https://codereview.chromium.org/320753002/diff/20001/chrome/test/base/javascr... chrome/test/base/javascript_browsertest.h:30: void SetUpOnMainThread(); On 2014/06/08 23:27:22, Peter Lundblad wrote: > OVERRIDE Done. https://codereview.chromium.org/320753002/diff/20001/chrome/test/base/javascr... chrome/test/base/javascript_browsertest.h:40: // call. On 2014/06/09 06:59:46, dmazzoni wrote: > nit: fits on previous line Done. https://codereview.chromium.org/320753002/diff/20001/chrome/test/base/web_ui_... File chrome/test/base/web_ui_browser_test_browsertest.cc (right): https://codereview.chromium.org/320753002/diff/20001/chrome/test/base/web_ui_... chrome/test/base/web_ui_browser_test_browsertest.cc:84: void TestDone() { RunJavascriptFunction("testDone"); } On 2014/06/08 23:27:22, Peter Lundblad wrote: > Is this file *just* git cl format or is there any non-style change here? > If the former, can we dorp it from the cl to not disttact the rest? reverted file. https://codereview.chromium.org/320753002/diff/20001/chrome/test/data/webui/t... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/320753002/diff/20001/chrome/test/data/webui/t... chrome/test/data/webui/test_api.js:833: if (chrome.send) { On 2014/06/09 06:59:46, dmazzoni wrote: > I think it'd be more clear if you determined whether this was a webui test or a > chromevox test in setup/initialization, and then just switch on that here. But > if not, at least clearly document here that chrome.send is for webui tests, and > so on... There are just two instances with probably no more coming, so I'll just document here. https://codereview.chromium.org/320753002/diff/20001/chrome/test/data/webui/t... chrome/test/data/webui/test_api.js:836: valueResult = { 'result': result[0], message: result[1]}; On 2014/06/08 23:27:22, Peter Lundblad wrote: > nit: put a space before the last } to be symmetric. Done.
lgtm https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... File chrome/test/base/extension_load_waiter.cc (right): https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.cc:48: void ExtensionLoadWaiter::Reset() { Nit: blank line before https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... File chrome/test/base/extension_load_waiter.h (right): https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.h:16: // A class used to load and wait for the ChromeVox component extension. Update this comment, this waits for any extension to load. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/javascr... File chrome/test/base/javascript_browser_test.h (right): https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/javascr... chrome/test/base/javascript_browser_test.h:22: JavaScriptBrowserTest() {} Nit: blank line before
+ phajdan.jr for chrome/test I've slightly changed the way WaitFor works; it now takes a Closure which will load the extension to be waited for. I've also changed the ChromeVox fixture to unload ChromeVox if previously loaded. This is in part to give each test a clean environment to run in and also to work around the problem of tests failing if you have a braille display connected (since ChromeVox auto starts). Thanks. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... File chrome/test/base/extension_load_waiter.cc (right): https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.cc:48: void ExtensionLoadWaiter::Reset() { On 2014/06/10 16:06:16, dmazzoni wrote: > Nit: blank line before Done. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/javascr... File chrome/test/base/javascript_browser_test.h (right): https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/javascr... chrome/test/base/javascript_browser_test.h:22: JavaScriptBrowserTest() {} On 2014/06/10 16:06:16, dmazzoni wrote: > Nit: blank line before Done.
https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... File chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi (right): https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi:97: '--deps_js', '<(chromevox_test_deps_js_file)', Need to remove deps file here as well. https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi:97: '--deps_js', '<(chromevox_test_deps_js_file)', Need to remove deps file here as well. https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi:97: '--deps_js', '<(chromevox_test_deps_js_file)', Need to remove deps file here as well. https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi:132: '<(DEPTH)/chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs', nit: Use .. here instead of the full path to make the line shorter and make it easier to find the important part. https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi:132: '<(DEPTH)/chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs', nit: Use .. here instead of the full path to make the line shorter and make it easier to find the important part. https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi:132: '<(DEPTH)/chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs', nit: Use .. here instead of the full path to make the line shorter and make it easier to find the important part. https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... File chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs (right): https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs:19: browsePreload: null, This seems like something we would want in a common base class for all tests of this kind. https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs:19: browsePreload: null, This seems like something we would want in a common base class for all tests of this kind. https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs:19: browsePreload: null, This seems like something we would want in a common base class for all tests of this kind. https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs:22: closureModuleDeps: [], This is the value from the base class, why do you repeat it here? https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs:22: closureModuleDeps: [], This is the value from the base class, why do you repeat it here? https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs:22: closureModuleDeps: [], This is the value from the base class, why do you repeat it here? https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs:26: GEN_BLOCK(function() {/*! nit: indentation. https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs:26: GEN_BLOCK(function() {/*! nit: indentation. https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs:26: GEN_BLOCK(function() {/*! nit: indentation. https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs:35: GEN_BLOCK(function() {/*! nit: indentation. https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs:35: GEN_BLOCK(function() {/*! nit: indentation. https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs:35: GEN_BLOCK(function() {/*! nit: indentation. https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs:43: }; I think this is a candidate for refactoring to split the common test stuff from te tests below. https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs:43: }; I think this is a candidate for refactoring to split the common test stuff from te tests below. https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs:43: }; I think this is a candidate for refactoring to split the common test stuff from te tests below. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... File chrome/test/base/extension_js_browser_test.h (right): https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_js_browser_test.h:14: // Add (or reuse) a WaitFor* method (e.g. WaitForWebStoreExtension) to this Seems like the documentation is making forward-looking statements;) https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_js_browser_test.h:14: // Add (or reuse) a WaitFor* method (e.g. WaitForWebStoreExtension) to this Seems like the documentation is making forward-looking statements;) https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_js_browser_test.h:14: // Add (or reuse) a WaitFor* method (e.g. WaitForWebStoreExtension) to this Seems like the documentation is making forward-looking statements;) https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_js_browser_test.h:29: void WaitFor(const char* extension_id); Call WaitForExtension for clarity. Also, clarify what happens if the extension was already loaded. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_js_browser_test.h:29: void WaitFor(const char* extension_id); Call WaitForExtension for clarity. Also, clarify what happens if the extension was already loaded. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_js_browser_test.h:29: void WaitFor(const char* extension_id); Call WaitForExtension for clarity. Also, clarify what happens if the extension was already loaded. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... File chrome/test/base/extension_load_waiter.cc (right): https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.cc:14: : load_looper_(NULL), extension_id_(""), browser_context_(NULL) { No need to initialize scoped_refptr with NULL, set extension_id_ to NULL instead of empty string? https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.cc:14: : load_looper_(NULL), extension_id_(""), browser_context_(NULL) { No need to initialize scoped_refptr with NULL, set extension_id_ to NULL instead of empty string? https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.cc:14: : load_looper_(NULL), extension_id_(""), browser_context_(NULL) { No need to initialize scoped_refptr with NULL, set extension_id_ to NULL instead of empty string? https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.cc:21: Reset(); Suggestion: make this so that you can only wait once to make the state management of this class less error-prone (say you wait for a second extension and then something else uses the browser context believeing it is still for the first extension). https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.cc:21: Reset(); Suggestion: make this so that you can only wait once to make the state management of this class less error-prone (say you wait for a second extension and then something else uses the browser context believeing it is still for the first extension). https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.cc:21: Reset(); Suggestion: make this so that you can only wait once to make the state management of this class less error-prone (say you wait for a second extension and then something else uses the browser context believeing it is still for the first extension). https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.cc:48: void ExtensionLoadWaiter::Reset() { nit: add blank line. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.cc:48: void ExtensionLoadWaiter::Reset() { nit: add blank line. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.cc:48: void ExtensionLoadWaiter::Reset() { nit: add blank line. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... File chrome/test/base/extension_load_waiter.h (right): https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.h:16: // A class used to load and wait for the ChromeVox component extension. Remove chromevox here. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.h:16: // A class used to load and wait for the ChromeVox component extension. Remove chromevox here. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.h:16: // A class used to load and wait for the ChromeVox component extension. Remove chromevox here. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.h:22: // Waits for extension with |extension_id| to load. ^ nit: Do you need a particle here? https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.h:22: // Waits for extension with |extension_id| to load. ^ nit: Do you need a particle here? https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.h:22: // Waits for extension with |extension_id| to load. ^ nit: Do you need a particle here? https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.h:23: void WaitFor(const char* extension_id); Note in the docs that the extension id should be a static char array pointer. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.h:23: void WaitFor(const char* extension_id); Note in the docs that the extension id should be a static char array pointer. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.h:23: void WaitFor(const char* extension_id); Note in the docs that the extension id should be a static char array pointer. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.h:30: // Get the browser context associated with the loaded extension. Note that this can only be called after waiting for an extension. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.h:30: // Get the browser context associated with the loaded extension. Note that this can only be called after waiting for an extension. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.h:30: // Get the browser context associated with the loaded extension. Note that this can only be called after waiting for an extension. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.h:33: // Get the id of the loaded extension. Either make this class a one-shot only or change documentation to 'last loaded extension'. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.h:33: // Get the id of the loaded extension. Either make this class a one-shot only or change documentation to 'last loaded extension'. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.h:33: // Get the id of the loaded extension. Either make this class a one-shot only or change documentation to 'last loaded extension'. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/javascr... File chrome/test/base/javascript_browser_test.cc (right): https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/javascr... chrome/test/base/javascript_browser_test.cc:54: AddLibrary(base::FilePath(kA11yAuditLibraryJSPath)); Want to add a TODO that this should be moved to webui test? https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/javascr... chrome/test/base/javascript_browser_test.cc:54: AddLibrary(base::FilePath(kA11yAuditLibraryJSPath)); Want to add a TODO that this should be moved to webui test? https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/javascr... chrome/test/base/javascript_browser_test.cc:54: AddLibrary(base::FilePath(kA11yAuditLibraryJSPath)); Want to add a TODO that this should be moved to webui test? https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/js2gtes... File chrome/test/base/js2gtest.js (right): https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/js2gtes... chrome/test/base/js2gtest.js:52: * @type {string} ('extension' | 'unit' | 'webui') Add code to validate that the test is one of these types. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/js2gtes... chrome/test/base/js2gtest.js:52: * @type {string} ('extension' | 'unit' | 'webui') Add code to validate that the test is one of these types. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/js2gtes... chrome/test/base/js2gtest.js:52: * @type {string} ('extension' | 'unit' | 'webui') Add code to validate that the test is one of these types. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/js2gtes... chrome/test/base/js2gtest.js:79: * but is not required or supported for |testType| === 'extension' or nit: Is this comment out of date talking about type === 'browser'? Also, simplify the esentence by not listing all tests this is not supported for;) https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/js2gtes... chrome/test/base/js2gtest.js:79: * but is not required or supported for |testType| === 'extension' or nit: Is this comment out of date talking about type === 'browser'? Also, simplify the esentence by not listing all tests this is not supported for;) https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/js2gtes... chrome/test/base/js2gtest.js:79: * but is not required or supported for |testType| === 'extension' or nit: Is this comment out of date talking about type === 'browser'? Also, simplify the esentence by not listing all tests this is not supported for;) https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/js2gtes... chrome/test/base/js2gtest.js:298: maybeGenHeader(testFixture); Does the defered generation of the file header play well with the GEN function?
https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... File chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi (right): https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi:97: '--deps_js', '<(chromevox_test_deps_js_file)', On 2014/06/11 16:45:04, Peter Lundblad wrote: > Need to remove deps file here as well. Done. https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox_tests.gypi:132: '<(DEPTH)/chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs', On 2014/06/11 16:45:04, Peter Lundblad wrote: > nit: Use .. here instead of the full path to make the line shorter and make it > easier to find the important part. Done. https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... File chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs (right): https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs:19: browsePreload: null, On 2014/06/11 16:45:04, Peter Lundblad wrote: > This seems like something we would want in a common base class for all tests of > this kind. Will refactor in a future cl (once there's a second .extjs file that needs it). https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs:22: closureModuleDeps: [], On 2014/06/11 16:45:04, Peter Lundblad wrote: > This is the value from the base class, why do you repeat it here? Done. https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs:26: GEN_BLOCK(function() {/*! On 2014/06/11 16:45:04, Peter Lundblad wrote: > nit: indentation. Done. https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs:35: GEN_BLOCK(function() {/*! On 2014/06/11 16:45:04, Peter Lundblad wrote: > nit: indentation. Done. https://codereview.chromium.org/320753002/diff/60001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox2/cvox2/background/background.extjs:43: }; On 2014/06/11 16:45:04, Peter Lundblad wrote: > I think this is a candidate for refactoring to split the common test stuff from > te tests below. Same as above (needed only when there's 2+ files). https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... File chrome/test/base/extension_js_browser_test.h (right): https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_js_browser_test.h:29: void WaitFor(const char* extension_id); On 2014/06/11 16:45:05, Peter Lundblad wrote: > Call WaitForExtension for clarity. Also, clarify what happens if the extension > was already loaded. Ok. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... File chrome/test/base/extension_load_waiter.cc (right): https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.cc:14: : load_looper_(NULL), extension_id_(""), browser_context_(NULL) { On 2014/06/11 16:45:05, Peter Lundblad wrote: > No need to initialize scoped_refptr with NULL, set extension_id_ to NULL instead > of empty string? Done. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.cc:21: Reset(); On 2014/06/11 16:45:05, Peter Lundblad wrote: > Suggestion: make this so that you can only wait once to make the state > management of this class less error-prone (say you wait for a second extension > and then something else uses the browser context believeing it is still for the > first extension). Done. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.cc:48: void ExtensionLoadWaiter::Reset() { On 2014/06/11 16:45:05, Peter Lundblad wrote: > nit: add blank line. Already done. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... File chrome/test/base/extension_load_waiter.h (right): https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.h:16: // A class used to load and wait for the ChromeVox component extension. On 2014/06/10 16:06:16, dmazzoni wrote: > Update this comment, this waits for any extension to load. Already done. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.h:22: // Waits for extension with |extension_id| to load. On 2014/06/11 16:45:06, Peter Lundblad wrote: > ^ > nit: Do you need a particle here? Particle? Don't follow....it looks fine I think. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.h:23: void WaitFor(const char* extension_id); On 2014/06/11 16:45:06, Peter Lundblad wrote: > Note in the docs that the extension id should be a static char array pointer. You mean a pointer to a static char array? Ok sure. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.h:30: // Get the browser context associated with the loaded extension. On 2014/06/11 16:45:06, Peter Lundblad wrote: > Note that this can only be called after waiting for an extension. You actually can call it; it will just be NULL. Clarified the doc. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... chrome/test/base/extension_load_waiter.h:33: // Get the id of the loaded extension. On 2014/06/11 16:45:06, Peter Lundblad wrote: > Either make this class a one-shot only or change documentation to 'last loaded > extension'. Made one shot. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/javascr... File chrome/test/base/javascript_browser_test.cc (right): https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/javascr... chrome/test/base/javascript_browser_test.cc:54: AddLibrary(base::FilePath(kA11yAuditLibraryJSPath)); On 2014/06/11 16:45:07, Peter Lundblad wrote: > Want to add a TODO that this should be moved to webui test? Moved this down to WebUIBrowserTest. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/js2gtes... File chrome/test/base/js2gtest.js (right): https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/js2gtes... chrome/test/base/js2gtest.js:52: * @type {string} ('extension' | 'unit' | 'webui') On 2014/06/11 16:45:07, Peter Lundblad wrote: > Add code to validate that the test is one of these types. Sure ok. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/js2gtes... chrome/test/base/js2gtest.js:79: * but is not required or supported for |testType| === 'extension' or On 2014/06/11 16:45:07, Peter Lundblad wrote: > nit: Is this comment out of date talking about type === 'browser'? Also, > simplify the esentence by not listing all tests this is not supported for;) Updated. https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/js2gtes... chrome/test/base/js2gtest.js:298: maybeGenHeader(testFixture); On 2014/06/11 16:45:07, Peter Lundblad wrote: > Does the defered generation of the file header play well with the > GEN function? I haven't had any problems; was there a specific concern?
dtseng@chromium.org writes: > https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... > chrome/test/base/extension_load_waiter.h:22: // Waits for extension with > |extension_id| to load. > On 2014/06/11 16:45:06, Peter Lundblad wrote: > > ^ > > nit: Do you need a particle here? > > Particle? Don't follow....it looks fine I think. haha, I meant 'article' usch as 'the' as in 'Wiat for the extension...', but if you think it sounds correct then it is probably;) > https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/js2gtes... > chrome/test/base/js2gtest.js:298: maybeGenHeader(testFixture); > On 2014/06/11 16:45:07, Peter Lundblad wrote: > > Does the defered generation of the file header play well with the > > GEN function? > > I haven't had any problems; was there a specific concern? Look at a file such as obj/chrome/browser_tests.gen/chrome/test/data/webui/certificate_viewer_dialog_test-gen.cc before and after your change. Even if it compiles, the ordering looks really odd and could probably cause issues in the future. //Peter > https://codereview.chromium.org/320753002/ -- To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Good point. Placed maybeGenHeader inside of GEN as well. Unfortunate, but necessary it seems. On Wed, Jun 11, 2014 at 11:44 AM, 'Peter Nilsson Lundblad' via Chromium-reviews <chromium-reviews@chromium.org> wrote: > dtseng@chromium.org writes: > > > https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... > > chrome/test/base/extension_load_waiter.h:22: // Waits for extension with > > |extension_id| to load. > > On 2014/06/11 16:45:06, Peter Lundblad wrote: > > > ^ > > > nit: Do you need a particle here? > > > > Particle? Don't follow....it looks fine I think. > > haha, I meant 'article' usch as 'the' as in 'Wiat for the extension...', > but if you think it sounds correct then it is probably;) > > > > https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/js2gtes... > > chrome/test/base/js2gtest.js:298: maybeGenHeader(testFixture); > > On 2014/06/11 16:45:07, Peter Lundblad wrote: > > > Does the defered generation of the file header play well with the > > > GEN function? > > > > I haven't had any problems; was there a specific concern? > > Look at a file such as > obj/chrome/browser_tests.gen/chrome/test/data/webui/certificate_viewer_dialog_test-gen.cc > before > and after your change. Even if it compiles, the ordering looks really odd > and could > probably cause issues in the future. > > //Peter > > > https://codereview.chromium.org/320753002/ > > -- > > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@chromium.org. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Btw, ping @phajdan.jr, I thik you're still the most appropriate owner. On Wed, Jun 11, 2014 at 12:11 PM, David Tseng <dtseng@chromium.org> wrote: > Good point. Placed maybeGenHeader inside of GEN as well. Unfortunate, but > necessary it seems. > > > > On Wed, Jun 11, 2014 at 11:44 AM, 'Peter Nilsson Lundblad' via > Chromium-reviews <chromium-reviews@chromium.org> wrote: > >> dtseng@chromium.org writes: >> > >> https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/extensi... >> > chrome/test/base/extension_load_waiter.h:22: // Waits for extension with >> > |extension_id| to load. >> > On 2014/06/11 16:45:06, Peter Lundblad wrote: >> > > ^ >> > > nit: Do you need a particle here? >> > >> > Particle? Don't follow....it looks fine I think. >> >> haha, I meant 'article' usch as 'the' as in 'Wiat for the extension...', >> but if you think it sounds correct then it is probably;) >> >> > >> https://codereview.chromium.org/320753002/diff/60001/chrome/test/base/js2gtes... >> > chrome/test/base/js2gtest.js:298: maybeGenHeader(testFixture); >> > On 2014/06/11 16:45:07, Peter Lundblad wrote: >> > > Does the defered generation of the file header play well with the >> > > GEN function? >> > >> > I haven't had any problems; was there a specific concern? >> >> Look at a file such as >> obj/chrome/browser_tests.gen/chrome/test/data/webui/certificate_viewer_dialog_test-gen.cc >> before >> and after your change. Even if it compiles, the ordering looks really >> odd and could >> probably cause issues in the future. >> >> //Peter >> >> > https://codereview.chromium.org/320753002/ >> >> -- >> >> To unsubscribe from this group and stop receiving emails from it, send an >> email to chromium-reviews+unsubscribe@chromium.org. >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm
LGTM with comments https://codereview.chromium.org/320753002/diff/180001/chrome/test/base/extens... File chrome/test/base/extension_js_browser_test.cc (right): https://codereview.chromium.org/320753002/diff/180001/chrome/test/base/extens... chrome/test/base/extension_js_browser_test.cc:26: CHECK(load_waiter_->browser_context()); Please don't crash the entire test unless really necessary. Why not EXPECT_* and then return false? https://codereview.chromium.org/320753002/diff/180001/chrome/test/base/javasc... File chrome/test/base/javascript_browser_test.cc (right): https://codereview.chromium.org/320753002/diff/180001/chrome/test/base/javasc... chrome/test/base/javascript_browser_test.cc:89: ASSERT_TRUE(ok) << "User library not found: " If possible please use bool return value to communicate this instead.
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dtseng@chromium.org/320753002/200001
https://codereview.chromium.org/320753002/diff/180001/chrome/test/base/extens... File chrome/test/base/extension_js_browser_test.cc (right): https://codereview.chromium.org/320753002/diff/180001/chrome/test/base/extens... chrome/test/base/extension_js_browser_test.cc:26: CHECK(load_waiter_->browser_context()); On 2014/06/13 13:45:23, Paweł Hajdan Jr. wrote: > Please don't crash the entire test unless really necessary. Why not EXPECT_* and > then return false? Done. https://codereview.chromium.org/320753002/diff/180001/chrome/test/base/javasc... File chrome/test/base/javascript_browser_test.cc (right): https://codereview.chromium.org/320753002/diff/180001/chrome/test/base/javasc... chrome/test/base/javascript_browser_test.cc:89: ASSERT_TRUE(ok) << "User library not found: " On 2014/06/13 13:45:23, Paweł Hajdan Jr. wrote: > If possible please use bool return value to communicate this instead. I've left a TODO because it would involve changing the subclasses/generated subclasses to assert the return value is true which I'll try to handle in a second round focused on cleanup.
Message was sent while issue was closed.
Change committed as 277249 |
