Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
21432002:
Rewrote few prefs related pyauto tests as browser_tests. (Closed)
|
Created:
7 years, 4 months ago by pshenoy Modified:
7 years, 4 months ago Reviewers:
Bernhard Bauer CC:
chromium-reviews, anantha Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionRewrote few prefs related pyauto tests as browser_tests.
BUG=None.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215335
Patch Set 1 #
Total comments: 22
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Messages
Total messages: 11 (0 generated)
Hi Bernhard, I have converted few prefs related pyauto tests (chrome/test/functional/prefs.py) to browser tests. Could you please take a look? -- Prashanth
https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... File chrome/browser/prefs/pref_functional_browsertest.cc (right): https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... chrome/browser/prefs/pref_functional_browsertest.cc:30: } Nit: Newline before public/protected/private sections. https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... chrome/browser/prefs/pref_functional_browsertest.cc:34: content::DownloadTestObserver* CreateWaiter( Who owns this object? The caller? If so, you can return a scoped_ptr. https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... chrome/browser/prefs/pref_functional_browsertest.cc:35: Browser* browser, int num_downloads) { Does this not fit on the previous line? https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... chrome/browser/prefs/pref_functional_browsertest.cc:37: BrowserContext::GetDownloadManager(browser->profile()); Indent two more spaces. https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... chrome/browser/prefs/pref_functional_browsertest.cc:42: base::FilePath test_data_dir_; Nit: A newline before this would be nice. Hm, is this even used? https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... chrome/browser/prefs/pref_functional_browsertest.cc:119: // Verify enabling disbling javascript perfs work. "Verify that enabling/disabling Javascript in prefs works." https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... chrome/browser/prefs/pref_functional_browsertest.cc:145: chrome::Exit(); Hm, I'm confused -- this is an in-process test, right? So what does chrome::Exit() do then? https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... chrome/browser/prefs/pref_functional_browsertest.cc:153: IN_PROC_BROWSER_TEST_F(PrefsFunctionalTest, TestToolbarButtonsPref) { This doesn't really test much else than the previous test.
https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... File chrome/browser/prefs/pref_functional_browsertest.cc (right): https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... chrome/browser/prefs/pref_functional_browsertest.cc:30: } On 2013/08/01 11:38:42, Bernhard Bauer wrote: > Nit: Newline before public/protected/private sections. Done. https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... chrome/browser/prefs/pref_functional_browsertest.cc:34: content::DownloadTestObserver* CreateWaiter( On 2013/08/01 11:38:42, Bernhard Bauer wrote: > Who owns this object? The caller? If so, you can return a scoped_ptr. I am using scoped_ptr in line# 69 where it is actually getting called. Please suggest if that's enough? https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... chrome/browser/prefs/pref_functional_browsertest.cc:35: Browser* browser, int num_downloads) { On 2013/08/01 11:38:42, Bernhard Bauer wrote: > Does this not fit on the previous line? Done. https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... chrome/browser/prefs/pref_functional_browsertest.cc:37: BrowserContext::GetDownloadManager(browser->profile()); On 2013/08/01 11:38:42, Bernhard Bauer wrote: > Indent two more spaces. Done. https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... chrome/browser/prefs/pref_functional_browsertest.cc:42: base::FilePath test_data_dir_; On 2013/08/01 11:38:42, Bernhard Bauer wrote: > Nit: A newline before this would be nice. > > Hm, is this even used? Done. https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... chrome/browser/prefs/pref_functional_browsertest.cc:119: // Verify enabling disbling javascript perfs work. On 2013/08/01 11:38:42, Bernhard Bauer wrote: > "Verify that enabling/disabling Javascript in prefs works." Done. https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... chrome/browser/prefs/pref_functional_browsertest.cc:145: chrome::Exit(); On 2013/08/01 11:38:42, Bernhard Bauer wrote: > Hm, I'm confused -- this is an in-process test, right? So what does > chrome::Exit() do then? I'm confused too :-) I wanted to close all browser windows and chrome::Exit() actually did it for me. I am not sure how do we simulate restarting the browser within browser_test. Note: I tried running the test without "chrome::Exit() and chrome::NewWindow()" and it works fine. Please suggest if there is a better way of handling this. https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... chrome/browser/prefs/pref_functional_browsertest.cc:153: IN_PROC_BROWSER_TEST_F(PrefsFunctionalTest, TestToolbarButtonsPref) { On 2013/08/01 11:38:42, Bernhard Bauer wrote: > This doesn't really test much else than the previous test. Done.
https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... File chrome/browser/prefs/pref_functional_browsertest.cc (right): https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... chrome/browser/prefs/pref_functional_browsertest.cc:34: content::DownloadTestObserver* CreateWaiter( On 2013/08/01 17:26:44, pshenoy wrote: > On 2013/08/01 11:38:42, Bernhard Bauer wrote: > > Who owns this object? The caller? If so, you can return a scoped_ptr. > > I am using scoped_ptr in line# 69 where it is actually getting called. Please > suggest if that's enough? You can also return a scoped_ptr here. The effect is the same, but it documents that the caller takes ownership, and the compiler will verify it too. https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... chrome/browser/prefs/pref_functional_browsertest.cc:145: chrome::Exit(); On 2013/08/01 17:26:44, pshenoy wrote: > On 2013/08/01 11:38:42, Bernhard Bauer wrote: > > Hm, I'm confused -- this is an in-process test, right? So what does > > chrome::Exit() do then? > > I'm confused too :-) I wanted to close all browser windows and chrome::Exit() > actually did it for me. I am not sure how do we simulate restarting the browser > within browser_test. > > Note: I tried running the test without "chrome::Exit() and chrome::NewWindow()" > and it works fine. Well, yes. The test sets a preference and then reads it again. Whether you close browser windows or not doesn't have any effect on that. > Please suggest if there is a better way of handling this. It depends on what you want to test. If you want to test that preferences are preserved across browser restarts, you will need to start the browser twice, which you can't do in a single test. One thing you could do would be to split up the test into two: one test that sets the preference, possibly commits the pending write, and then verifies that the Preferences file has been written out to disk, and one test that initially makes sure the Preferences file exists on disk, then starts the browser and verifies that the preference has been read. Another thing to test (which TestSessionRestoreShowBookmarkBar seems to do) is to test the *effect* of the preferences, by setting a preference and then verifying the resulting behavior. That depends on the preference of course.
https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... File chrome/browser/prefs/pref_functional_browsertest.cc (right): https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... chrome/browser/prefs/pref_functional_browsertest.cc:34: content::DownloadTestObserver* CreateWaiter( On 2013/08/01 22:20:07, Bernhard Bauer wrote: > On 2013/08/01 17:26:44, pshenoy wrote: > > On 2013/08/01 11:38:42, Bernhard Bauer wrote: > > > Who owns this object? The caller? If so, you can return a scoped_ptr. > > > > I am using scoped_ptr in line# 69 where it is actually getting called. Please > > suggest if that's enough? > > You can also return a scoped_ptr here. The effect is the same, but it documents > that the caller takes ownership, and the compiler will verify it too. Done. https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... chrome/browser/prefs/pref_functional_browsertest.cc:145: chrome::Exit(); On 2013/08/01 22:20:07, Bernhard Bauer wrote: > On 2013/08/01 17:26:44, pshenoy wrote: > > On 2013/08/01 11:38:42, Bernhard Bauer wrote: > > > Hm, I'm confused -- this is an in-process test, right? So what does > > > chrome::Exit() do then? > > > > I'm confused too :-) I wanted to close all browser windows and chrome::Exit() > > actually did it for me. I am not sure how do we simulate restarting the > browser > > within browser_test. > > > > Note: I tried running the test without "chrome::Exit() and > chrome::NewWindow()" > > and it works fine. > > Well, yes. The test sets a preference and then reads it again. Whether you close > browser windows or not doesn't have any effect on that. > > > Please suggest if there is a better way of handling this. > > It depends on what you want to test. If you want to test that preferences are > preserved across browser restarts, you will need to start the browser twice, > which you can't do in a single test. One thing you could do would be to split up > the test into two: one test that sets the preference, possibly commits the > pending write, and then verifies that the Preferences file has been written out > to disk, and one test that initially makes sure the Preferences file exists on > disk, then starts the browser and verifies that the preference has been read. > > Another thing to test (which TestSessionRestoreShowBookmarkBar seems to do) is > to test the *effect* of the preferences, by setting a preference and then > verifying the resulting behavior. That depends on the preference of course. I have removed 'chrome::Exit() and chrome::NewWindow()' for these tests as it has no effect. Just curious. How do we preserve preferences across tests in browser_tests? As I understand (I have very limited knowledge about browser_tests) each browser_test launches browser with a clean profile. So am not sure how we can make use of the preference set by one test in another.
LGTM https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... File chrome/browser/prefs/pref_functional_browsertest.cc (right): https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... chrome/browser/prefs/pref_functional_browsertest.cc:145: chrome::Exit(); On 2013/08/01 23:44:33, pshenoy wrote: > On 2013/08/01 22:20:07, Bernhard Bauer wrote: > > On 2013/08/01 17:26:44, pshenoy wrote: > > > On 2013/08/01 11:38:42, Bernhard Bauer wrote: > > > > Hm, I'm confused -- this is an in-process test, right? So what does > > > > chrome::Exit() do then? > > > > > > I'm confused too :-) I wanted to close all browser windows and > chrome::Exit() > > > actually did it for me. I am not sure how do we simulate restarting the > > browser > > > within browser_test. > > > > > > Note: I tried running the test without "chrome::Exit() and > > chrome::NewWindow()" > > > and it works fine. > > > > Well, yes. The test sets a preference and then reads it again. Whether you > close > > browser windows or not doesn't have any effect on that. > > > > > Please suggest if there is a better way of handling this. > > > > It depends on what you want to test. If you want to test that preferences are > > preserved across browser restarts, you will need to start the browser twice, > > which you can't do in a single test. One thing you could do would be to split > up > > the test into two: one test that sets the preference, possibly commits the > > pending write, and then verifies that the Preferences file has been written > out > > to disk, and one test that initially makes sure the Preferences file exists on > > disk, then starts the browser and verifies that the preference has been read. > > > > Another thing to test (which TestSessionRestoreShowBookmarkBar seems to do) is > > to test the *effect* of the preferences, by setting a preference and then > > verifying the resulting behavior. That depends on the preference of course. > > I have removed 'chrome::Exit() and chrome::NewWindow()' for these tests as it > has no effect. > > Just curious. How do we preserve preferences across tests in browser_tests? As I > understand (I have very limited knowledge about browser_tests) each browser_test > launches browser with a clean profile. So am not sure how we can make use of the > preference set by one test in another. Use a PRE_ prefix: http://www.chromium.org/developers/testing/browser-tests#TOC-Tests-Spanning-R... Alternatively, use two tests: One that verifies that the preference is set in the Preferences file on disk, and one that writes out that Preferences file before starting the browser. Those can then run independently.
https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... File chrome/browser/prefs/pref_functional_browsertest.cc (right): https://codereview.chromium.org/21432002/diff/1/chrome/browser/prefs/pref_fun... chrome/browser/prefs/pref_functional_browsertest.cc:145: chrome::Exit(); On 2013/08/02 08:46:12, Bernhard Bauer wrote: > On 2013/08/01 23:44:33, pshenoy wrote: > > On 2013/08/01 22:20:07, Bernhard Bauer wrote: > > > On 2013/08/01 17:26:44, pshenoy wrote: > > > > On 2013/08/01 11:38:42, Bernhard Bauer wrote: > > > > > Hm, I'm confused -- this is an in-process test, right? So what does > > > > > chrome::Exit() do then? > > > > > > > > I'm confused too :-) I wanted to close all browser windows and > > chrome::Exit() > > > > actually did it for me. I am not sure how do we simulate restarting the > > > browser > > > > within browser_test. > > > > > > > > Note: I tried running the test without "chrome::Exit() and > > > chrome::NewWindow()" > > > > and it works fine. > > > > > > Well, yes. The test sets a preference and then reads it again. Whether you > > close > > > browser windows or not doesn't have any effect on that. > > > > > > > Please suggest if there is a better way of handling this. > > > > > > It depends on what you want to test. If you want to test that preferences > are > > > preserved across browser restarts, you will need to start the browser twice, > > > which you can't do in a single test. One thing you could do would be to > split > > up > > > the test into two: one test that sets the preference, possibly commits the > > > pending write, and then verifies that the Preferences file has been written > > out > > > to disk, and one test that initially makes sure the Preferences file exists > on > > > disk, then starts the browser and verifies that the preference has been > read. > > > > > > Another thing to test (which TestSessionRestoreShowBookmarkBar seems to do) > is > > > to test the *effect* of the preferences, by setting a preference and then > > > verifying the resulting behavior. That depends on the preference of course. > > > > I have removed 'chrome::Exit() and chrome::NewWindow()' for these tests as it > > has no effect. > > > > Just curious. How do we preserve preferences across tests in browser_tests? As > I > > understand (I have very limited knowledge about browser_tests) each > browser_test > > launches browser with a clean profile. So am not sure how we can make use of > the > > preference set by one test in another. > > Use a PRE_ prefix: > http://www.chromium.org/developers/testing/browser-tests#TOC-Tests-Spanning-R... > > Alternatively, use two tests: One that verifies that the preference is set in > the Preferences file on disk, and one that writes out that Preferences file > before starting the browser. Those can then run independently. Thank you so much for the information.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pshenoy@chromium.org/21432002/17001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pshenoy@chromium.org/21432002/17001
Message was sent while issue was closed.
Change committed as 215335 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
