+derat

Why do you think we need to change the current behavior?
It might be unintuitive, but it's documented in the header and it kind of makes sense (in other words, all tests which use customized fakes need to initialize DBusThreadManager after all).

I'm OK with either way, but it seems the current implementation is intentionally doing that.
https://codereview.chromium.org/515573003/diff/20001/extensions/shell/browser/shell_audio_controller_chromeos_unittest.cc#newcode35

The GetSetterForTesting stuff already completely confuses me every time I try to use it within tests, so I'll defer to others with stronger opinions here.

It's confusing because you have to remember to call Shutdown(), even though you never called a function starting with Initialize().

If you want to keep the current behavior it might help to rename that function to InitializeWithSetterForTest() or just InitializeForTest() and return the existing Setter.

I will certainly admit that the name GetSetterForTesting is not intuitive. It should really be named something like "InitializeForTestAndGetSetter." On the other hand, it is essentially just doing lazy initialization, which is not an uncommon pattern.

It was written for convenenience and we use it a lot. I would be fine with breaking out an explicit InitializeForTesting method, which we can call explicitly where it would improve readability, but I would prefer to still have GetSetterForTesting call that rather than add it to all 90 or so current uses.

FYI the reason I asked Toni to file this bug is that the Shutdown() was missing in 5 different places: https://chromium-review.googlesource.com/c/chromium/src/+/912431

This isn't a one-off problem.

Possibly-silly question: Why does DBusThreadManagerSetter exist? It seems like an unnecessary extra layer.

Right now, the general pattern appears to be that tests call chromeos::DBusThreadManager::GetSetterForTesting()->SetFooClient(std::move(whatever)). GetSetterForTesting creates a DBusThreadManager with use_real_clients=false if we don't already have one and returns a new DBusThreadManagerSetter.

DBusThreadManagerSetter has 22 SetFooClient methods, all of which look similar to this (although some use clients_browser_ instead):

void DBusThreadManagerSetter::SetBiodClient(
    std::unique_ptr<BiodClient> client) {
  DBusThreadManager::Get()->clients_common_->biod_client_ = std::move(client);
}

Could we instead move these methods into DBusThreadManager as SetFooClientForTest and get rid of DBusThreadManagerSetter entirely? We'd also need to make tests explicitly call a new DBusThreadManager::InitializeForTest method that does the setup that GetSetterForTesting does currently, but that seems like a good idea anyway.

DBusThreadManagerSetter appears to have the "benefit" of allowing tests to reuse a DBusThreadManager that was initialized by an earlier test. I'm not sure whether that's a good idea or not, but it seems like we could preserve the behavior in a new InitializeForTest method if we wanted to.

re #6: Ah. I agree that the lack of "Initialize" in GetSetterForTesting
makes the need to call Shutdown less obvious. That bit of info makes this
issue make more sense to me.

This seems like a good use case for a scoped helper that would handle
shutdown?

All of this code has evolved, so if somebody actively working on this code
wants to re-re-re-factor it I am happy to review any such changes :)

Scoped helper sounds good for both tests and the production code.

Moving setters to DBusThreadManager to get rid of DBusThreadManagerSetter sounds good too.