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 -~----------~----~----~----~------~----~------~--~---
