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