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