mengw15 commented on code in PR #4377:
URL: https://github.com/apache/texera/pull/4377#discussion_r3077432168
##########
frontend/src/app/workspace/service/workflow-websocket/workflow-websocket.service.ts:
##########
@@ -131,8 +133,8 @@ export class WorkflowWebsocketService {
this.webSocketResponseSubject.next(event as TexeraWebsocketEvent)
);
- // refresh connection status
- this.websocketEvent().subscribe(evt => {
+ // refresh connection status — store the reference so it is torn down in
closeWebsocket()
Review Comment:
Maybe not change the comment here, remove " — store the reference so it is
torn down in closeWebsocket()" to make this comment consistent with other
comments. I think there is no need to mention the solution in the codebase, as
it is included in the PR
##########
frontend/src/app/workspace/service/workflow-websocket/workflow-websocket.service.spec.ts:
##########
@@ -34,4 +34,32 @@ describe("WorkflowWebsocketService", () => {
it("should be created", () => {
expect(service).toBeTruthy();
});
+
Review Comment:
Both tests bypass openWebsocket and manually assign a Subscription to the
private statusUpdateSubscription field. That means the actual bug path,
"calling openWebsocket N times leaves N listeners on the subject because the
returned Subscription was discarded", is not exercised. Is there a way that
calls openWebsocket multiple times (mocking webSocket) and then asserts there
is exactly one active subscription. If so, I think one test case would be
enough.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
