Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
225693005:
Try to fix ProfileBrowserTest. (Closed)
|
Created:
6 years, 8 months ago by Marc Treib Modified:
6 years, 8 months ago CC:
chromium-reviews, rginda+watch_chromium.org, yoshiki+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionTry to fix ProfileBrowserTest.
BUG=165760, 140882, 141141, 141517, 142787
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264493
Patch Set 1 : Test fixes - don't seem to be flaky anymore! #Patch Set 2 : Disable tests again. #
Total comments: 3
Patch Set 3 : Cleanup: Common helper function to spin threads. #Messages
Total messages: 11 (0 generated)
With some extra manual cleanup, the tests don't seem to be flaky anymore, as far as I can tell (at the very least, they're now a lot less flaky on Linux). I'd like to tentatively re-enable them in another CL, so that can be reverted separately if necessary.
Why are you putting all the creation steps in its own scope? Do you know why that's necessary? I see you disabled in a second patch set. Does this not actually fix stuff?
https://codereview.chromium.org/225693005/diff/20001/chrome/browser/profiles/... File chrome/browser/profiles/profile_browsertest.cc (right): https://codereview.chromium.org/225693005/diff/20001/chrome/browser/profiles/... chrome/browser/profiles/profile_browsertest.cc:87: content::RunAllPendingInMessageLoop(content::BrowserThread::FILE); Can you use a ThreadBundle and eliminate all this special casing?
On 2014/04/09 20:05:01, rpetterson wrote: > Why are you putting all the creation steps in its own scope? Do you know why > that's necessary? The extra scope isn't really necessary, it's just to delete the Profile before spinning the threads. I find this somewhat nicer than explicitly reset()ing the pointer (but I can do that as well if you prefer it). > I see you disabled in a second patch set. Does this not actually fix stuff? It does, as far as I can tell on my Linux box. However, it has happened before that someone partially fixed the tests, and when some of them turned out to be still flaky, the whole thing was reverted. So I'd like to do the actual enabling in another CL.
https://codereview.chromium.org/225693005/diff/20001/chrome/browser/profiles/... File chrome/browser/profiles/profile_browsertest.cc (right): https://codereview.chromium.org/225693005/diff/20001/chrome/browser/profiles/... chrome/browser/profiles/profile_browsertest.cc:87: content::RunAllPendingInMessageLoop(content::BrowserThread::FILE); On 2014/04/09 20:05:11, rpetterson wrote: > Can you use a ThreadBundle and eliminate all this special casing? You mean a TestBrowserThreadBundle? As I understand it, that's meant only for unit tests, not browser tests? Here, all the threads already exist. However, I could replicate the full shutdown behavior from TestBrowserThreadBundle.
lgtm Thanks for cleaning these tests up! https://codereview.chromium.org/225693005/diff/20001/chrome/browser/profiles/... File chrome/browser/profiles/profile_browsertest.cc (right): https://codereview.chromium.org/225693005/diff/20001/chrome/browser/profiles/... chrome/browser/profiles/profile_browsertest.cc:87: content::RunAllPendingInMessageLoop(content::BrowserThread::FILE); On 2014/04/10 09:25:09, treib wrote: > On 2014/04/09 20:05:11, rpetterson wrote: > > Can you use a ThreadBundle and eliminate all this special casing? > > You mean a TestBrowserThreadBundle? As I understand it, that's meant only for > unit tests, not browser tests? Here, all the threads already exist. > However, I could replicate the full shutdown behavior from > TestBrowserThreadBundle. Ah, I didn't realize that that was just for browser tests. No worries. Although you could group them into a helper function to cut down on repeated code. Same with the profile creation. But I'm fine doing that in a cleanup CL.
On 2014/04/16 17:50:44, rpetterson wrote: > lgtm > > Thanks for cleaning these tests up! > > https://codereview.chromium.org/225693005/diff/20001/chrome/browser/profiles/... > File chrome/browser/profiles/profile_browsertest.cc (right): > > https://codereview.chromium.org/225693005/diff/20001/chrome/browser/profiles/... > chrome/browser/profiles/profile_browsertest.cc:87: > content::RunAllPendingInMessageLoop(content::BrowserThread::FILE); > On 2014/04/10 09:25:09, treib wrote: > > On 2014/04/09 20:05:11, rpetterson wrote: > > > Can you use a ThreadBundle and eliminate all this special casing? > > > > You mean a TestBrowserThreadBundle? As I understand it, that's meant only for > > unit tests, not browser tests? Here, all the threads already exist. > > However, I could replicate the full shutdown behavior from > > TestBrowserThreadBundle. > > Ah, I didn't realize that that was just for browser tests. No worries. Although > you could group them into a helper function to cut down on repeated code. Same > with the profile creation. But I'm fine doing that in a cleanup CL. Might as well just do it now :) I added a helper function to remove the duplicated "spin threads" code. I'll submit a followup CL to re-enable the tests once this one has landed.
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/treib@chromium.org/225693005/40001
Message was sent while issue was closed.
Change committed as 264493
Message was sent while issue was closed.
I believe this CL is causing a number of LSan failures. ProfileBrowserTest.ExitType: http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te... ProfileBrowserTest.ProfileDeletedBeforeReadmeCreated: http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te... |
