Dependency Injection framework for Chromium code

[Sergiy Byelozyorov]

Hi,

tl;dr injecting dependencies from an integration test is hard, especially when they are used in constructors/static members; let's discuss whether we want to add a dependency injection framework to Chromium code to make this easier and what criteria should such framework satisfy

Today in Chromium code there are mainly two mechanisms used for dependency injection in integration tests:

• pass dependencies via the constructor, and
• use helper methods (typically ending with ForTesting or _for_testing) to replace default dependencies with mocks.

Both approaches are not ideal: passing dependencies via constructor requires objects to be constructed in tests, which works well for unit tests, but not so well for integration tests, where objects are typically constructed by code under test. Using helper methods does not work when dependencies are already used in the constructor, hence one needs to create factories.

Situation becomes even more complicated when such dependencies are used in the static methods. For example, let's say we want to mock values returned by base::Time::Now() and want to replace this call with a mockable base::Clock*. Since it is called from a static method, base::Clock* has to be static as well, yet we are not allowed to add static members as static initialization is prohibited (for good reasons). The alternative is to use a method that stores a mockable object as a local static pointer, which is initialized on the first call and never destructed. However, this also requires an additional indirection to allow mocking it (full code):

1. class Example {
2.  public:
3.   static void PrintNow() {
4.     std::cout << GetClock().Now() << std::endl;
5.   }
6.    
7.   static void SetClockForTesting(base::Clock* clock) {
8.     *GetClockPtr() = clock;
9.   }
10.  
11.  private:
12.   static base::Clock** GetClockPtr() {
13.     static base::Clock* clock = new base::DefaultClock();
14.     return &clock;
15.   }
16.  
17.   static base::Clock& GetClock() {
18.       return **GetClockPtr();
19.   }
20. };

Furthermore, to avoid leaking memory on each call to SetClockForTesting and to reduce use-after-free risk, we also need to also use smart pointers, but ensure that they are not released after the program end (full code):

1. class Example {
2.  public:
3.    static void PrintNow() {
4.      std::cout << GetClock().Now() << std::endl;
5.    }
6.    
7.    static void SetClockForTesting(std::unique_ptr<base::Clock>& clock) {
8.      GetClockPtr() = std::move(clock);
9.    }
10.  
11.  private:
12.   static std::unique_ptr<base::Clock>& GetClockPtr() {
13.     static base::NoDestructor<std::unique_ptr<base::Clock>> clock([] {
14.       return std::make_unique<base::DefaultClock>();
15.     }());
16.     return *clock;
17.   }
18.  
19.   static base::Clock& GetClock() {
20.     return *GetClockPtr().get();
21.   }
22. };

As you can see, this becomes very complex very quickly. Programs in other languages, such as Java, often use dependency injection frameworks, e.g. Guice, which make it significantly easier to achieve the above on top of adding various advanced features such as conditional mocks. There are even C++ frameworks inspired by Guice, e.g. Fruit.

Hence, I wonder whether we should add such a framework to Chromium? Before selecting/creating a framework, we also need to agree on criteria that it must satisfy. IMHO such a framework must be:

• able to inject both instance and static dependencies,
• require little boilerplate for defining a mockable dep / override it in test, and
• be able to add mockable dependencies to existing classes incrementally (without needing global configs).

WDYT?

--

Sergiy Belozorov |

Software Engineer |

ser...@chromium.org

Google Germany GmbH

Erika-Mann-Straße 33

80636 München

AG Hamburg, HRB 86891 | Sitz der Gesellschaft: Hamburg | Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

[K. Moon]

I've mostly used DI in Java using Guice. What would an example of this look like in C++?

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAADpsr7L2vUCJkJriekVSw1xmHHiE%2BMorG7A_u_eGzM4S7fbRA%40mail.gmail.com.

[K. Moon]

I'd like to see the example you gave written for a framework like Fruit. Given the size of the audience, I think that would make for an easier evaluation than asking everyone to learn how to use Fruit and do the comparison themselves.

On Sat, Sep 25, 2021, 7:30 AM Sergiy Byelozyorov <ser...@chromium.org> wrote:

There is Fruit as linked in my original message. But there are probably others. We could also create one specifically tailored to Chromium needs, but we'd need to define requirements first.

[K. Moon]

To clarify: I don't think this is going to get any traction with the wider community unless there's a demonstrated improvement to Chromium code. You'd need to take some actual Chromium code, update it, argue that it's actually better, and then argue that it's worth the cost of switching. (Eventually, this would have to go through some sort of high-level eng review.) It's not reasonable to expect everyone else who works on Chromium to spend any of their time making that evaluation themselves: If you're making a proposal, that burden is yours.

Incidentally, my personal default position (which doesn't count for much, so don't feel discouraged by it :-) ) is that the status quo is acceptable, if not ideal, and probably not worth the engineering (and possibly performance) cost of switching, but I'd be open to convincing case studies showing otherwise.

On Mon, Sep 27, 2021 at 4:19 AM Sergiy Byelozyorov <ser...@chromium.org> wrote:

I think this Getting Started page is a pretty good start: https://github.com/google/fruit/wiki/tutorial:-getting-started.

[Roland Bock]

Not sure if this is feasible here, but one way I solved this kind of challenge in another project (closed source) was to implement a special test version of the static function we wanted to change. It is then a question of linking the production version to production code and the test version to the tests that need it.

In some cases (when the production version is in a library), it was sufficient to simply provide the test-version in the test's compilation unit. But that might be linker specific.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CACwGi-7ByAECQ3ME47SqGFcQeUUS%2BaNbxvxyGQLN6b%2BU8nTSfg%40mail.gmail.com.

[Nico Weber]

I find the argument that Guice makes anything significantly easier a bit suspect. In many codebases that use Guice, I find it very difficult to reason about what object gets injected where.

As a meta point, writing tests has worked alright with the current approaches for a long time. Adding a fairly different way of writing tests would mean that we'd have another style of tests in perpetuity, and everyone would have to learn both styles. So any proposed new way would have to have some significant advantage to make up for this cost.

So I'd try to solve your problem in the existing framework somehow. Maybe you can make your example function non-static?

--

[dan...@chromium.org]

On Fri, Sep 24, 2021 at 5:29 PM Sergiy Byelozyorov <ser...@chromium.org> wrote:

Hi,

tl;dr injecting dependencies from an integration test is hard, especially when they are used in constructors/static members; let's discuss whether we want to add a dependency injection framework to Chromium code to make this easier and what criteria should such framework satisfy

Today in Chromium code there are mainly two mechanisms used for dependency injection in integration tests:

• pass dependencies via the constructor, and
• use helper methods (typically ending with ForTesting or _for_testing) to replace default dependencies with mocks.

I didn't see it mentioned, but we also use factories in tests, where you can replace a Production type with a testing subclass, by overriding a factory. That type could then be passed to the constructor as above, and you no longer have it being created by the code under test alone.

 

--

[Sergiy Byelozyorov]

There is Fruit as linked in my original message. But there are probably others. We could also create one specifically tailored to Chromium needs, but we'd need to define requirements first.

On Fri, Sep 24, 2021, 23:32 K. Moon <km...@chromium.org> wrote:

[Sergiy Byelozyorov]

I think this Getting Started page is a pretty good start: https://github.com/google/fruit/wiki/tutorial:-getting-started.

[Joshua Pawlicki]

I'm probably not qualified to comment on this, but working with Guice in Java mostly taught me that dependency injection in object-oriented-programming is usually best done through pure interfaces, ctors, and factories, without any special framework or metaprogramming support.

IMHO, ideally, static functions that need injectable dependencies could take them as arguments, and we could use partial application to cut down on the toil involved on plumbing arguments all the way through long stacks. (I acknowledge this does mean keeping some base::Callbacks around; but I prefer that to SetXForTesting.)

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAADpsr7L2vUCJkJriekVSw1xmHHiE%2BMorG7A_u_eGzM4S7fbRA%40mail.gmail.com.

[Sergiy Byelozyorov]

Sorry for the delay in response (I was on vacation) and for misunderstanding what K. Moon were asking for. I am actually no expert in Fruit (found it myself after a bit of Googling) and so far I couldn't find a way to mock static members with it. So maybe it's not a good solution after all. But it is possible to write a new framework that does support that. Main idea would be to avoid the need to manually write code for managing memory with static function-scoped pointers-to-unique-ptr. This is rather complicated and doing it right is hard. Here is my attempt to write a simple macro-based solution simplifying this process (full code):

1. #define STATIC_MOCKABLE(type, name, default)                                \
2.   public:                                                                   \
3.     template <class T, class... Args>                                       \
4.     static T* Create##name##ForTesting(Args&&... args) {                    \
5.       Get##name##Ptr() = std::make_unique<T>(std::forward<Args>(args)...);  \
6.       return static_cast<T*>(Get##name##Ptr().get());                       \
7.     }                                                                       \
8.   private:                                                                  \
9.    static std::unique_ptr<type>& Get##name##Ptr() {                         \
10.      static base::NoDestructor<std::unique_ptr<type>> instance([] {         \
11.        return default;                                                      \
12.      }());                                                                  \
13.      return *instance;                                                      \
14.    }                                                                        \
15.    static type& Get##name() {                                               \
16.      return *Get##name##Ptr().get();                                        \
17.    }
18.  
19. // Example class allowing to inject custom Clock.

1. class Example {
2.  public:
3.   static void PrintNow() {
4.     std::cout << GetClock().Now() << std::endl;
5.   }
6.    

1.   STATIC_MOCKABLE(base::Clock, Clock, std::make_unique<base::DefaultClock>());
2. };

Note that usage is also simpler now:

1. auto* clock_mock = Example::CreateClockForTesting<base::MockClock>(11);
2. Example::PrintNow();
3. clock_mock->SetNow(22);
4. Example::PrintNow();

Previously we had to do the following:

1. std::unique_ptr<base::Clock> clock_mock = std::make_unique<base::MockClock>(11);
2. auto* raw_clock_mock = static_cast<base::MockClock*>(clock_mock.get());
3. Example::SetClockForTesting(clock_mock);
4. Example::PrintNow();
5. raw_clock_mock->SetNow(22);
6. Example::PrintNow();
   

Roland, thanks for sharing the idea with linking. It is a very interesting idea, but I wonder how that'd work in practice. In particular, I recall you've mentioned (outside of this thread) that some linkers allow multiple definitions of the same symbol and prefer the one that is "closest". From my experience most linkers just complain when there are multiple definitions. Is this some special feature that needs to be enabled via a flag? We'd probably need to make sure that it works on all platforms that we target.

Nico, I agree that injection frameworks make it harder to reason where an object gets injected from. There are other solutions to this problem though, e.g. Code Search layers showing injection locations based on analyzing actual binary/test runs.

Danakj, do you mean something like this? Or just a plain factory method that is called from the static method?

Joshua, can you please write an example using partial application and base::Callback? I am not very familiar with that.

[Joshua Pawlicki]

> Joshua, can you please write an example using partial application and base::Callback? I am not very familiar with that.

Transforming the above example:

class Example {
 public:
  static void PrintNow(base::Clock* clock) {
    std::cout << clock->Now() << std::endl;
  }
};

{ // In some test
  base::SimpleTestClock clock;
  base::RepeatingClosure example = base::BindRepeating(&Example::PrintNow, &clock);

  example.Run();
  clock->SetNow(22);
  example.Run();

}

In practical use a refcounted type or a more sophisticated Example might be used: the `&clock` is dangerous if `example` outlives `clock`.

One point about this code is that a concurrent call to Example::PrintNow isn't going to pick up the test clock, because each caller will be providing their own *clock. In other words, this test can no longer affect the behavior of subsequent / concurrent tests.

It's also not possible for a caller to forget to provide a clock (because the program won't compile). This can be tedious but good code structure can overcome that.

To put my own spin on Nico's earlier point, it is difficult to reason about programs that have functions with "SetForTesting" helpers or functions that depend on mutating globals. I agree that some DI frameworks can make it easier to write such code, but personally I don't find that a compelling reason to adopt or create them.

You received this message because you are subscribed to a topic in the Google Groups "Chromium-dev" group.
To unsubscribe from this topic, visit https://groups.google.com/a/chromium.org/d/topic/chromium-dev/4NPKEyyNmtU/unsubscribe.
To unsubscribe from this group and all its topics, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAADpsr45fTBVE6rhOsFPJuEvjFys6TH%3D3GVCPo8UdxG5QEWxqw%40mail.gmail.com.