Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7553009:
Add some browser tests for net-internals (Closed)
|
Created:
9 years, 4 months ago by mmenke Modified:
9 years, 4 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, tburkard+watch_chromium.org, dominich+watch_chromium.org, eroman, arv (Not doing code reviews), mmenke Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionUpdate net_internals tests to use the updated framework.
BUG=89057
TEST=NetInternalsTest.*
Patch Set 1 : '' #Patch Set 2 : update comments #Patch Set 3 : Fix test_view.js #
Total comments: 2
Patch Set 4 : Response to Paweł's comment, test fix (?) #Patch Set 5 : Just test result of first test #Patch Set 6 : Just test result of first test #
Total comments: 57
Patch Set 7 : Response to comments #Patch Set 8 : Rename framework.js #Patch Set 9 : Use g_browser.receive as function name when appropriate. #Patch Set 10 : Use g_browser.receive as function name when appropriate (Really, this time). #
Total comments: 38
Patch Set 11 : Response to Sheridan's comments (+Jdoc, +Unreached(). #Patch Set 12 : Fix comments #
Total comments: 10
Patch Set 13 : Response to Sheridan's comments #
Total comments: 2
Patch Set 14 : Response to Sheridan's comments (3) #Patch Set 15 : ??? #Patch Set 16 : sync #Patch Set 17 : Fix comment #Patch Set 18 : '' #
Messages
Total messages: 18 (0 generated)
Want to hear what you think of this testing framework for about:net-internals. On the plus side, the Javascript test runs asynchronously with the browser test code, so we can test net_internals_ui.cc code as well as the Javascript objects. We also get all expect/assert functions from test_api.js. On the down side, the infrastructure is a little complicated and hacks up test_api.js a bit. We'll also still have to use mocked out observed messages for things we can't easily have the browser do in actual tests (Like run SPDY, I believe). I'm currently not directly using the views I'm testing (TestView, etc), but we can add in some direct checks later. I think messing with the DOM directly is nice in that it makes sure the buttons are hooked up and such. Would catch binding a view to the wrong TabHandle, if done correctly, for instance.
I definitely like the title of this code review! Note that I am gone for the next 1-2 hours, I will get to this when I return. On Tue, Aug 2, 2011 at 11:59 AM, <mmenke@chromium.org> wrote: > Reviewers: eroman, > > Message: > Want to hear what you think of this testing framework for > about:net-internals. > > On the plus side, the Javascript test runs asynchronously with the browser > test > code, so we can test net_internals_ui.cc code as well as the Javascript > objects. > We also get all expect/assert functions from test_api.js. > > On the down side, the infrastructure is a little complicated and hacks up > test_api.js a bit. We'll also still have to use mocked out observed > messages > for things we can't easily have the browser do in actual tests (Like run > SPDY, I > believe). > > I'm currently not directly using the views I'm testing (TestView, etc), but > we > can add in some direct checks later. I think messing with the DOM directly > is > nice in that it makes sure the buttons are hooked up and such. Would catch > binding a view to the wrong TabHandle, if done correctly, for instance. > > Description: > Add browser test framework for net-internals, along with > tests for the test and prerender tabs, and removing > cookies and login info. > > BUG=89057 > TEST=NetInternalsTest.* > > Please review this at http://codereview.chromium.**org/7553009/<http://codereview.chromium.org/7553... > > SVN Base: svn://svn.chromium.org/chrome/**trunk/src/<http://svn.chromium.org/chrome/trunk/src/> > > Affected files: > M chrome/browser/prerender/**prerender_browsertest.cc > M chrome/browser/resources/net_**internals/browser_bridge.js > M chrome/browser/resources/net_**internals/test_view.html > A chrome/browser/ui/webui/net_**internals_ui_browsertest.cc > M chrome/chrome_tests.gypi > M chrome/common/url_constants.h > M chrome/common/url_constants.cc > M chrome/test/base/ui_test_**utils.h > M chrome/test/base/ui_test_**utils.cc > A chrome/test/data/webui/net_**internals/framework.js > A chrome/test/data/webui/net_**internals/log_view_painter.js > A chrome/test/data/webui/net_**internals/prerender_view.js > A chrome/test/data/webui/net_**internals/test_view.js > M chrome/test/data/webui/test_**api.js > > >
Drive-by with testing comments (chrome/test/OWNERS here, you need approval to commit). http://codereview.chromium.org/7553009/diff/7021/chrome/test/base/ui_test_uti... File chrome/test/base/ui_test_utils.h (right): http://codereview.chromium.org/7553009/diff/7021/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.h:501: // probably best not to rely of that to avoid any races. How about returning the last seen title when the message loop stopped? That would be less error-prone.
Thanks for the feedback. http://codereview.chromium.org/7553009/diff/7021/chrome/test/base/ui_test_uti... File chrome/test/base/ui_test_utils.h (right): http://codereview.chromium.org/7553009/diff/7021/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.h:501: // probably best not to rely of that to avoid any races. On 2011/08/02 20:19:16, Paweł Hajdan Jr. wrote: > How about returning the last seen title when the message loop stopped? That > would be less error-prone. Sounds good to me. Done. Went ahead and switched to using a vector, so more than two titles are allowed, though not sure if there's any use case where that's useful.
Code I commented in the drive-by LGTM with comments, thank you. http://codereview.chromium.org/7553009/diff/7023/chrome/test/base/ui_test_uti... File chrome/test/base/ui_test_utils.h (right): http://codereview.chromium.org/7553009/diff/7023/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.h:497: void AddTitle(const string16& expected_title); nit: Let's rename this to AlsoWaitForTitle. http://codereview.chromium.org/7553009/diff/7023/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.h:502: const string16& Wait() WARN_UNUSED_RESULT; nit: Let's rename this to WaitAndGetTitle.
Thanks for adding this Matt! I am still digesting the entirety of this change, I will finish off the review after the next review round. http://codereview.chromium.org/7553009/diff/7023/chrome/browser/resources/net... File chrome/browser/resources/net_internals/browser_bridge.js (right): http://codereview.chromium.org/7553009/diff/7023/chrome/browser/resources/net... chrome/browser/resources/net_internals/browser_bridge.js:329: this.disabled_ = true; it might be a good idea to clear the polling interval here as well: this.setPollingInterval(0); (in which case should remove the comment that it is only for testing). http://codereview.chromium.org/7553009/diff/7023/chrome/browser/ui/webui/net_... File chrome/browser/ui/webui/net_internals_ui_browsertest.cc (right): http://codereview.chromium.org/7553009/diff/7023/chrome/browser/ui/webui/net_... chrome/browser/ui/webui/net_internals_ui_browsertest.cc:33: nit: can you remove this blank line? http://codereview.chromium.org/7553009/diff/7023/chrome/browser/ui/webui/net_... chrome/browser/ui/webui/net_internals_ui_browsertest.cc:49: void set_expected_title_(string16 value) { expected_title_ = value; } nit: |const string16&| http://codereview.chromium.org/7553009/diff/7023/chrome/browser/ui/webui/net_... chrome/browser/ui/webui/net_internals_ui_browsertest.cc:53: string16 expected_title_; nit: DISABLE_COPY_AN_ASSIGN(NetInternalsTest); http://codereview.chromium.org/7553009/diff/7023/chrome/browser/ui/webui/net_... chrome/browser/ui/webui/net_internals_ui_browsertest.cc:56: NetInternalsTest::NetInternalsTest() : expected_title_(kPassTitle) { I believe the constructor is run once for each of the *_TEST_F(), is that correct? I recommend ensuring |expected_title_| is reset at the start of each test. That way re-ordering tests won't cause breaks due to side-effects from having called site_expected_title(). http://codereview.chromium.org/7553009/diff/7023/chrome/browser/ui/webui/net_... chrome/browser/ui/webui/net_internals_ui_browsertest.cc:180: // the firstresult, the order of events that occur, and the number of rows in nit: firstresult --> first result. http://codereview.chromium.org/7553009/diff/7023/chrome/common/url_constants.cc File chrome/common/url_constants.cc (right): http://codereview.chromium.org/7553009/diff/7023/chrome/common/url_constants.... chrome/common/url_constants.cc:48: const char kChromeUINetInternalsURL[] = "chrome://net-internals/"; That's curious, I seem to remember adding this constant at some point. http://codereview.chromium.org/7553009/diff/7023/chrome/test/base/ui_test_uti... File chrome/test/base/ui_test_utils.cc (right): http://codereview.chromium.org/7553009/diff/7023/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.cc:937: for (std::vector<string16>::const_iterator it = expected_titles_.begin(); [optional] If you prefer, could also used std::find() for this: std::vector<string16>::const_iterator it = std::find(expected_titles_.begin(), expected_titles_.end(), source_contents->GetTitle()); if (it != expected_titles_.end()) { .... } http://codereview.chromium.org/7553009/diff/7023/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.cc:941: observed_title_ = source_contents->GetTitle(); BTW I don't know anything about the GetTitle() function, but presumably it can't change while calling it from this thread? If that weren't the case, I suggest copying it into a temporary variable to avoid confusion. http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... File chrome/test/data/webui/net_internals/framework.js (right): http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:6: * The way these tests work is this: nit: "this" --> "as follows" http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:14: * testDone is called. At that point, or soon afterwards, the title to be "the title is to be" http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:16: * exceptions. Otherwise, it's set to 'Test Passed'. The behavior if "exceptions" --> "exception" http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:29: (function() { [optional] It might be helpful to define these functions as part of a namespace. Like say "testing.testDone", "testing.switchToView" http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:58: * Creates a test function function that can use the expect and assert nit: duplicated "function function". http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:91: }; nit: this semicolon is redundant. http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:100: if (parent.children[i].className == 'styledTable') { [optional] I believe you could also use the query selectors to find this node: See http://www.webkit.org/blog/156/queryselector-and-queryselectorall/ http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:108: }; nit: redundant semi-colon. http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:169: g_browser.addConstantsObserver(new ConstantsObserver()); sexy. http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:211: }; nit: redundant semicolon http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:215: assertFalse(false); are these two calls no-ops? http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:220: g_browser.addHostResolverInfoObserver(new HostResolverInfoObserver()); Does each sub-test re-load net-internals before starting, or do they all run together? Once possible issue here is that after the testDone() is called in onHostResolverInfoChanged() we don't remove the observer. So it could get called multiple times. Not sure if that is a problem. http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:232: }; nit: redundant semicolon. http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:240: g_browser.addHostResolverInfoObserver(new HostResolverInfoObserver()); Same comment as above, we don't remove the observer, hopefully getting multiple change events isn't a problem (i.e. calling testDone() multiple times). http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:252: }; ... http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... File chrome/test/data/webui/net_internals/log_view_painter.js (right): http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/log_view_painter.js:12: var expectations = [ I'll actually have to run the trybots now when refactoring :) http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... File chrome/test/data/webui/net_internals/prerender_view.js (right): http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/prerender_view.js:31: SUCCESS_URL_LINKED: 2, nit: remove the trailing comma. I don't remember what this does in Chrome, in my past life I remeber trailing commas in objects and arrays doing weird stuff on various browsers, like inserting null objects. http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... File chrome/test/data/webui/net_internals/test_view.js (right): http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/test_view.js:22: }; nit: remove for consistency
Just a couple high level responses. http://codereview.chromium.org/7553009/diff/7023/chrome/browser/ui/webui/net_... File chrome/browser/ui/webui/net_internals_ui_browsertest.cc (right): http://codereview.chromium.org/7553009/diff/7023/chrome/browser/ui/webui/net_... chrome/browser/ui/webui/net_internals_ui_browsertest.cc:56: NetInternalsTest::NetInternalsTest() : expected_title_(kPassTitle) { On 2011/08/03 00:32:24, eroman wrote: > I believe the constructor is run once for each of the *_TEST_F(), is that > correct? > > I recommend ensuring |expected_title_| is reset at the start of each test. That > way re-ordering tests won't cause breaks due to side-effects from having called > site_expected_title(). The constructor is indeed called once for each *_TEST_F. In fact, for the browser tests, each test is run in a new process, so we don't have to worry about side effects. http://codereview.chromium.org/7553009/diff/7023/chrome/common/url_constants.cc File chrome/common/url_constants.cc (right): http://codereview.chromium.org/7553009/diff/7023/chrome/common/url_constants.... chrome/common/url_constants.cc:48: const char kChromeUINetInternalsURL[] = "chrome://net-internals/"; On 2011/08/03 00:32:24, eroman wrote: > That's curious, I seem to remember adding this constant at some point. There's a separate constant for the url without the chrome://. Maybe that's what you're remembering. Or maybe someone removed the need for it at some point and removed the entry. http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... File chrome/test/data/webui/net_internals/framework.js (right): http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:169: g_browser.addConstantsObserver(new ConstantsObserver()); On 2011/08/03 00:32:24, eroman wrote: > sexy. If we sent constants from the UI thread, we could probably get rid of that...But I'm paranoid. http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:215: assertFalse(false); On 2011/08/03 00:32:24, eroman wrote: > are these two calls no-ops? Yea, or at least they should be, essentially. They do have some side effects, as each call from each function is tracked, so, for example, if the 5th call of assertTrue in one function fails, the framework can guess which call it was that asserted. http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:220: g_browser.addHostResolverInfoObserver(new HostResolverInfoObserver()); On 2011/08/03 00:32:24, eroman wrote: > Does each sub-test re-load net-internals before starting, or do they all run > together? > > Once possible issue here is that after the testDone() is called in > onHostResolverInfoChanged() we don't remove the observer. So it could get called > multiple times. Not sure if that is a problem. Each test runs in a separate process and navigates to the page again, which is why I don't bother to clean up. It's also part of the reason why the browser tests are so slow.
http://codereview.chromium.org/7553009/diff/7023/chrome/browser/resources/net... File chrome/browser/resources/net_internals/browser_bridge.js (right): http://codereview.chromium.org/7553009/diff/7023/chrome/browser/resources/net... chrome/browser/resources/net_internals/browser_bridge.js:329: this.disabled_ = true; On 2011/08/03 00:32:24, eroman wrote: > it might be a good idea to clear the polling interval here as well: > > this.setPollingInterval(0); > > > (in which case should remove the comment that it is only for testing). Good idea, done. http://codereview.chromium.org/7553009/diff/7023/chrome/browser/ui/webui/net_... File chrome/browser/ui/webui/net_internals_ui_browsertest.cc (right): http://codereview.chromium.org/7553009/diff/7023/chrome/browser/ui/webui/net_... chrome/browser/ui/webui/net_internals_ui_browsertest.cc:33: On 2011/08/03 00:32:24, eroman wrote: > nit: can you remove this blank line? Done http://codereview.chromium.org/7553009/diff/7023/chrome/browser/ui/webui/net_... chrome/browser/ui/webui/net_internals_ui_browsertest.cc:49: void set_expected_title_(string16 value) { expected_title_ = value; } On 2011/08/03 00:32:24, eroman wrote: > nit: |const string16&| Done. http://codereview.chromium.org/7553009/diff/7023/chrome/browser/ui/webui/net_... chrome/browser/ui/webui/net_internals_ui_browsertest.cc:53: string16 expected_title_; On 2011/08/03 00:32:24, eroman wrote: > nit: DISABLE_COPY_AN_ASSIGN(NetInternalsTest); Done. http://codereview.chromium.org/7553009/diff/7023/chrome/browser/ui/webui/net_... chrome/browser/ui/webui/net_internals_ui_browsertest.cc:180: // the firstresult, the order of events that occur, and the number of rows in On 2011/08/03 00:32:24, eroman wrote: > nit: firstresult --> first result. Done. http://codereview.chromium.org/7553009/diff/7023/chrome/test/base/ui_test_uti... File chrome/test/base/ui_test_utils.cc (right): http://codereview.chromium.org/7553009/diff/7023/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.cc:937: for (std::vector<string16>::const_iterator it = expected_titles_.begin(); On 2011/08/03 00:32:24, eroman wrote: > [optional] If you prefer, could also used std::find() for this: > > std::vector<string16>::const_iterator it = std::find(expected_titles_.begin(), > expected_titles_.end(), source_contents->GetTitle()); > > if (it != expected_titles_.end()) { > .... > } > Good idea, done. http://codereview.chromium.org/7553009/diff/7023/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.cc:941: observed_title_ = source_contents->GetTitle(); On 2011/08/03 00:32:24, eroman wrote: > BTW I don't know anything about the GetTitle() function, but presumably it can't > change while calling it from this thread? > > If that weren't the case, I suggest copying it into a temporary variable to > avoid confusion. It can't change, but switching to using .find() results in only one call, anyways. http://codereview.chromium.org/7553009/diff/7023/chrome/test/base/ui_test_uti... File chrome/test/base/ui_test_utils.h (right): http://codereview.chromium.org/7553009/diff/7023/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.h:497: void AddTitle(const string16& expected_title); On 2011/08/02 21:45:57, Paweł Hajdan Jr. wrote: > nit: Let's rename this to AlsoWaitForTitle. Done. http://codereview.chromium.org/7553009/diff/7023/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.h:502: const string16& Wait() WARN_UNUSED_RESULT; On 2011/08/02 21:45:57, Paweł Hajdan Jr. wrote: > nit: Let's rename this to WaitAndGetTitle. Done. http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... File chrome/test/data/webui/net_internals/framework.js (right): http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:6: * The way these tests work is this: On 2011/08/03 00:32:24, eroman wrote: > nit: "this" --> "as follows" Done. http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:14: * testDone is called. At that point, or soon afterwards, the title to be On 2011/08/03 00:32:24, eroman wrote: > "the title is to be" Done. http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:16: * exceptions. Otherwise, it's set to 'Test Passed'. The behavior if On 2011/08/03 00:32:24, eroman wrote: > "exceptions" --> "exception" Done. http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:29: (function() { On 2011/08/03 00:32:24, eroman wrote: > [optional] It might be helpful to define these functions as part of a namespace. > Like say "testing.testDone", "testing.switchToView" Done, though with another namespace (netInternalsTest). http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:58: * Creates a test function function that can use the expect and assert On 2011/08/03 00:32:24, eroman wrote: > nit: duplicated "function function". Fixed. http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:91: }; On 2011/08/03 00:32:24, eroman wrote: > nit: this semicolon is redundant. Done. http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:100: if (parent.children[i].className == 'styledTable') { On 2011/08/03 00:32:24, eroman wrote: > [optional] I believe you could also use the query selectors to find this node: > > See http://www.webkit.org/blog/156/queryselector-and-queryselectorall/ Done. http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:108: }; On 2011/08/03 00:32:24, eroman wrote: > nit: redundant semi-colon. Removed. http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:211: }; On 2011/08/03 00:32:24, eroman wrote: > nit: redundant semicolon Done. http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:215: assertFalse(false); On 2011/08/03 00:53:22, Matt Menke wrote: > On 2011/08/03 00:32:24, eroman wrote: > > are these two calls no-ops? > > Yea, or at least they should be, essentially. They do have some side effects, > as each call from each function is tracked, so, for example, if the 5th call of > assertTrue in one function fails, the framework can guess which call it was that > asserted. Went ahead and removed them. http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:232: }; On 2011/08/03 00:32:24, eroman wrote: > nit: redundant semicolon. Done. http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/framework.js:252: }; On 2011/08/03 00:32:24, eroman wrote: > ... Done. http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... File chrome/test/data/webui/net_internals/prerender_view.js (right): http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/prerender_view.js:31: SUCCESS_URL_LINKED: 2, On 2011/08/03 00:32:24, eroman wrote: > nit: remove the trailing comma. I don't remember what this does in Chrome, in my > past life I remeber trailing commas in objects and arrays doing weird stuff on > various browsers, like inserting null objects. Done. http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... File chrome/test/data/webui/net_internals/test_view.js (right): http://codereview.chromium.org/7553009/diff/7023/chrome/test/data/webui/net_i... chrome/test/data/webui/net_internals/test_view.js:22: }; On 2011/08/03 00:32:24, eroman wrote: > nit: remove for consistency Done.
[+scr]: Please review the changes to test_api.js. The change is so that I can use runTestFunction() asynchronously in callbacks from the browser, so can test the C++ backend in addition to the Javascript code.
LGTM
Partial review of js (have to run to meetings). Mostly nits on JSDoc. Also, use assertNotReached() instead of assertTrue(false), e.g. (and expect* versions) http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... File chrome/test/data/webui/net_internals/net_internals_test.js (right): http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:6: * The way these tests work is as follows: nit: @fileoverview http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:25: const TESTING_POLL_INTERVAL_MS = 50; Do use @const & don't use const (see http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Constants) http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:32: * called at most once for each test. @param {boolean} success Description of success param. http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:57: * The resulting function has no return value. @param for testName, testFunction http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:85: // Dictionary of tests. @type {Object.<string, Function>} http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:100: * possible. use @param http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:133: * |g_browser.receive|. @param http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:148: */ @param http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:160: * header row. @param http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:190: expectTrue(false); expectNotReached http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:194: assertFalse(true); assertNotReached http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:245: assertTrue(false); assertNotReached http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... File chrome/test/data/webui/net_internals/prerender_view.js (right): http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/prerender_view.js:36: */ @param http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/prerender_view.js:74: */ @param http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/prerender_view.js:86: * move straight to the history. In the latter case, we skip a state. @param http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/prerender_view.js:104: * failed, and it should now be in the history. @param http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/prerender_view.js:134: * Adds a <link rel="prerender" href="url"> to the document. @param http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/test... File chrome/test/data/webui/test_api.js (right): http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/test... chrome/test/data/webui/test_api.js:486: * |errors|, runs the test surrounded by an expect to catch Errors. If This doesn't clear |errors| anymore. http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/test... chrome/test/data/webui/test_api.js:493: * @see createExpect @see runTestFunction
http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... File chrome/test/data/webui/net_internals/net_internals_test.js (right): http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:6: * The way these tests work is as follows: On 2011/08/03 18:29:57, Sheridan Rawlins wrote: > nit: @fileoverview Done. http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:25: const TESTING_POLL_INTERVAL_MS = 50; On 2011/08/03 18:29:57, Sheridan Rawlins wrote: > Do use @const & don't use const (see > http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Constants) Done. http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:32: * called at most once for each test. On 2011/08/03 18:29:57, Sheridan Rawlins wrote: > @param {boolean} success Description of success param. Done. http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:57: * The resulting function has no return value. On 2011/08/03 18:29:57, Sheridan Rawlins wrote: > @param for testName, testFunction Done. http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:85: // Dictionary of tests. On 2011/08/03 18:29:57, Sheridan Rawlins wrote: > @type {Object.<string, Function>} Done. http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:100: * possible. On 2011/08/03 18:29:57, Sheridan Rawlins wrote: > use @param Done. http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:133: * |g_browser.receive|. On 2011/08/03 18:29:57, Sheridan Rawlins wrote: > @param Done. http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:148: */ On 2011/08/03 18:29:57, Sheridan Rawlins wrote: > @param Done. http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:160: * header row. On 2011/08/03 18:29:57, Sheridan Rawlins wrote: > @param Done. http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:190: expectTrue(false); On 2011/08/03 18:29:57, Sheridan Rawlins wrote: > expectNotReached Done http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:194: assertFalse(true); On 2011/08/03 18:29:57, Sheridan Rawlins wrote: > assertNotReached Done http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:245: assertTrue(false); On 2011/08/03 18:29:57, Sheridan Rawlins wrote: > assertNotReached Done http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... File chrome/test/data/webui/net_internals/prerender_view.js (right): http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/prerender_view.js:36: */ On 2011/08/03 18:29:57, Sheridan Rawlins wrote: > @param Done. http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/prerender_view.js:74: */ On 2011/08/03 18:29:57, Sheridan Rawlins wrote: > @param Done. http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/prerender_view.js:86: * move straight to the history. In the latter case, we skip a state. On 2011/08/03 18:29:57, Sheridan Rawlins wrote: > @param Done. http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/prerender_view.js:104: * failed, and it should now be in the history. On 2011/08/03 18:29:57, Sheridan Rawlins wrote: > @param Done. http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/prerender_view.js:134: * Adds a <link rel="prerender" href="url"> to the document. On 2011/08/03 18:29:57, Sheridan Rawlins wrote: > @param Done. http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/test... File chrome/test/data/webui/test_api.js (right): http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/test... chrome/test/data/webui/test_api.js:486: * |errors|, runs the test surrounded by an expect to catch Errors. If On 2011/08/03 18:29:57, Sheridan Rawlins wrote: > This doesn't clear |errors| anymore. Wasn't sure if I should leave that in or not, as calling it does still clear |errors|. Removed. http://codereview.chromium.org/7553009/diff/25001/chrome/test/data/webui/test... chrome/test/data/webui/test_api.js:493: * @see createExpect On 2011/08/03 18:29:57, Sheridan Rawlins wrote: > @see runTestFunction Done.
LGTM with nits. http://codereview.chromium.org/7553009/diff/20023/chrome/test/data/webui/net_... File chrome/test/data/webui/net_internals/log_view_painter.js (right): http://codereview.chromium.org/7553009/diff/20023/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/log_view_painter.js:71: expectTrue(stripped !== entry, Does expectNotEquals do what you need for this? It prints better error messages on failure. http://codereview.chromium.org/7553009/diff/20023/chrome/test/data/webui/net_... File chrome/test/data/webui/net_internals/net_internals_test.js (right): http://codereview.chromium.org/7553009/diff/20023/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:31: // Indicates the test is complete. @type {boolean} http://codereview.chromium.org/7553009/diff/20023/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:179: * @param {expectedRows} expectedRows Expected number of rows in the table. s/{expectedRows}/{number}/ http://codereview.chromium.org/7553009/diff/20023/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:188: * TODO(mmenke): check that the tab visibility changes as expected. @param http://codereview.chromium.org/7553009/diff/20023/chrome/test/data/webui/net_... File chrome/test/data/webui/net_internals/test_view.js (right): http://codereview.chromium.org/7553009/diff/20023/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/test_view.js:122: * @param {expectedRows} expectedRows Expected number of rows in the table. s/{expectedRows}/{number}/
Thanks so much for the review. http://codereview.chromium.org/7553009/diff/20023/chrome/test/data/webui/net_... File chrome/test/data/webui/net_internals/log_view_painter.js (right): http://codereview.chromium.org/7553009/diff/20023/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/log_view_painter.js:71: expectTrue(stripped !== entry, On 2011/08/03 22:21:16, Sheridan Rawlins wrote: > Does expectNotEquals do what you need for this? It prints better error messages > on failure. Oh, it does. I assumed it just used ==. Switched. http://codereview.chromium.org/7553009/diff/20023/chrome/test/data/webui/net_... File chrome/test/data/webui/net_internals/net_internals_test.js (right): http://codereview.chromium.org/7553009/diff/20023/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:31: // Indicates the test is complete. On 2011/08/03 22:21:16, Sheridan Rawlins wrote: > @type {boolean} Done. http://codereview.chromium.org/7553009/diff/20023/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:179: * @param {expectedRows} expectedRows Expected number of rows in the table. On 2011/08/03 22:21:16, Sheridan Rawlins wrote: > s/{expectedRows}/{number}/ Done. http://codereview.chromium.org/7553009/diff/20023/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:188: * TODO(mmenke): check that the tab visibility changes as expected. On 2011/08/03 22:21:16, Sheridan Rawlins wrote: > @param Done. http://codereview.chromium.org/7553009/diff/20023/chrome/test/data/webui/net_... File chrome/test/data/webui/net_internals/test_view.js (right): http://codereview.chromium.org/7553009/diff/20023/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/test_view.js:122: * @param {expectedRows} expectedRows Expected number of rows in the table. On 2011/08/03 22:21:16, Sheridan Rawlins wrote: > s/{expectedRows}/{number}/ Done.
missed one nit - there may be other places you return stuff... If so, just jsdoc 'em if possible. Thanks, -SCR http://codereview.chromium.org/7553009/diff/24022/chrome/test/data/webui/net_... File chrome/test/data/webui/net_internals/net_internals_test.js (right): http://codereview.chromium.org/7553009/diff/24022/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:67: * @param {Function} testFunction The function to run. @return {function()} Description.
http://codereview.chromium.org/7553009/diff/24022/chrome/test/data/webui/net_... File chrome/test/data/webui/net_internals/net_internals_test.js (right): http://codereview.chromium.org/7553009/diff/24022/chrome/test/data/webui/net_... chrome/test/data/webui/net_internals/net_internals_test.js:67: * @param {Function} testFunction The function to run. On 2011/08/04 00:25:21, Sheridan Rawlins wrote: > @return {function()} Description. Done (And there was one other, too, also done),
Can't process patch for file chrome/test/data/webui/net_internals/test_view.js. File's status is None, patchset upload is incomplete.
Change committed as 95430 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
