-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(HTTPSend): Allow HTTPSend commands to be sent in separate sequential queues (SOFIE-3431) #346
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release51 #346 +/- ##
=============================================
- Coverage 55.73% 55.72% -0.01%
=============================================
Files 129 129
Lines 10119 10124 +5
Branches 2520 2520
=============================================
+ Hits 5640 5642 +2
+ Misses 4477 4105 -372
- Partials 2 377 +375 ☔ View full report in Codecov by Sentry. |
@@ -591,11 +591,10 @@ export class QuantelManager extends EventEmitter { | |||
}) | |||
} | |||
public clearAllWaitWithPort(portId: string) { | |||
if (!this._waitWithPorts[portId]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a bug task already for this SOFIE-3419, as this is not a recent mistake so needs a thorough test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not see any use of clearAllWaitWithPort
but it seemed clearly wrong, so I just changed it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right yeah, this quantel one isn't called anymore due to a mistake in the refactoring. so I guess this can be in whatever state you prefer as it is currently dead code. It is also refactored in r52 too, so expect a minor merge conflict there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the quantel related change needs to be reverted, other than that looks good
Co-authored-by: Julian Waller <git@julusian.co.uk>
About the Contributor
This pull request is posted on behalf of the NRK.
Type of Contribution
This is a:
Bug fix
Current Behavior
All commands in an HTTPSend device are sent sequentially, ignoring the
queueId
property.New Behavior
HTTPSend device again supports the queueId property to allow separate sequential request queues.
Testing Instructions
Other Information
Status