Allow unit tests being run in parallel again #870
Conversation
Signed-off-by: Julian Fleischer <[email protected]>
Signed-off-by: Julian Fleischer <[email protected]>
|
Concept ACK |
Do you suggest two-step initialization? I'd love to hear your points why components shouldn't fully initialize themselves during initialization. Elaborating on this topic will help us follow best practices during further component development. |
I have to admit I am a bit unsure about it myself in this particular case, which is also why I did not do any further changes. But to elaborate on this a bit / brainstorm: In case of the proposer for instance, which is a kind of component which I would call an "active" component (it does thing's on its own, namely proposing), I like to start it explicitly after the whole program is initialized. The reason being: There is, let's say, an RPC component which depends on the proposer but not the other way around. The proposer is initialized. Only then the RPC component is initialized. So it's conceivable that the proposer starts while it can not be controlled though RPC yet – because the system it not completely up yet. I would like the whole system to be in a runnable state but not running - which is how I have seen and written a lot of software successfully. Like (1) Step one initialize (configuration and everything) (2) Step two: Actually start the thing. In code something like: In case of the database I am unsure because checking that we can write to a database and everything is part of bringing the system into a runnable state, or put it another way: So while my initial thought was "it should not do this" I think I have come to terms with it and think that it's right the way it does it. Just if if was doing something actively, start outbound connections, start proposing, these kinds of things, I'd want it to be behind a Start() method. The injector already has this capability of stopping components which do have a Stop() method and same logic could be applied for Start(), but so far we do not have a terrible lot of use cases for this. |
cmihai
left a comment
cmihai
left a comment
There was a problem hiding this comment.
utACK 55f90af.
Just throwing this out there: maybe, instead of adding a factory function for each component, even though we only need it for InjectorConfiguration, we could overload Unmanaged::Init so that is accepts both a pointer and a factory function?
That would be very nice I guess. Although I am personally not too big a fan of overloading functions I think this would be a sweet addition. I would nevertheless follow up on this in a separate pull request. |
Fixes warning introduced by #870. ``` In file included from ./injector.h:17: ./injector_config.h:8:1: warning: 'UnitEInjectorConfiguration' defined as a struct here but previously declared as a class [-Wmismatched-tags] struct UnitEInjectorConfiguration { ^ ./finalization/state_db.h:24:1: note: did you mean struct here? class UnitEInjectorConfiguration; ^~~~~ struct ``` Signed-off-by: Stanislav Frolov <[email protected]>
Follow-up to #870 (review) – as suggested by @cmihai Signed-off-by: Julian Fleischer <[email protected]>
5 participants
Currently unit tests can not be run in parallel. This prevents us from speeding up ci builds (see bitcoin/bitcoin#12926, #865) and won't work with the updated build definitions from bitcoin 0.17 (#860). This is a regression which was introduced in #793. This pull request fixes #861.
The problem is that the
StateDBcomponent does something actively when being initialized (not something a component should do), which is to access disk and so on, which can fail. Since there is aUnitEInjectorfor everyBasicTestingSetuprunning the unit tests in parallel creates a database in the same directory per unit tests suite which clashes and breaks and throws and aborts and dies. This can be prevented by using an in-memory database, but the problem shifts: How to get that configuration in?I did not want to expose this as a user-definable setting and also not as a chain parameter, so these two configuration options are not available. Which is why I created
UnitEInjectorConfiguration. This is something which should not be necessary as usually you would not expose the same module as in production in unit tests but use a subset or just the mocked version, but since the rest of bitcoin is not in well-defined components and accesses them usingGetComponenta global injector has to be available in some tests.The change ultimately does not affect the prod version of things, but the tests, which inject a different
UnitEInjectorConfiguration. I made this astructrather then just a flagunit_testssuch that it is extensible in case we have other cases like this. In order to make the injector be able to access fields from it's own instance in the definition of a component I altered the definition ofUNMANAGED_COMPONENTa bit.I would like to point out that these initialization issues would also have happened with an Application class (#723), as that is exactly what
UnitEInjectoris, just you would inject things manually.