[service-bus] Closing some open areas where we could mask errors, and fixing flaky tests to be more reliable. #15761
Conversation
This file contains 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
ghost
added
the
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
richardpark-msft
changed the title
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
… what's been used as the standard time in other libraries - Fixes to simplify the flaky tests. Mostly just reducing the number of non-deterministic events (connections restarting, etc..) that were being heavily depended on, and just changing them to manually initiate the events we need instead (ie, force detach). - Fixed a spot in receiveMessages() where, if the link had been closed, we'd falsly return an empty array instead of throwing an exception indicating the link closed.
richardpark-msft
force-pushed
the
sb-flakytests-disconnects
branch
from
2e20b72
to
67b662f
Compare
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run js - service-bus - tests |
1 similar comment
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Co-authored-by: chradek <51000525+chradek@users.noreply.github.com>
…ft/azure-sdk-for-js into sb-flakytests-disconnects
|
/azp run js - service-bus - tests |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
|
/azp run js - service-bus - tests |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
- The "open a receiver on a non-existent entity" should wait a bit longer since establishing the connection can be slow if network conditions are bad. - Updating to throw a more realistic error inside of the onDetached() call (it'll be a non-retryable exception, not a ServiceBusError like I used).
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…added when I was trying to allow use of a deprecated field) - Fixing assert to look for SessionLockLost error with peekLock+session disconnection tests.
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
1 run passed with latest fixes. |
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Another! |
|
/azp run js - service-bus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
chradek
approved these changes
Jun 21, 2021
|
The failure that's in there currently is not related to my change - it's in the streaming receiver (and sessions to boot!). I'm going to go ahead and commit this PR, and focus on the session flaky test next. |
|
/check-enforcer override |
openapi-sdkautomation bot
pushed a commit
to AzureSDKAutomation/azure-sdk-for-js
that referenced
this pull request
Aug 26, 2021
Correct stable 2020 01 01 (Azure#15761) * Remove definitions that don't exist in 2020-01-01 and extract Incident Severity to a definition. * Use IncidentSeverityEnum instead of defining an incorrect enum which have 'Critical' value. * Remove duplicate definition of IncidentInfo which is already defined in Bookmarks.json * Remove Settings from SecurityInsights.json * Make prettier Co-authored-by: Anat Gilenson <anatgilenson@microsoft.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Making changes to simplify a flaky test (and hopefully make it more reliable).
The main issue with the 'handle interrupted detach' method was that it relied on too many moving parts to work reliably. We could just eternally loop like we'd expect customers to do, but in the end we have a very simple test we're trying to perform - we want to receive, and while we're in the process of draining, cause a detach and have it early exit and reject/resolve immediately rather than waiting for the timeout.
I reworked the test to make that simpler by just removing the unneeded connection.idle() and just calling directly into the onDetached method. Because it happens prior to rhea even seeing that we're draining we should reliably win that race each time.
There were a couple of other things changed for this PR as well:
Fixes #13461