Skip to content

test: Make it easier to use simulated time in integration test, and switch hds_integration_test to use it.#6139

Merged
mattklein123 merged 11 commits intoenvoyproxy:masterfrom
jmarantz:sim-time-hds-integration-test
Mar 3, 2019
Merged

test: Make it easier to use simulated time in integration test, and switch hds_integration_test to use it.#6139
mattklein123 merged 11 commits intoenvoyproxy:masterfrom
jmarantz:sim-time-hds-integration-test

Conversation

@jmarantz
Copy link
Contributor

@jmarantz jmarantz commented Mar 1, 2019

Description:

  • A previous refactor around TimeSystem lifetime in tests left the previous mechanism partially around in a zombie state. This cleans those up.
  • There was a problematic pattern in a couple of integration tests of having the test-class inherit from SimulatedTimeSystem. This failed due to a conflicting implementation of timeSystem(). It's resolved by instead inheriting from a class that contains a SimulatedTimeSystem, which has the same effect of establishing -- for the duration of the test method -- the SimulatedTimeSystem as being the current live time-system singleton.
  • There was a performance problem using SimulatedTimeSystem::waitFor with very short durations, which was fixed using a min().
  • Switches hds_simulation_test to SimulatedTimeSystem.
    Risk Level: low
    Testing: //test/...
    Docs Changes: n/a
    Release Notes: n/a

@jmarantz
Use simulated time in the hds_integration_test, and clean up some det…
6dc95ba 
…ritrus from an older sim-time integration mechanism.

Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Merge branch 'master' into sim-time-hds-integration-test
63d4cac 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
All tests working except mysql_integration_test.
ccaa4db 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Copy link
Contributor Author

jmarantz commented Mar 1, 2019

@sschepens ptal

@jmarantz
Copy link
Contributor Author

jmarantz commented Mar 1, 2019

Hold off on review -- I thought that echo2_integration_test had been brought in and thus I could change this integration test API in one PR now. I'll restore the realTime() and 3-arg ctor form in a bit.

@jmarantz jmarantz closed this Mar 1, 2019
@jmarantz
Revert the removal of realTime() to allow envoy-filter-example to work.
965b7c5 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Copy link
Contributor Author

jmarantz commented Mar 1, 2019

Reverted incompatible changes with envoy-filter-example.

@jmarantz jmarantz reopened this Mar 1, 2019
@sschepens
Copy link
Contributor

sschepens commented Mar 1, 2019

@jmarantz seems legit, thanks for taking the time!

jmarantz added 2 commits March 1, 2019 12:11
@jmarantz
Revert "Revert the removal of realTime() to allow envoy-filter-exampl…
899e3a5 
…e to work."

This reverts commit 965b7c5.

Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Merge branch 'master' into sim-time-hds-integration-test
7473fec 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
dnoe
dnoe previously approved these changes Mar 1, 2019
View reviewed changes
Copy link
Contributor

@dnoe dnoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I think we both might have been confused about required ordering between the filter example and here, and sorry if I added to that confusion.

It looks like CI here failed because filter example failed to build. It's possible there was an implied required ordering we didn't understand, or maybe you just raced your revert here against the update to filter example and things happened in the wrong order?

@dnoe
Copy link
Contributor

dnoe commented Mar 1, 2019

/retest

@repokitteh-read-only
Copy link

repokitteh-read-only bot commented Mar 1, 2019

🔨 rebuilding ci/circleci: tsan (failed build)
🔨 rebuilding ci/circleci: asan (failed build)

🐱

Caused by: a #6139 (comment) was created by @dnoe.

see: more, trace.

@dnoe
Copy link
Contributor

dnoe commented Mar 1, 2019 •
edited
Loading

Looks like you will need to revert 899e3a5 and then clean that up in a follow up PR. Sorry for misleading you.

htuch
htuch previously approved these changes Mar 1, 2019
View reviewed changes
jmarantz added 2 commits March 1, 2019 19:48
@jmarantz
Revert "Revert "Revert the removal of realTime() to allow envoy-filte…
4748e38 
…r-example to work.""

This reverts commit 899e3a5.

Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Merge branch 'master' into sim-time-hds-integration-test
cb190df 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz jmarantz dismissed stale reviews from htuch and dnoe via cb190df March 2, 2019 00:48
jmarantz added 3 commits March 1, 2019 20:09
@jmarantz
Add missing include.
77eb9f5 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Merge branch 'master' into sim-time-hds-integration-test
25a1f6a 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Add comments to mixin class.
3f41ef4 
Signed-off-by: Joshua Marantz <jmarantz@google.com>
mattklein123
mattklein123 approved these changes Mar 3, 2019
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I need to use simulated time in some code I'm going to write soon and this will make it easier!

@mattklein123 mattklein123 merged commit 741df7a into envoyproxy:master Mar 3, 2019
@jmarantz jmarantz deleted the sim-time-hds-integration-test branch March 3, 2019 20:22
@dnoe dnoe mentioned this pull request Mar 4, 2019
Merged
fredlas pushed a commit to fredlas/envoy that referenced this pull request Mar 5, 2019
@jmarantz @fredlas
test: Make it easier to use simulated time in integration test, and s…
cfab71f 
…witch hds_integration_test to use it. (envoyproxy#6139)

Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
@jmarantz jmarantz mentioned this pull request Mar 10, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

@snowp snowp Awaiting requested review from snowp

@zuercher zuercher Awaiting requested review from zuercher

+2 more reviewers

@dnoe dnoe dnoe left review comments

@htuch htuch htuch left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@dnoe dnoe

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jmarantz @sschepens @dnoe @mattklein123 @htuch