Remove TestEventsLimit(), and minor cleanups#35844
Merged
vdemeester merged 3 commits intomoby:masterfrom Dec 22, 2017
Merged
Conversation
This test was added a long time ago, and over the years has proven to be flaky, and slow. To address those issues, it was modified to; - cleanup containers afterwards - take clock-skew into account - improve performance by parallelizing the container runs - _reduce_ parallelization to address platform issues on Windows (twice..) - adjust the test to take new limits into account - adjust the test to account for more events being generated by containers The last change to this test (made in ddae20c) actually broke the test, as it's now testing that all events sent by containers (`numContainers*eventPerContainer`) are received, but the number of events that is generated (17 containers * 7 events = 119) is less than the limit (256 events). The limit is already covered by the `TestLogEvents` unit-test, that was added in 8d05642, and tests that the number of events is limited to `eventsLimit`. This patch removes the test, because it's not needed. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Commit 59d45c3 changed the `eventsLimit` from 64 to 256, but did not update the GoDoc accordingly. This patch updates the GoDoc for `Subscribe` and `SubscribeTopic` to match the actual limit. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The `Status` field was deprecated in favor of `Action`. This patch updates the test to use the `Action` field, but adds a check that both are set to the same value. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Member
Author
|
Wow, what's this error https://jenkins.dockerproject.org/job/Docker-PRs-powerpc/7619/console? This is a bit odd; where's the reference |
Member
Author
AkihiroSuda
approved these changes
Dec 20, 2017
thaJeztah
referenced
this pull request
Dec 20, 2017
Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.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
6 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.
This test was added a long time ago, and over the years has proven to be flaky (#28873, #22601, #25533, #21473, #13498), and slow (using 30 containers, and taking over a minute to run (#12675)).
To address those issues, it was modified to;
The last change to this test (made in ddae20c#diff-9e07f7c1f4d16eb428d628f85ee1a6f1R125) actually broke the test, as it's now testing that all events sent by containers (
numContainers*eventPerContainer) are received, but the number of events that is generated (17 containers * 7 events = 119) is less than the limit (256 events).The limit is already covered by the
TestLogEventsunit-test that was added in #7452, and tests that the number of events is limited toeventsLimit.This patch removes the test, because it's not needed.
Two other changes were added;