Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
5610006:
Converted download UI tests to Browser tests. (Closed)
|
Created:
10 years ago by ahendrickson Modified:
9 years, 7 months ago CC:
chromium-reviews, rdsmith+dwatch_chromium.org, Paweł Hajdan Jr. Visibility:
Public. |
DescriptionConverted download UI tests to Browser tests.
Converted NavigateWithURLAsync/WaitForDownloadShelfVisible calls to NavigateToURLWithDisposition. This waits until the navigation is done before returning, avoiding the need for the 'sleep and wait' loop.
BUG=none
TEST=Run browser_tests --gtest_filter=DownloadTest.*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=70900
Patch Set 1 #Patch Set 2 : Merged from trunk. #Patch Set 3 : Cleaned up code. Fixed failing tests. #
Total comments: 59
Patch Set 4 : Merge with trunk, and address comments. #
Total comments: 21
Patch Set 5 : Implemented Randy's comments. #Patch Set 6 : Merged with trunk. #
Total comments: 33
Patch Set 7 : Changes per Pawel & Brett's comments. #
Total comments: 6
Patch Set 8 : Removed WaitForWindowClosed(). #
Total comments: 21
Patch Set 9 : Addressed Randy & Pawel's comments. #Patch Set 10 : Checked that WaitForBrowserNotInSet() does not return an excluded browser. #
Total comments: 4
Patch Set 11 : More cleanup #
Messages
Total messages: 20 (0 generated)
http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:244: ASSERT_TRUE(CreateAndSetDownloadsDirectory()); My understanding is that we can't use ASSERT_* inside of helper functions as per our discussion. So this may need to be rewritten--I'll wait on your talking with Chris. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:248: false); Is there ever a case where we *don't* want to specify whether we prompt for download or not? DisableMIMETypeSelect wants us always to prompt; are there any other cases where we want to allow prompting at all? If not, I'd make force_on_prompt into just the argument provided to the SetBoolean; that allows you to remove all uses of kPromptForDownload in all tests and just replace it with the correct arg to InitialSetup(). http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:256: void PopBrowser() { browsers_.pop(); } If you take my suggestion below about giving CreateAndSetDownloadsDirectory a Browser* argument, I think the need for these calls goes away, which I think is good, as people used to what they mean in other browser tests will be confused by the difference here. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:258: virtual Browser* browser() const { return browsers_.top(); } I'd suggest browsers_.empty() -> return NULL; http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:266: bool CreateAndSetDownloadsDirectory() { The two usages of this function are in InitialSetup() and in IncognitoDownload. In both cases we've just before the call been playing with the browser pointer we want for the call, and in IncognitoDownload we're doing a PushBrowser() just before to make sure that this call has the right browser (in a way that, to me, feels a little indirect ans susceptible to future error; you need to understand the innards of both CreateAndSetDownloadsDirectory() and PushBrowser() at the point of call in IncognitoDownload). I'd recommend just making this call take a browser argument. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:272: bool SetDownloadsDirectoryForBrowser(Browser* browser) { The only use I can find of SetDownloadsDirectoryForBrowser is above, in CreateAndSetDownloadsDirectory. If that's it, I think these two routines should be collapsed into one, at least until we need the separate functionality. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:300: // |disposition| indicates where the naiation occurs (current tab, new Nit: "navigation" http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:307: SelectExpectation expectation, |expectation| isn't mentioned in the comments for the routine and should be. The comment for that arg should mention that this routine will always unblock if the select file dialog is brought up; the only effect of |expectation| is determining whether the test passes/fails/etc. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:315: observer->WaitForFinished(); I'd make clear in comments that we're blocking for the navigation in NavigateToURLWithDisposition and blocking for the download completion in WaitForFinished(). (It's fine to sketch that all out in the one comment above.) http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:368: // to be checked. I think the usual format for this is "TODO(<your ldap>): ..." to indicate that this is something that isn't being done and should be in the future. The above comment isn't clear as to whether it's describing a requirement on the test that it satisfies, or a todo for the future. An alternative would be "Note that |expected_title_in_progress| and |expected_title_finished| are currently ignored; see TODO below." http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:379: // TODO(tc): check download status text before downloading. I think since the text is primarily yours (?), the name should be yours as well. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:530: // The test is considered flaky due to bug http://crbug.com/26325. Obviously, remove references to 26325. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:553: EXPECT_EQ(2, browser()->tab_count()); It would seem natural to this test to check that the shelf is visible here. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:572: // "download-finish". At that time, the download will finish. I'd add "These tests don't currently test much due to holes in RunSizeTest; see comments in that routine for details." http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:579: IN_PROC_BROWSER_TEST_F(DownloadTest, FLAKY_UnknownSize) { For what it's worth, I believe these tests won't normally trip over 63237 (the slow download will generally allow the history system to catch up), which may be why they're marked flaky rather than disabled. I think it's still possible that sometimes the history system will be wicked slow, and we'll still time out. But these tests should usually work properly. I.e. I don't think FLAKY_ is right; I'd still vote for DISABLED_ (since I think a timeout *could* happen) but we could also enable these. Obviously, we want to watch the flakiness dashboard after committing unless we check them in DISABLED_. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:629: PopBrowser(); I'd call this below, near CloseWindow(), just to keep reality and the browser list in alignment. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:651: Not a requirement for this CL, but I invite you to think about slightly more descriptive suffixes than the [123] we've got below and to change the names of the tests if you do. The suffixes are confusing, especially because we're often dealing with some number of tabs, which sometimes match the suffix and sometimes don't :-J. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:660: IN_PROC_BROWSER_TEST_F(DownloadTest, FLAKY_DontCloseNewTab1) { I can't seen any reason why this test should be flaky--it doesn't download. If it is flaky, that's worth knowing. I'd check it in enabled and watch the flakiness dashboard. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:662: // Because it's an HTML link, it should open a tab rather than downloading. This comment reads weirdly in combination with the NEW_BACKGROUND_TAB below. The comment implies that there's code deep in the guts of chromium that is being tested that checks the nature of the URL, and opens a new tab because it's an HTML link. But the NEW_BACKGROUND_TAB below implies to me "Open a new tab unilaterally". If I look at the original UI test, it didn't have this comment; I'd remove it. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:682: // subsystem in in http://crbug.com/63237. nit: "in in" http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:684: IN_PROC_BROWSER_TEST_F(DownloadTest, FLAKY_CloseNewTab1) { Can't imagine why this wouldn't timeout via 62637. Remember that the FLAKY/DISABLED settings were colliding with the transition Pawel made to turn the bots red on timeouts for flaky tests, so tests that were only occasionally failing might have been marked FLAKY. But this all pretty much comes down to "we're going to need to be watching the flakiness dashboard for these after commit" :-J :-J. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:705: // Open a web page, then download a file in another tab via a Javascript call. Elsewhere current tab, foreground tab, background tab is specified along with "open a web page"; I'd suggest doing that here as well (possibly specifying that that decision is made automatically by the system here). Also, a later comments specifically calls out that the javascript opens a temporary tab, so that should probably be done here for consistency (and because it's relevant to the main point of the test, which is how many tabs are left). http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:737: // then download a file in the new tab. I'd suggest that in general, when the test relies on the "temporary tab" nature of javascript:openNew() that that be called out. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:811: // download a file in a new tab. Close the new tab when the download is done. Is the new tab closed by the test? I don't see the code to do so below--is it in the javascript associated with the form on the download page? (Mostly me trying to understand the comment, but it wouldn't hurt to answer the question in the comment as well, for the next person trying to understand.) http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:874: Browser* download_browser = ui_test_utils::GetBrowser(1); Does GetBrowser() give a guarantee that we get the windows in order of creation? If not, I think getting the list and removing the known browser from it is the right thing. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:895: IN_PROC_BROWSER_TEST_F(DownloadTest, DISABLED_NewWindow) { This test looks like a strict superset of DontCloseNewWindow. If you agree with that analysis, I'd nuke DontCloseNewWindow. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:905: ui_test_utils::BROWSER_TEST_NONE); Micro-nit: Consistency is a win, so in the absence of hitting the 80 column limit I'd format these lines the same (either starting on the same line as the call or starting on the next line) throughout this file. But saying I don't care a lot is a bit of an overstatement; this is more making you aware of the possibility of changing it than an actual request :-}. http://codereview.chromium.org/5610006/diff/15001/chrome/test/ui_test_utils.cc File chrome/test/ui_test_utils.cc (right): http://codereview.chromium.org/5610006/diff/15001/chrome/test/ui_test_utils.c... chrome/test/ui_test_utils.cc:508: browser = WaitForNewBrowserWithCount(browser_count); It's a judgment call, but I think reassigning browser here is a bit dangerous; it's the only place in the function that that assignment happens, and it only happens on one particular flag, so someone scanning the function could easily miss this, assume browser is the same from the beginning, and make some incorrect change on that basis. Not a big deal, but possibly worth changing. http://codereview.chromium.org/5610006/diff/15001/chrome/test/ui_test_utils.h File chrome/test/ui_test_utils.h (right): http://codereview.chromium.org/5610006/diff/15001/chrome/test/ui_test_utils.h... chrome/test/ui_test_utils.h:65: BROWSER_TEST_WAIT_FOR_TAB = 1 << 1, // Wait for a new tab. So just confirming: If I WAIT_FOR_TAB but don't WAIT_FOR_NAVIGATION, the wait will return when the tab is created but before any specified navigation occurs? http://codereview.chromium.org/5610006/diff/15001/chrome/test/ui_test_utils.h... chrome/test/ui_test_utils.h:109: Browser* WaitForAnyNewTab(); This is only used in one place, which is inside ui_test_utils.cc; it should probably be declared and defined in an anonymous namespace in that file. Ditto for WaitForNewBrowserWithCount(). Ditto for NavigateToURLWithDispositionBlockUntilNavigationComplete() (used twice, but both in ui_test_utils.cc). http://codereview.chromium.org/5610006/diff/15001/chrome/test/ui_test_utils.h... chrome/test/ui_test_utils.h:140: // |disposition| indicates what tab the download occurs in, and nit: This call isn't about downloads specifically. http://codereview.chromium.org/5610006/diff/15001/chrome/test/ui_test_utils.h... chrome/test/ui_test_utils.h:154: // blocking until the |number_of_navigations| specified complete. This is a bit confusing, at least to an untutored eye. How can a URL imply more than one navigation? Or to put it differently, what does a single navigation consist of?
http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:244: ASSERT_TRUE(CreateAndSetDownloadsDirectory()); On 2010/12/13 21:44:58, rdsmith wrote: > My understanding is that we can't use ASSERT_* inside of helper functions as per > our discussion. So this may need to be rewritten--I'll wait on your talking > with Chris. I removed most of the ASSERT's, including all that were in helper functions. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:248: false); On 2010/12/13 21:44:58, rdsmith wrote: > Is there ever a case where we *don't* want to specify whether we prompt for > download or not? DisableMIMETypeSelect wants us always to prompt; are there any > other cases where we want to allow prompting at all? If not, I'd make > force_on_prompt into just the argument provided to the SetBoolean; that allows > you to remove all uses of kPromptForDownload in all tests and just replace it > with the correct arg to InitialSetup(). There is still one instance where it has to occur in the tests themselves -- in the case of the Incognito download. Done. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:256: void PopBrowser() { browsers_.pop(); } On 2010/12/13 21:44:58, rdsmith wrote: > If you take my suggestion below about giving CreateAndSetDownloadsDirectory a > Browser* argument, I think the need for these calls goes away, which I think is > good, as people used to what they mean in other browser tests will be confused > by the difference here. It's also needed for |CreateWaiter()|, which is called via |DownloadAndWait()|. I expect this to be used more when we have other tests. I think of |browser()| as returning the current browser/window that we're working with. When we open a new window to download into, that changes. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:258: virtual Browser* browser() const { return browsers_.top(); } On 2010/12/13 21:44:58, rdsmith wrote: > I'd suggest browsers_.empty() -> return NULL; Done. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:266: bool CreateAndSetDownloadsDirectory() { On 2010/12/13 21:44:58, rdsmith wrote: > The two usages of this function are in InitialSetup() and in IncognitoDownload. > In both cases we've just before the call been playing with the browser pointer > we want for the call, and in IncognitoDownload we're doing a PushBrowser() just > before to make sure that this call has the right browser (in a way that, to me, > feels a little indirect ans susceptible to future error; you need to understand > the innards of both CreateAndSetDownloadsDirectory() and PushBrowser() at the > point of call in IncognitoDownload). I'd recommend just making this call take a > browser argument. I'd have to make other functions take a browser argument too, and pass it around. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:272: bool SetDownloadsDirectoryForBrowser(Browser* browser) { On 2010/12/13 21:44:58, rdsmith wrote: > The only use I can find of SetDownloadsDirectoryForBrowser is above, in > CreateAndSetDownloadsDirectory. If that's it, I think these two routines should > be collapsed into one, at least until we need the separate functionality. Done. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:300: // |disposition| indicates where the naiation occurs (current tab, new On 2010/12/13 21:44:58, rdsmith wrote: > Nit: "navigation" Done. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:307: SelectExpectation expectation, On 2010/12/13 21:44:58, rdsmith wrote: > |expectation| isn't mentioned in the comments for the routine and should be. > The comment for that arg should mention that this routine will always unblock if > the select file dialog is brought up; the only effect of |expectation| is > determining whether the test passes/fails/etc. Done. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:315: observer->WaitForFinished(); On 2010/12/13 21:44:58, rdsmith wrote: > I'd make clear in comments that we're blocking for the navigation in > NavigateToURLWithDisposition and blocking for the download completion in > WaitForFinished(). (It's fine to sketch that all out in the one comment above.) Done. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:379: // TODO(tc): check download status text before downloading. On 2010/12/13 21:44:58, rdsmith wrote: > I think since the text is primarily yours (?), the name should be yours as well. Done. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:530: // The test is considered flaky due to bug http://crbug.com/26325. On 2010/12/13 21:44:58, rdsmith wrote: > Obviously, remove references to 26325. Done. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:553: EXPECT_EQ(2, browser()->tab_count()); On 2010/12/13 21:44:58, rdsmith wrote: > It would seem natural to this test to check that the shelf is visible here. Done. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:572: // "download-finish". At that time, the download will finish. On 2010/12/13 21:44:58, rdsmith wrote: > I'd add "These tests don't currently test much due to holes in RunSizeTest; see > comments in that routine for details." > Done. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:579: IN_PROC_BROWSER_TEST_F(DownloadTest, FLAKY_UnknownSize) { On 2010/12/13 21:44:58, rdsmith wrote: > For what it's worth, I believe these tests won't normally trip over 63237 (the > slow download will generally allow the history system to catch up), which may be > why they're marked flaky rather than disabled. I think it's still possible that > sometimes the history system will be wicked slow, and we'll still time out. But > these tests should usually work properly. I.e. I don't think FLAKY_ is right; > I'd still vote for DISABLED_ (since I think a timeout *could* happen) but we > could also enable these. > > Obviously, we want to watch the flakiness dashboard after committing unless we > check them in DISABLED_. I'm going to enable them, then. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:629: PopBrowser(); On 2010/12/13 21:44:58, rdsmith wrote: > I'd call this below, near CloseWindow(), just to keep reality and the browser > list in alignment. Done. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:660: IN_PROC_BROWSER_TEST_F(DownloadTest, FLAKY_DontCloseNewTab1) { On 2010/12/13 21:44:58, rdsmith wrote: > I can't seen any reason why this test should be flaky--it doesn't download. If > it is flaky, that's worth knowing. I'd check it in enabled and watch the > flakiness dashboard. Done. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:662: // Because it's an HTML link, it should open a tab rather than downloading. On 2010/12/13 21:44:58, rdsmith wrote: > This comment reads weirdly in combination with the NEW_BACKGROUND_TAB below. > The comment implies that there's code deep in the guts of chromium that is being > tested that checks the nature of the URL, and opens a new tab because it's an > HTML link. But the NEW_BACKGROUND_TAB below implies to me "Open a new tab > unilaterally". If I look at the original UI test, it didn't have this comment; > I'd remove it. Changed the comments, here and elsewhere. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:682: // subsystem in in http://crbug.com/63237. On 2010/12/13 21:44:58, rdsmith wrote: > nit: "in in" I don't understand. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:705: // Open a web page, then download a file in another tab via a Javascript call. On 2010/12/13 21:44:58, rdsmith wrote: > Elsewhere current tab, foreground tab, background tab is specified along with > "open a web page"; I'd suggest doing that here as well (possibly specifying that > that decision is made automatically by the system here). > > Also, a later comments specifically calls out that the javascript opens a > temporary tab, so that should probably be done here for consistency (and because > it's relevant to the main point of the test, which is how many tabs are left). > Added "current tab", but no temporary tab is created. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:737: // then download a file in the new tab. On 2010/12/13 21:44:58, rdsmith wrote: > I'd suggest that in general, when the test relies on the "temporary tab" nature > of javascript:openNew() that that be called out. The |openNew()| function is embedded in the web page, and varies. It doesn't always open a temporary tab. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:811: // download a file in a new tab. Close the new tab when the download is done. On 2010/12/13 21:44:58, rdsmith wrote: > Is the new tab closed by the test? I don't see the code to do so below--is it > in the javascript associated with the form on the download page? > > (Mostly me trying to understand the comment, but it wouldn't hurt to answer the > question in the comment as well, for the next person trying to understand.) Done. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:874: Browser* download_browser = ui_test_utils::GetBrowser(1); On 2010/12/13 21:44:58, rdsmith wrote: > Does GetBrowser() give a guarantee that we get the windows in order of creation? > If not, I think getting the list and removing the known browser from it is the > right thing. In this particular case, it's guaranteed. If you open and close browsers randomly, it won't be. http://codereview.chromium.org/5610006/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:905: ui_test_utils::BROWSER_TEST_NONE); On 2010/12/13 21:44:58, rdsmith wrote: > Micro-nit: Consistency is a win, so in the absence of hitting the 80 column > limit I'd format these lines the same (either starting on the same line as the > call or starting on the next line) throughout this file. But saying I don't > care a lot is a bit of an overstatement; this is more making you aware of the > possibility of changing it than an actual request :-}. I personally prefer this method of lining up variables; I find it easier to read. I only go to the 'indent 4 spaces' method when i run into the 80-character limit. http://codereview.chromium.org/5610006/diff/15001/chrome/test/ui_test_utils.h File chrome/test/ui_test_utils.h (right): http://codereview.chromium.org/5610006/diff/15001/chrome/test/ui_test_utils.h... chrome/test/ui_test_utils.h:65: BROWSER_TEST_WAIT_FOR_TAB = 1 << 1, // Wait for a new tab. On 2010/12/13 21:44:58, rdsmith wrote: > So just confirming: If I WAIT_FOR_TAB but don't WAIT_FOR_NAVIGATION, the wait > will return when the tab is created but before any specified navigation occurs? Correct. http://codereview.chromium.org/5610006/diff/15001/chrome/test/ui_test_utils.h... chrome/test/ui_test_utils.h:109: Browser* WaitForAnyNewTab(); On 2010/12/13 21:44:58, rdsmith wrote: > This is only used in one place, which is inside ui_test_utils.cc; it should > probably be declared and defined in an anonymous namespace in that file. Ditto > for WaitForNewBrowserWithCount(). Ditto for > NavigateToURLWithDispositionBlockUntilNavigationComplete() (used twice, but both > in ui_test_utils.cc). > Moved. I do think it could be used by other files though. http://codereview.chromium.org/5610006/diff/15001/chrome/test/ui_test_utils.h... chrome/test/ui_test_utils.h:140: // |disposition| indicates what tab the download occurs in, and On 2010/12/13 21:44:58, rdsmith wrote: > nit: This call isn't about downloads specifically. Done. http://codereview.chromium.org/5610006/diff/15001/chrome/test/ui_test_utils.h... chrome/test/ui_test_utils.h:154: // blocking until the |number_of_navigations| specified complete. On 2010/12/13 21:44:58, rdsmith wrote: > This is a bit confusing, at least to an untutored eye. How can a URL imply more > than one navigation? Or to put it differently, what does a single navigation > consist of? Yeah, I found it a bit odd too -- I copied the comment from |NavigateToURLBlockUntilNavigationsComplete|. One possibility is that a navigation might cause other navigations to occur (for example, if the URL is a Javascript call).
Here's the next round. There are a couple of things I think it'd be useful to talk about offline (the references to "temporary tabs", the GetBrowser() ordering, and the discussion around the browser() override); let's try and do that tomorrow. But this is looking good. I'm going to be very psyched when this goes in. http://codereview.chromium.org/5610006/diff/21001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/5610006/diff/21001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:233: // Sanity check default values for window / tab count and shelf visibility. Suggest moving comment down one line; it doesn't describe what's happening in PushBrowser(). http://codereview.chromium.org/5610006/diff/21001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:260: EXPECT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &test_dir_)); Given that there's no way to propagate the success or failure of this call up the stack to abort the test if it fails, I'd put an ASSERT_* (or the moral equivalent, i.e. an EXPECT_* followed by a return false, and an ASSERT_TRUE at top test level) that test_dir_ was initialized correctly somewhere. If this fails, everything else is gonna barf all over the floor, which sorta defines what needs to be ASSERT_'d to me. http://codereview.chromium.org/5610006/diff/21001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:266: if (browser()) { Nit/suggestion: if (!browser()) return false; ... That'd allow the rest of the routine not to be indented. http://codereview.chromium.org/5610006/diff/21001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:296: // foreground tab, etc). Nit: You left out url. It's pretty obvious what it does, so it's not worth many words; maybe just change the first phrase to "Download |url|"? (It feels weird to list all the args except that one.) http://codereview.chromium.org/5610006/diff/21001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:348: EXPECT_TRUE(file_util::PathExists(origin_file)); The pattern I'd suggest for things that you'd like to be assertions in helper functions is: a) Make the helper function return a bool for success/failure. b) Test right after the expect for success/failure of whatever the expect tests, and return false if it fails. c) Assert in the calling code that the helper function returns true. I don't think it's really important; primarily it's about reducing noise in test output in the case of a failure. But I think it's a good habit to get into, in case one of the proto-assertions not being true later leads to a crash or timeout, which are test failure types that we'd really like to avoid. http://codereview.chromium.org/5610006/diff/21001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:370: // |expected_title_in_finished| need to be checked. Thanks! http://codereview.chromium.org/5610006/diff/21001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:403: // TODO(ahendrickson): check download status text after downloading. Nit: This looks to be a duplicate with the above comment. http://codereview.chromium.org/5610006/diff/21001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:580: IN_PROC_BROWSER_TEST_F(DownloadTest, FLAKY_UnknownSize) { I think we've agreed to bring references to 63237 and FLAKY_/DISABLED_ into alignment, i.e. if it refers to 63237 in all its naked glory, it's disabled, but and if it doesn't (or does in a "but we shouldn't trip over it" fashion) it's enabled/flaky (depending on what other bugs are referenced). Could you either execute that comment/category normalization or grab me and tell me why we shouldn't? (Another way to put this is that the 63237 boilerplate should include words like "will cause a timeout and thus this test must be disabled" and that tests with that unmodified boilerplate should just be disabled. I know of no way that 63237 could just produce a failed test without a timeout.) http://codereview.chromium.org/5610006/diff/21001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:681: // subsystem in in http://crbug.com/63237. Nit: The word "in" is doubled in the preceding sentence. http://codereview.chromium.org/5610006/diff/21001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:687: // closed when the download is done. Is the tab automatically closed when the download is done, or when the system realizes that it's a download? I'm not sure that it's the latter, but I thought it was. http://codereview.chromium.org/5610006/diff/21001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:858: IN_PROC_BROWSER_TEST_F(DownloadTest, DISABLED_DontCloseNewWindow) { Is there a reason why you didn't respond to my earlier comment about the relationship between this test and NewWindow?
http://codereview.chromium.org/5610006/diff/21001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/5610006/diff/21001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:233: // Sanity check default values for window / tab count and shelf visibility. On 2010/12/19 23:52:49, rdsmith wrote: > Suggest moving comment down one line; it doesn't describe what's happening in > PushBrowser(). Done. http://codereview.chromium.org/5610006/diff/21001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:260: EXPECT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &test_dir_)); On 2010/12/19 23:52:49, rdsmith wrote: > Given that there's no way to propagate the success or failure of this call up > the stack to abort the test if it fails, I'd put an ASSERT_* (or the moral > equivalent, i.e. an EXPECT_* followed by a return false, and an ASSERT_TRUE at > top test level) that test_dir_ was initialized correctly somewhere. If this > fails, everything else is gonna barf all over the floor, which sorta defines > what needs to be ASSERT_'d to me. Unfortunately, this is an inherited function that returns void. I moved this to |InitialSetup()|, and made that return a boolean. |RunSizeTest()| is also affected. http://codereview.chromium.org/5610006/diff/21001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:266: if (browser()) { On 2010/12/19 23:52:49, rdsmith wrote: > Nit/suggestion: if (!browser()) return false; ... > That'd allow the rest of the routine not to be indented. Done. http://codereview.chromium.org/5610006/diff/21001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:296: // foreground tab, etc). On 2010/12/19 23:52:49, rdsmith wrote: > Nit: You left out url. It's pretty obvious what it does, so it's not worth many > words; maybe just change the first phrase to "Download |url|"? (It feels weird > to list all the args except that one.) Done. http://codereview.chromium.org/5610006/diff/21001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:348: EXPECT_TRUE(file_util::PathExists(origin_file)); On 2010/12/19 23:52:49, rdsmith wrote: > The pattern I'd suggest for things that you'd like to be assertions in helper > functions is: > a) Make the helper function return a bool for success/failure. > b) Test right after the expect for success/failure of whatever the expect tests, > and return false if it fails. > c) Assert in the calling code that the helper function returns true. > > I don't think it's really important; primarily it's about reducing noise in test > output in the case of a failure. But I think it's a good habit to get into, in > case one of the proto-assertions not being true later leads to a crash or > timeout, which are test failure types that we'd really like to avoid. Done. http://codereview.chromium.org/5610006/diff/21001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:403: // TODO(ahendrickson): check download status text after downloading. On 2010/12/19 23:52:49, rdsmith wrote: > Nit: This looks to be a duplicate with the above comment. One says "before", and the other says "after". http://codereview.chromium.org/5610006/diff/21001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:580: IN_PROC_BROWSER_TEST_F(DownloadTest, FLAKY_UnknownSize) { On 2010/12/19 23:52:49, rdsmith wrote: > I think we've agreed to bring references to 63237 and FLAKY_/DISABLED_ into > alignment, i.e. if it refers to 63237 in all its naked glory, it's disabled, but > and if it doesn't (or does in a "but we shouldn't trip over it" fashion) it's > enabled/flaky (depending on what other bugs are referenced). Could you either > execute that comment/category normalization or grab me and tell me why we > shouldn't? > > (Another way to put this is that the 63237 boilerplate should include words like > "will cause a timeout and thus this test must be disabled" and that tests with > that unmodified boilerplate should just be disabled. I know of no way that > 63237 could just produce a failed test without a timeout.) Hmm, we could put in code that gets around the hang for 63237 by cleaning up the specific files we use, right? Is this better or worse than marking the tests DISABLED? http://codereview.chromium.org/5610006/diff/21001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:681: // subsystem in in http://crbug.com/63237. On 2010/12/19 23:52:49, rdsmith wrote: > Nit: The word "in" is doubled in the preceding sentence. Done. http://codereview.chromium.org/5610006/diff/21001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:687: // closed when the download is done. On 2010/12/19 23:52:49, rdsmith wrote: > Is the tab automatically closed when the download is done, or when the system > realizes that it's a download? I'm not sure that it's the latter, but I thought > it was. Hmm, actually I'm not sure. http://codereview.chromium.org/5610006/diff/21001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:858: IN_PROC_BROWSER_TEST_F(DownloadTest, DISABLED_DontCloseNewWindow) { On 2010/12/19 23:52:49, rdsmith wrote: > Is there a reason why you didn't respond to my earlier comment about the > relationship between this test and NewWindow? Sorry, that was an oversight. You are correct that one is a superset of the other. I'll remove the "Dont" version.
Now ready for review.
NOW it's ready for review $-).
http://codereview.chromium.org/5610006/diff/29001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/5610006/diff/29001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:40: } while(false) Include a comment about how this macro will be used and in what contexts. (You might want to simplify it down to: EXPECT_TRUE(test); if (!(test)) return false; with appropriate curlies, that would probably be easier for other people reading the code to understand. But I'd like the comment either way.) http://codereview.chromium.org/5610006/diff/29001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:267: return browsers_.empty() ? NULL : browsers_.top(); I think we agreed that overall it made sense to remove this abstraction? (too much confusion between the InProcessBrowserTest interface, the DownloadTest interface, and the local tests) http://codereview.chromium.org/5610006/diff/29001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:920: Browser* download_browser = ui_test_utils::GetBrowser(1); For the reasons we discussed (reliance on the internals of GetBrowser() without a guarantee in the routine specification) I'd be more comfortable here if you got both browsers, figured out which one you already had (in browser()), and probed at the other one rather than doing a GetBrowser(1). Alternatively, you could specify what the index to GetBrowser() means in a comment where it's declared (e.g. in strict order by creation) but then make sure that it's an accurate specification :-}.
http://codereview.chromium.org/5610006/diff/29001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/5610006/diff/29001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:33: #define MOCK_ASSERT_TRUE(test) \ I'd strongly prefer to avoid that macro. Instead, I'd suggest something like: if (!test) { ADD_FAILURE(); return false; } http://codereview.chromium.org/5610006/diff/29001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:241: MOCK_ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &test_dir_)); Alternatively, you can do if (!PathService::Get(...)) return false here, and ASSERT in the caller. Yet another alternative is to make this method void, use ASSERTs inside, and then ASSERT_NO_FATAL_FAILURE in the caller. http://codereview.chromium.org/5610006/diff/29001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:262: // We now use a browser stack. nit: It would deserve some more explanations. It's not obvious to me why this is needed without reading more code. http://codereview.chromium.org/5610006/diff/29001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:266: virtual Browser* browser() const { Nooooo! Don't do that, it's going to be super-confusing. http://codereview.chromium.org/5610006/diff/29001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:440: std::stack<Browser *> browsers_; nit: No space between Browser and "*". Please add a comment. http://codereview.chromium.org/5610006/diff/29001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:443: // NOTES: nit: Those notes are not download test specific, and are implementation-specific (DIR_TEST_DATA might change). I'd suggest to remove them. http://codereview.chromium.org/5610006/diff/29001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:563: EXPECT_NE(static_cast<TabContentsWrapper *>(NULL), nit: No space before "*", please make sure to fix the entire CL. http://codereview.chromium.org/5610006/diff/29001/chrome/test/in_process_brow... File chrome/test/in_process_browser_test.h (right): http://codereview.chromium.org/5610006/diff/29001/chrome/test/in_process_brow... chrome/test/in_process_browser_test.h:125: // Creates an incognito browser; nit: ";" -> "." http://codereview.chromium.org/5610006/diff/29001/chrome/test/ui_test_utils.cc File chrome/test/ui_test_utils.cc (right): http://codereview.chromium.org/5610006/diff/29001/chrome/test/ui_test_utils.c... chrome/test/ui_test_utils.cc:424: void WaitForWindowClosed(Browser* browser) { I'd strongly prefer to add another variant of WaitForNotification that takes a NotificationSource rather than create hundreds of functions for every notification. http://codereview.chromium.org/5610006/diff/29001/chrome/test/ui_test_utils.c... chrome/test/ui_test_utils.cc:465: Browser* WaitForNewBrowserWithCount(size_t start_count) { This function seems quite confusing. Can we avoid adding it to ui_test_utils? http://codereview.chromium.org/5610006/diff/29001/chrome/test/ui_test_utils.c... chrome/test/ui_test_utils.cc:525: if (!(browser_test_flags & BROWSER_TEST_WAIT_FOR_NAVIGATION)) This seems to contradict the function's name. :-( Can we fix that? http://codereview.chromium.org/5610006/diff/29001/chrome/test/ui_test_utils.c... chrome/test/ui_test_utils.cc:705: Browser* GetBrowser(int index) { This seems more like something to be added to BrowserList, and I'm not sure whether we want that (i.e. whether the index will always stay the same, etc, etc). http://codereview.chromium.org/5610006/diff/29001/chrome/test/ui_test_utils.h File chrome/test/ui_test_utils.h (right): http://codereview.chromium.org/5610006/diff/29001/chrome/test/ui_test_utils.h... chrome/test/ui_test_utils.h:62: enum BrowserTestFlags { nit: Choose a more specific name than BrowserTestFlags.
http://codereview.chromium.org/5610006/diff/29001/chrome/test/in_process_brow... File chrome/test/in_process_browser_test.h (right): http://codereview.chromium.org/5610006/diff/29001/chrome/test/in_process_brow... chrome/test/in_process_browser_test.h:125: // Creates an incognito browser; This comment is useless, it just repeats the function name. Perhaps it should say "Like CreateBrowser but creates an incognito window." which implies all the navigation and waiting that CreateBrowser has.
http://codereview.chromium.org/5610006/diff/29001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/5610006/diff/29001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:33: #define MOCK_ASSERT_TRUE(test) \ On 2011/01/03 09:06:47, Paweł Hajdan Jr. wrote: > I'd strongly prefer to avoid that macro. Instead, I'd suggest something like: > > if (!test) { > ADD_FAILURE(); > return false; > } I'd rather have information about the failure in the output log. http://codereview.chromium.org/5610006/diff/29001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:40: } while(false) On 2010/12/23 18:00:34, rdsmith wrote: > Include a comment about how this macro will be used and in what contexts. > > (You might want to simplify it down to: > > EXPECT_TRUE(test); > if (!(test)) return false; > > with appropriate curlies, that would probably be easier for other people reading > the code to understand. But I'd like the comment either way.) I'm doing it this way so |test| doesn't get evaluated twice. Adding the comment. http://codereview.chromium.org/5610006/diff/29001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:262: // We now use a browser stack. On 2011/01/03 09:06:47, Paweł Hajdan Jr. wrote: > nit: It would deserve some more explanations. It's not obvious to me why this is > needed without reading more code. Removing the browser stack. http://codereview.chromium.org/5610006/diff/29001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:266: virtual Browser* browser() const { On 2011/01/03 09:06:47, Paweł Hajdan Jr. wrote: > Nooooo! Don't do that, it's going to be super-confusing. Removing the browser stack. http://codereview.chromium.org/5610006/diff/29001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:267: return browsers_.empty() ? NULL : browsers_.top(); On 2010/12/23 18:00:34, rdsmith wrote: > I think we agreed that overall it made sense to remove this abstraction? (too > much confusion between the InProcessBrowserTest interface, the DownloadTest > interface, and the local tests) > > Removing the browser stack. http://codereview.chromium.org/5610006/diff/29001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:440: std::stack<Browser *> browsers_; On 2011/01/03 09:06:47, Paweł Hajdan Jr. wrote: > nit: No space between Browser and "*". Please add a comment. Removing the browser stack. http://codereview.chromium.org/5610006/diff/29001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:443: // NOTES: On 2011/01/03 09:06:47, Paweł Hajdan Jr. wrote: > nit: Those notes are not download test specific, and are implementation-specific > (DIR_TEST_DATA might change). I'd suggest to remove them. Changed the note's text. I needed this information to figure out what the tests were doing, and it was hard finding these files. I'd like to keep the information here. http://codereview.chromium.org/5610006/diff/29001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:563: EXPECT_NE(static_cast<TabContentsWrapper *>(NULL), On 2011/01/03 09:06:47, Paweł Hajdan Jr. wrote: > nit: No space before "*", please make sure to fix the entire CL. The style guide is ambiguous about this, so I've always done it this way in templates. However, a search reveals that most of the Chrome code doesn't have the space so I'll change it. http://codereview.chromium.org/5610006/diff/29001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:920: Browser* download_browser = ui_test_utils::GetBrowser(1); On 2010/12/23 18:00:34, rdsmith wrote: > For the reasons we discussed (reliance on the internals of GetBrowser() without > a guarantee in the routine specification) I'd be more comfortable here if you > got both browsers, figured out which one you already had (in browser()), and > probed at the other one rather than doing a GetBrowser(1). Alternatively, you > could specify what the index to GetBrowser() means in a comment where it's > declared (e.g. in strict order by creation) but then make sure that it's an > accurate specification :-}. > Done. http://codereview.chromium.org/5610006/diff/29001/chrome/test/in_process_brow... File chrome/test/in_process_browser_test.h (right): http://codereview.chromium.org/5610006/diff/29001/chrome/test/in_process_brow... chrome/test/in_process_browser_test.h:125: // Creates an incognito browser; On 2011/01/03 09:06:47, Paweł Hajdan Jr. wrote: > nit: ";" -> "." Done. http://codereview.chromium.org/5610006/diff/29001/chrome/test/in_process_brow... chrome/test/in_process_browser_test.h:125: // Creates an incognito browser; On 2011/01/04 18:44:12, brettw wrote: > This comment is useless, it just repeats the function name. Perhaps it should > say "Like CreateBrowser but creates an incognito window." which implies all the > navigation and waiting that CreateBrowser has. Done. http://codereview.chromium.org/5610006/diff/29001/chrome/test/ui_test_utils.cc File chrome/test/ui_test_utils.cc (right): http://codereview.chromium.org/5610006/diff/29001/chrome/test/ui_test_utils.c... chrome/test/ui_test_utils.cc:424: void WaitForWindowClosed(Browser* browser) { On 2011/01/03 09:06:47, Paweł Hajdan Jr. wrote: > I'd strongly prefer to add another variant of WaitForNotification that takes a > NotificationSource rather than create hundreds of functions for every > notification. Done. http://codereview.chromium.org/5610006/diff/29001/chrome/test/ui_test_utils.c... chrome/test/ui_test_utils.cc:465: Browser* WaitForNewBrowserWithCount(size_t start_count) { On 2011/01/03 09:06:47, Paweł Hajdan Jr. wrote: > This function seems quite confusing. Can we avoid adding it to ui_test_utils? Replaced the function. http://codereview.chromium.org/5610006/diff/29001/chrome/test/ui_test_utils.c... chrome/test/ui_test_utils.cc:525: if (!(browser_test_flags & BROWSER_TEST_WAIT_FOR_NAVIGATION)) On 2011/01/03 09:06:47, Paweł Hajdan Jr. wrote: > This seems to contradict the function's name. :-( Can we fix that? Added a comment explaining this. http://codereview.chromium.org/5610006/diff/29001/chrome/test/ui_test_utils.c... chrome/test/ui_test_utils.cc:705: Browser* GetBrowser(int index) { On 2011/01/03 09:06:47, Paweł Hajdan Jr. wrote: > This seems more like something to be added to BrowserList, and I'm not sure > whether we want that (i.e. whether the index will always stay the same, etc, > etc). No longer using this -- replaced with a more appropriate function. http://codereview.chromium.org/5610006/diff/29001/chrome/test/ui_test_utils.h File chrome/test/ui_test_utils.h (right): http://codereview.chromium.org/5610006/diff/29001/chrome/test/ui_test_utils.h... chrome/test/ui_test_utils.h:62: enum BrowserTestFlags { On 2011/01/03 09:06:47, Paweł Hajdan Jr. wrote: > nit: Choose a more specific name than BrowserTestFlags. Done.
http://codereview.chromium.org/5610006/diff/51001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/5610006/diff/51001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:40: #define MOCK_ASSERT_TRUE(test) \ If you really want some kind of macro here, I think we should add it in a separate CL, in a generic location (maybe base/test? Brett may have more ideas). This is a quite common scenario in our tests (ASSERT-like behavior in non-void function/method), and I'd like to avoid everyone implementing his own version of the macro, with slightly different name and behavior. After adding the macro in some utilities file, we should then convert all tests to use it. It's going to be a large change, so it'd be nice to discuss it on chromium-dev first. Oh, and note I suggested a few alternatives other than the macro and ADD_FAILURE, so this is not the only way it can be done. http://codereview.chromium.org/5610006/diff/51001/chrome/test/in_process_brow... File chrome/test/in_process_browser_test.h (right): http://codereview.chromium.org/5610006/diff/51001/chrome/test/in_process_brow... chrome/test/in_process_browser_test.h:73: virtual Browser* browser() const { return browser_; } nit: This shouldn't be needed now, right? http://codereview.chromium.org/5610006/diff/51001/chrome/test/ui_test_utils.cc File chrome/test/ui_test_utils.cc (right): http://codereview.chromium.org/5610006/diff/51001/chrome/test/ui_test_utils.c... chrome/test/ui_test_utils.cc:424: NotificationType::BROWSER_CLOSED, Please see my earlier comment about this.
http://codereview.chromium.org/5610006/diff/51001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/5610006/diff/51001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:40: #define MOCK_ASSERT_TRUE(test) \ On 2011/01/05 08:05:52, Paweł Hajdan Jr. wrote: > If you really want some kind of macro here, I think we should add it in a > separate CL, in a generic location (maybe base/test? Brett may have more ideas). > > This is a quite common scenario in our tests (ASSERT-like behavior in non-void > function/method), and I'd like to avoid everyone implementing his own version of > the macro, with slightly different name and behavior. > > After adding the macro in some utilities file, we should then convert all tests > to use it. It's going to be a large change, so it'd be nice to discuss it on > chromium-dev first. > > Oh, and note I suggested a few alternatives other than the macro and > ADD_FAILURE, so this is not the only way it can be done. Done. http://codereview.chromium.org/5610006/diff/51001/chrome/test/in_process_brow... File chrome/test/in_process_browser_test.h (right): http://codereview.chromium.org/5610006/diff/51001/chrome/test/in_process_brow... chrome/test/in_process_browser_test.h:73: virtual Browser* browser() const { return browser_; } On 2011/01/05 08:05:52, Paweł Hajdan Jr. wrote: > nit: This shouldn't be needed now, right? You are correct. Removed. http://codereview.chromium.org/5610006/diff/51001/chrome/test/ui_test_utils.cc File chrome/test/ui_test_utils.cc (right): http://codereview.chromium.org/5610006/diff/51001/chrome/test/ui_test_utils.c... chrome/test/ui_test_utils.cc:424: NotificationType::BROWSER_CLOSED, On 2011/01/05 08:05:52, Paweł Hajdan Jr. wrote: > Please see my earlier comment about this. Done.
http://codereview.chromium.org/5610006/diff/58002/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/5610006/diff/58002/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:5: #include <stack> Do we still need this? http://codereview.chromium.org/5610006/diff/58002/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:230: I'd put a comment here that a false return implies setup failure and should be asserted on in the caller. That should probably be done for all the helper routines. http://codereview.chromium.org/5610006/diff/58002/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:274: // outside of that context. This comment is left-over from when the routine called browser() to get the browser and is a little confusing outside of that context. It'd be more accurate to say "browser must be specified by the caller", but even that is pretty obvious from the calling signature and could be left out. http://codereview.chromium.org/5610006/diff/58002/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:298: // values in the BrowserTestWaitFlags enum. I'd refer to this as ui_test_utils::BrowserTestWaitFlags enum so folks know where to look for it if they're curious. http://codereview.chromium.org/5610006/diff/58002/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:337: const FilePath& origin_filename) { For consistency of interface among the different methods of DownloadTest, I'm inclined to think that this should take a browser argument if it needs one rather than relying on browser(). (This is a suggestion, not a requirement--it's test code, and I weigh programmer convenience more highly in the convenience vs. consistency balance in test code.) http://codereview.chromium.org/5610006/diff/58002/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:369: bool downloaded_file_deleted = file_util::DieFileDie(downloaded_file, false); Nit: Make < 80 characters.
http://codereview.chromium.org/5610006/diff/58002/chrome/test/ui_test_utils.cc File chrome/test/ui_test_utils.cc (right): http://codereview.chromium.org/5610006/diff/58002/chrome/test/ui_test_utils.c... chrome/test/ui_test_utils.cc:55: void NavigateToURLWithDispositionBlockUntilNavigationsComplete( nit: This forward-declaration is really weird and doesn't seem to be needed. Can we get rid of it? http://codereview.chromium.org/5610006/diff/58002/chrome/test/ui_test_utils.c... chrome/test/ui_test_utils.cc:443: Browser* new_browser = Source<Browser>(observer.source()).ptr(); nit: Why not just return Source ... ? http://codereview.chromium.org/5610006/diff/58002/chrome/test/ui_test_utils.c... chrome/test/ui_test_utils.cc:450: if (new_browser == NULL) { nit: if (!new_browser), no braces {} http://codereview.chromium.org/5610006/diff/58002/chrome/test/ui_test_utils.c... chrome/test/ui_test_utils.cc:451: new_browser = WaitForNewBrowser(); Should we somehow check it's not in |excluded_browsers|?
http://codereview.chromium.org/5610006/diff/58002/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/5610006/diff/58002/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:5: #include <stack> On 2011/01/06 18:31:17, rdsmith wrote: > Do we still need this? No. http://codereview.chromium.org/5610006/diff/58002/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:230: On 2011/01/06 18:31:17, rdsmith wrote: > I'd put a comment here that a false return implies setup failure and should be > asserted on in the caller. That should probably be done for all the helper > routines. > Done. http://codereview.chromium.org/5610006/diff/58002/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:274: // outside of that context. On 2011/01/06 18:31:17, rdsmith wrote: > This comment is left-over from when the routine called browser() to get the > browser and is a little confusing outside of that context. It'd be more > accurate to say "browser must be specified by the caller", but even that is > pretty obvious from the calling signature and could be left out. Done. http://codereview.chromium.org/5610006/diff/58002/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:298: // values in the BrowserTestWaitFlags enum. On 2011/01/06 18:31:17, rdsmith wrote: > I'd refer to this as ui_test_utils::BrowserTestWaitFlags enum so folks know > where to look for it if they're curious. Done. http://codereview.chromium.org/5610006/diff/58002/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:337: const FilePath& origin_filename) { On 2011/01/06 18:31:17, rdsmith wrote: > For consistency of interface among the different methods of DownloadTest, I'm > inclined to think that this should take a browser argument if it needs one > rather than relying on browser(). (This is a suggestion, not a > requirement--it's test code, and I weigh programmer convenience more highly in > the convenience vs. consistency balance in test code.) Done. http://codereview.chromium.org/5610006/diff/58002/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:369: bool downloaded_file_deleted = file_util::DieFileDie(downloaded_file, false); On 2011/01/06 18:31:17, rdsmith wrote: > Nit: Make < 80 characters. Done. http://codereview.chromium.org/5610006/diff/58002/chrome/test/ui_test_utils.cc File chrome/test/ui_test_utils.cc (right): http://codereview.chromium.org/5610006/diff/58002/chrome/test/ui_test_utils.c... chrome/test/ui_test_utils.cc:55: void NavigateToURLWithDispositionBlockUntilNavigationsComplete( On 2011/01/06 20:45:48, Paweł Hajdan Jr. wrote: > nit: This forward-declaration is really weird and doesn't seem to be needed. Can > we get rid of it? IIRC, it's required for GCC compilation. http://codereview.chromium.org/5610006/diff/58002/chrome/test/ui_test_utils.c... chrome/test/ui_test_utils.cc:443: Browser* new_browser = Source<Browser>(observer.source()).ptr(); On 2011/01/06 20:45:48, Paweł Hajdan Jr. wrote: > nit: Why not just return Source ... ? Done. http://codereview.chromium.org/5610006/diff/58002/chrome/test/ui_test_utils.c... chrome/test/ui_test_utils.cc:450: if (new_browser == NULL) { On 2011/01/06 20:45:48, Paweł Hajdan Jr. wrote: > nit: if (!new_browser), no braces {} Done. http://codereview.chromium.org/5610006/diff/58002/chrome/test/ui_test_utils.c... chrome/test/ui_test_utils.cc:451: new_browser = WaitForNewBrowser(); On 2011/01/06 20:45:48, Paweł Hajdan Jr. wrote: > Should we somehow check it's not in |excluded_browsers|? For what I'm doing, it's not an issue. My understanding is that there is some time between the browser being created and the BROWSER_WINDOW_READY signal. We check for the existence of a new browser first, and then wait for the signal if it doesn't exist yet. However, the way we use it is to wait until the previous window is finished before opening a new one, so only the new window should trigger the wait.
http://codereview.chromium.org/5610006/diff/58002/chrome/test/ui_test_utils.cc File chrome/test/ui_test_utils.cc (right): http://codereview.chromium.org/5610006/diff/58002/chrome/test/ui_test_utils.c... chrome/test/ui_test_utils.cc:451: new_browser = WaitForNewBrowser(); On 2011/01/06 22:33:39, ahendrickson wrote: > On 2011/01/06 20:45:48, Paweł Hajdan Jr. wrote: > > Should we somehow check it's not in |excluded_browsers|? > > For what I'm doing, it's not an issue. > > My understanding is that there is some time between the browser being created > and the BROWSER_WINDOW_READY signal. We check for the existence of a new > browser first, and then wait for the signal if it doesn't exist yet. However, > the way we use it is to wait until the previous window is finished before > opening a new one, so only the new window should trigger the wait. Yeah, but if it's in ui_test_utils.cc, it should behave generally, not just for our purposes. I'm not sure I see a pathway in which we could pass in a browser pointer to exclude that hasn't yet made it into the global browser list, but if we do (e.g. if we make the change that was being talked about on your other CL to not return browser pointers without windows from the browser list) this routine should be resilient against that. I.e. at least based on my current understanding of this routine and the browser list infrastructure, I think we should make sure the browser returned from WaitForNewBrowser() isn't in the set of excluded browsers. If you are pretty sure that should never happen, a DCHECK would be fine, but if you can see some pathway in the future that might cause it to happen, code it completely now and save someone some pain.
Added a DCHECK in WaitForBrowserNotInSet().
On 2011/01/07 16:42:57, ahendrickson wrote: > Added a DCHECK in WaitForBrowserNotInSet(). LGTM.
LGTM with 2 comments. http://codereview.chromium.org/5610006/diff/74001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/5610006/diff/74001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:375: if (!downloaded_file_deleted) nit: Just return download_file_deleted, sorry I missed it before. http://codereview.chromium.org/5610006/diff/74001/chrome/test/ui_test_utils.cc File chrome/test/ui_test_utils.cc (right): http://codereview.chromium.org/5610006/diff/74001/chrome/test/ui_test_utils.c... chrome/test/ui_test_utils.cc:55: void NavigateToURLWithDispositionBlockUntilNavigationsComplete( Instead of the forward-declaration, could you just put the function's body before any of places that call it?
http://codereview.chromium.org/5610006/diff/74001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/5610006/diff/74001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:375: if (!downloaded_file_deleted) On 2011/01/08 08:07:56, Paweł Hajdan Jr. wrote: > nit: Just return download_file_deleted, sorry I missed it before. Done. http://codereview.chromium.org/5610006/diff/74001/chrome/test/ui_test_utils.cc File chrome/test/ui_test_utils.cc (right): http://codereview.chromium.org/5610006/diff/74001/chrome/test/ui_test_utils.c... chrome/test/ui_test_utils.cc:55: void NavigateToURLWithDispositionBlockUntilNavigationsComplete( On 2011/01/08 08:07:56, Paweł Hajdan Jr. wrote: > Instead of the forward-declaration, could you just put the function's body > before any of places that call it? Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
