Hi Jay, thanks for volunteering. What's your timeframe for looking at this?
-Ben On Mon, Apr 6, 2009 at 10:02 AM, Jay Campan <[email protected]> wrote: > I'll take a look at it since I am rewriting the SSL UI tests to be in-proc. > > Jay > > On Mon, Apr 6, 2009 at 9:58 AM, Darin Fisher <[email protected]> wrote: >> Anyone want to try to make this happen? :-) >> >> -Darin >> >> On Mon, Apr 6, 2009 at 5:47 AM, Marc-Antoine Ruel <[email protected]> >> wrote: >>> >>> I think this is a great idea and definitely implementable. Filed: >>> http://code.google.com/p/chromium/issues/detail?id=9764 >>> >>> >>> On Mon, Apr 6, 2009 at 1:20 AM, Darin Fisher <[email protected]> wrote: >>> > I think a better design for the in-browser unit tests would be to >>> > redesign >>> > them to work more like a normal browser. Instead of trying to restart >>> > BrowserMain, we should go all the way back to ChromeMain and unload >>> > chrome.dll. To make that work, perhaps we could bake the unit tests and >>> > the >>> > Chrome code into a ChromeTest.dll. Then we could load that before each >>> > test, and invoke its ChromeMain with a special command line flag that >>> > would >>> > magically invoke the corresponding test. With some cleverness we could >>> > probably make this work. Then advantage is that we would have a clean >>> > Chrome for each test without having to teach Chrome how to do something >>> > (namely, shutting down and restarting without unloading) it never needs >>> > to >>> > know how to do otherwise. It seems way to fragile (read: recipe for >>> > flakiness) to try to patch all of the odd statics. >>> > -Darin >>> > >>> > >>> > >>> > On Sat, Apr 4, 2009 at 1:00 PM, Mike Belshe <[email protected]> wrote: >>> >> >>> >> Actually, I think its the opposite; the one which bit Ben this week was >>> >> not a leakysingleton. Actually, the leakysingleton would have worked >>> >> fine >>> >> in this case.. He was forcing AtExit to be called, and then statics >>> >> which >>> >> held LazyInitPtrs were effectively uninitialized (no bug here, this is >>> >> a >>> >> really weird thing to do). Ironically, the LeakySingletons wouldn't >>> >> have an >>> >> issue in Ben's case, because they didn't get cleaned up.... >>> >> If we want to support Ben's case, I think we need to have better >>> >> encapsulation of all globals. I suspect the only way to get there is >>> >> to >>> >> eliminate most all statics. I think Dean made a lot of progress here, >>> >> but >>> >> we still have a lot more statics. Until they are gone, there will >>> >> always be >>> >> more to mop up. >>> >> Incidentally, when looking at Apple's webkit build, I noticed that >>> >> their >>> >> compiles will fail if new statics are introduced. I assume this is for >>> >> performance reasons, but maybe it's because webkit needs to be embedded >>> >> in >>> >> other systems and statics make that difficult. Anyway, we could add >>> >> something to our build as well which also checked to see if statics >>> >> were >>> >> created. >>> >> Mike >>> >> >>> >> On Sat, Apr 4, 2009 at 9:56 AM, Ben Goodger (Google) <[email protected]> >>> >> wrote: >>> >>> >>> >>> Thanks for the background, M-A and Dean. >>> >>> >>> >>> I started looking into this because I wrote an in-process browser test >>> >>> for a bug fix I wrote last week. I ended up finding one conflicting >>> >>> ad-hoc singleton and fixed the issue (see my changelist to extension >>> >>> error reporter) + one issue w/LazyInstanceHelper (see separate >>> >>> changelist). But you're right, there's likely more. >>> >>> >>> >>> Part of the challenge here is that the unit tests are run in an order >>> >>> that's unknown to me, and seems to be non-deterministic based on >>> >>> recency of modification and/or clobber build, but perhaps I'm >>> >>> overlooking something obvious? It definitely possible to encounter >>> >>> issues depending on what tests run before and after other tests in the >>> >>> same process. So it seems likely that seemingly unrelated tests could >>> >>> end up broken on the builders when people ran them locally and they >>> >>> passed. This would cause people to think the unrelated test bustage >>> >>> was "flakiness". >>> >>> >>> >>> Increased coverage will likely help here, along with requiring use of >>> >>> classes like Singleton/avoiding statics in code reviews. Starting this >>> >>> coming week, I am going to be a lot more aggressive about asking for >>> >>> in-process browser tests for at least the front end code reviews that >>> >>> I do. >>> >>> >>> >>> -Ben >>> >>> >>> >>> On Sat, Apr 4, 2009 at 9:48 AM, Marc-Antoine Ruel <[email protected]> >>> >>> wrote: >>> >>> > I recall when Singleton and AtExitManager were created, there was >>> >>> > discussion about controlled shutdown. I think this use case clearly >>> >>> > show why LeakySingletonTrait shouldn't be used whenever possible and >>> >>> > controled shutdown has important use-cases. If it is a multithreaded >>> >>> > issue, the culprit threads should be join-ed. >>> >>> > >>> >>> > Now, w.r.t. to what Ben said, I know adhoc singletons definitely >>> >>> > exists in a few place in webkit and in chromium. Finding them will >>> >>> > be >>> >>> > a lot of fun but would be a smart thing to do. (And by the way I >>> >>> > need >>> >>> > to thank Dean for having fixed a few of those in the past) >>> >>> > >>> >>> > Ben do you have a specific plan to fix this issue? >>> >>> > >>> >>> > M-A >>> >>> > >>> >>> > On Friday, April 3, 2009, Ben Goodger (Google) <[email protected]> >>> >>> > wrote: >>> >>> >> >>> >>> >> The new in process browser test framework creates a headless >>> >>> >> browser-like environment where rich cross-sectional tests can be >>> >>> >> run >>> >>> >> as unit tests rather than as ui tests. UI tests are a PITA and so >>> >>> >> this >>> >>> >> is very enticing. >>> >>> >> >>> >>> >> One issue we've had with so far is that many objects implement >>> >>> >> singletons in an ad-hoc way, sometimes caching pointers to objects >>> >>> >> that later get destroyed. Since a browser startup sequence is >>> >>> >> simulated (by a call to BrowserMain) for each run of an in-process >>> >>> >> browser test, when you run multiple in-process browser tests in the >>> >>> >> same unit_tests.exe run you will eventually crash somewhere. >>> >>> >> >>> >>> >> One solution is to have all singleton objects derive from >>> >>> >> base::Singleton, and use the base::AtExitManager to shut them down >>> >>> >> cleanly at the end of each test run. The only question is - how >>> >>> >> many >>> >>> >> ad-hoc singletons are there? >>> >>> >> >>> >>> >> Does this conversion seem reasonable? What challenges do people >>> >>> >> foresee? >>> >>> >> >>> >>> >> On a related note, with the commencement of the fixit week next >>> >>> >> week >>> >>> >> we're going to get a lot more strict about requiring tests for >>> >>> >> functionality. Barring the above issue, the in-process browser >>> >>> >> testing >>> >>> >> framework makes it relatively simple to write tests for complex >>> >>> >> functionality. I think we should require tests be written using >>> >>> >> this >>> >>> >> tool if applicable and have the tests be disabled if they fail when >>> >>> >> run in combination with others, so they don't turn the buildbot >>> >>> >> red, >>> >>> >> with the idea of fixing the framework at the same time and then >>> >>> >> turning them all on. The tests will pass when run in isolation. >>> >>> >> >>> >>> >> -Ben >>> >>> >> >>> >>> >> >> >>> >>> >> >>> >>> > >>> >>> >>> >>> >>> >> >>> >> >>> >> >> >>> > >>> > >> >> >> >> >> > --~--~---------~--~----~------------~-------~--~----~ Chromium Developers mailing list: [email protected] View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~----------~----~----~----~------~----~------~--~---
