test: Make time-system into a global singleton to simplify its use and plumbing, and avoid multi-time-system test confusion.#5708
Merged
jmarantz merged 26 commits intoenvoyproxy:masterfrom Jan 29, 2019
Conversation
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
… template & subclass. Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
jmarantz
changed the title
…Multi-threaded time-system creation really does not work. Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Contributor
Author
|
/retest |
|
🔨 rebuilding |
… on coverage. Signed-off-by: Joshua Marantz <jmarantz@google.com>
…essages. Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Contributor
Author
|
/retest |
|
🔨 rebuilding |
and not the helper. Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
jmarantz
changed the title
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Contributor
Author
|
/retest |
|
🔨 rebuilding |
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Contributor
Author
|
/retest |
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
dnoe
previously approved these changes
Jan 28, 2019
…with it. Signed-off-by: Joshua Marantz <jmarantz@google.com>
dnoe
previously approved these changes
Jan 28, 2019
Contributor
dnoe
left a comment
dnoe
left a comment
There was a problem hiding this comment.
Nice, this bit of doc is a perfect overview. Thanks!
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Contributor
Author
|
/retest |
|
🔨 rebuilding |
Contributor
Author
|
@ggreenway can you give this a senior-committer review? Thanks! |
ggreenway
requested changes
Jan 29, 2019
Member
ggreenway
left a comment
ggreenway
left a comment
There was a problem hiding this comment.
Just one grammar nit, then I think it's good to go. Feel free to merge without me seeing it again.
Signed-off-by: Joshua Marantz <jmarantz@google.com>
ggreenway
approved these changes
Jan 29, 2019
Contributor
Author
|
/retest |
|
🔨 rebuilding |
danzh2010
pushed a commit
to danzh2010/envoy
that referenced
this pull request
Jan 31, 2019
…d plumbing, and avoid multi-time-system test confusion. (envoyproxy#5708) * Enforce TimeSystem being a true singleton. Share delagation logic via template & subclass. * Add description in test/README.md of the time-system and how to test with it. Signed-off-by: Joshua Marantz <jmarantz@google.com>
This was referenced Mar 1, 2019
jmarantz
added a commit
to jmarantz/envoy-filter-example
that referenced
this pull request
Mar 1, 2019
…est-scoped singleton introduced in envoyproxy/envoy#5708 Signed-off-by: Joshua Marantz <jmarantz@google.com>
dnoe
pushed a commit
to envoyproxy/envoy-filter-example
that referenced
this pull request
Mar 1, 2019
…est-scoped singleton introduced in envoyproxy/envoy#5708 (#79) This constructor no longer makes sense in the context of the global test-scoped singleton introduced in envoyproxy/envoy#5708 (#79) Signed-off-by: Joshua Marantz <jmarantz@google.com>
fredlas
pushed a commit
to fredlas/envoy
that referenced
this pull request
Mar 5, 2019
…d plumbing, and avoid multi-time-system test confusion. (envoyproxy#5708) * Enforce TimeSystem being a true singleton. Share delagation logic via template & subclass. * Add description in test/README.md of the time-system and how to test with it. Signed-off-by: Joshua Marantz <jmarantz@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description: There are three variants of the time-system derivation we use in tests: TestTimeSystem: TestRealTimeSystem, SimulatedTimeSystem, and MockTimeSystem (used in only one test). We want to avoid having more than one of these live at a time in a test, as that is hard to reason about. IMO this should ideally be done by plumbing them through the call-stack, but this is impossible to do with our pattern of 0-arg constructor Mocks. So we use Test::Global to manage a reference-counted time-system, which is released and cleared after each test-method.
The drawback of this approach is that constructing a TimeSystem has side effects in that it initializes a global singleton, which is then picked up by mock structures (Dispatcher, Server, FactoryContext) at first use. I don't love this situation but adding constructor-args to Mocks is a significant amount of work and a change in pattern.
The benefit is that this ensures with an ASSERT that no more than one TimeSystem is instantiated at a time.
This breaks out step 1 from #5660 .
Risk Level: low: mainly a risk of comprehension in the test system due to the side effects.
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a