Fix up boadcasts, tests, and logging across the board#1400
Merged
NickCraver merged 27 commits intomasterfrom Mar 23, 2020
Merged
Fix up boadcasts, tests, and logging across the board#1400NickCraver merged 27 commits intomasterfrom
NickCraver merged 27 commits intomasterfrom
Conversation
Thie changes the broadcast to be only before the master reconfiguration to both before *and* after - a fix following 88dcf0c to fix the gap. Also fixes tests by overall lessening runtime (and thus buld server time), fixes a few static timeouts to be conditional (so they short circuit faster if met), and brings some tests that were some variant of the above into RELEASE since they're safe now. Also, we're locking the output because...that was jacked up in races.
added 23 commits
March 21, 2020 13:34
This isn't determistic - that's okay. We just want to see that it happened.
Spin instead of assume worst case in the constructor
These are jumping the gun in a row
Contributor
|
Looking good! |
added 2 commits
March 21, 2020 22:08
This needs more love, quick fix for now
Collaborator
Author
|
@mgravell Should be good for review - this isn't perfect but does fix more of the flaky. I propose we merge this so I can get the other PRs using fewer builds to get green - we're eating build minutes on flaky nonsense at the moment. This also just generally reduces the arbitrary pausing (instead spin checking the condition we want with a timeout) to eat fewer build minutes as well. Our builds basically doubled in expense when Sentinel landed due to these pauses. I should have paid more attention then. |
mgravell
reviewed
Mar 23, 2020
Collaborator
|
oh, ignore my suggestion; didn't realize that was a test; in that case: don't care about a |
mgravell
approved these changes
Mar 23, 2020
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
3 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 changes does the following:
UntilConditionto take aTimeSpan, just because clearer. Even though I IntelliSense completed.FromMinutes()earlier and watched it like an idiot for a while...I stand by this decision!ITestOutputHelperwriter because...that was jacked up in races:Note: a lot of the test changes are just optimizations to delays which allow longer but short-circuit sooner. The important changes are in broadcast and locking around the test runner. I can think of downsides to neither, but want some @mgravell eyes. This should resolve a lot of flaky-ness with local (and build agent) tests. Not all of it, but a lot of it!