test: Add a mechanism for creating singletons in tests#5086
test: Add a mechanism for creating singletons in tests#5086htuch merged 13 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
… the test-runner that there are none remaining. This also involved renaming a helper class that tracks the entire system to 'Globals', which is where the new method was exposed. Signed-off-by: Joshua Marantz <jmarantz@google.com>
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
Looks good, this seems a useful tool to have around for tests.
test/test_common/global.cc
Outdated
| ASSERT(singleton->ref_count_ == 0); | ||
| singleton->ptr_ = make_object(); | ||
| } | ||
| ++singleton->ref_count_; |
There was a problem hiding this comment.
How come you're not using a shared_ptr/weak_ptr pattern here but doing it by hand?
There was a problem hiding this comment.
I thought about this for a bit; couldn't figure out how to make the pieces fit given the heterogenous map and casting.
There was a problem hiding this comment.
What about std::shared_ptr<Singleton> (and corresponding weak_ptr), with a void * still inside the object to provide the type cast abilities?
There was a problem hiding this comment.
I thought of that, but thought it annoying that I'd have to hold a functor for calling the class-specific delete in the singleton.
Then I thought I'd give it a shot and it wasn't too bad to just do that, so switching to weak_ptr.
This simplifies reasoning about the system, and potential for contention is not important in tests. It does, however, make it harder to use thread-annotation due to aliasing. Signed-off-by: Joshua Marantz <jmarantz@google.com>
…ngs in globals.cc/h Signed-off-by: Joshua Marantz <jmarantz@google.com>
test/test_common/global.cc
Outdated
| ASSERT(singleton->ref_count_ == 0); | ||
| singleton->ptr_ = make_object(); | ||
| } | ||
| ++singleton->ref_count_; |
There was a problem hiding this comment.
What about std::shared_ptr<Singleton> (and corresponding weak_ptr), with a void * still inside the object to provide the type cast abilities?
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
LGTM, do you have any thoughts on the one comment I left?
test/test_common/global.h
Outdated
| * start. | ||
| */ | ||
| class Globals { | ||
| using MakeObjectFn = std::function<void*()>; |
There was a problem hiding this comment.
Optionally might be able to base this on https://github.com/envoyproxy/envoy/blob/master/source/common/common/c_smart_ptr.h#L9.
There was a problem hiding this comment.
I had another idea; I just added a templatized derived class which handles deletion in a type-safe way (though we do still need to use static_cast to access the pointer and ref from Singleton).
ptal.
…ing a functor. Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
For the record, I am against singletons in general, and believe it's more maintainable to pass context objects through the call/instantiation stack, as is done in Envoy production code. However the test system employs a deep system of mocks that are constructed without context. As a result, any new context that may be needed in the future (e.g. SymbolTable in envoyproxy#4980, ThreadSystem in envoyproxy#5055 and envoyproxy#5072 and FileSystem in envoyproxy#5031) need a mechanism to plumb data into the system as instantiated during tests. I believe Api::OsSysCalls() is another example that might benefit from this. This class provides a mechanism to have a bounded-scope singleton, such that anywhere an instance of a type T is desired, the same instance will be available. Once all references to a Global go out of scope, the shared instance is destroyed and the pointer to it nulled. Thus, this singleton pattern has the property that between tests, the state will be completely reset, as long as there are no memory leaks. The new class, Test::Global, creates a potential conflict with tests that had declared using testing::Test;, so this PR also cleans those up and adds a format check to avoid having that pattern re-occur. Risk Level: the main risk here is that this class will be abused and used in lieu of proper object passing when that's most appropriate. However, the new library is declared as a bazel test library and is in the test/... tree, so hopefully -- with proper code reviews, the usage will be limited. Testing: //test/... Signed-off-by: Joshua Marantz <jmarantz@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>
2 participants
Description: For the record, I am against singletons in general, and believe it's more maintainable to pass context objects through the call/instantiation stack, as is done in Envoy production code. However the test system employs a deep system of mocks that are constructed without context. As a result, any new context that may be needed in the future (e.g. SymbolTable in #4980, ThreadSystem in #5055 and #5072 and FileSystem in #5031) need a mechanism to plumb data into the system as instantiated during tests. I believe Api::OsSysCalls() is another example that might benefit from this.
This class provides a mechanism to have a bounded-scope singleton, such that anywhere an instance of a type T is desired, the same instance will be available. Once all references to a Global go out of scope, the shared instance is destroyed and the pointer to it nulled.
Thus, this singleton pattern has the property that between tests, the state will be completely reset, as long as there are no memory leaks.
The new class, Test::Global, creates a potential conflict with tests that had declared
using testing::Test;, so this PR also cleans those up and adds a format check to avoid having that pattern re-occur.Risk Level: the main risk here is that this class will be abused and used in lieu of proper object passing when that's most appropriate. However, the new library is declared as a bazel test library and is in the test/... tree, so hopefully -- with proper code reviews, the usage will be limited.
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a