kunwp1 commented on code in PR #8854:
URL: https://github.com/apache/texera/pull/8854#discussion_r4321086204
##########
frontend/src/app/common/service/computing-unit/computing-unit-status/computing-unit-status.service.ts:
##########
@@ -145,7 +146,8 @@ export class ComputingUnitStatusService implements
OnDestroy {
this.refreshSubscription = this.refreshComputingUnitListSignal
.pipe(
- switchMap(() => this.computingUnitService.listComputingUnits()),
+ // A failed fetch ends only that request, not the refresh stream.
+ switchMap(() =>
this.computingUnitService.listComputingUnits().pipe(catchError(() => EMPTY))),
Review Comment:
`catchError(() => EMPTY)` swallows more than just HTTP failures. The mapping
step (`parseDashboardUnit`) runs inside the wrapped observable so a bug there
would now fail silently every tick. Previously it left one error in the
console. Could we keep returning `EMPTY` but log anything that isn't an
`HttpErrorResponse`? The same applies to the poll above.
##########
frontend/src/app/common/service/computing-unit/computing-unit-status/computing-unit-status.service.spec.ts:
##########
@@ -328,6 +390,62 @@ describe("ComputingUnitStatusService", () => {
}
});
+ it("keeps polling the selected unit after one failed fetch", () => {
+ vi.useFakeTimers();
+ try {
+ const managing = TestBed.inject(WorkflowComputingUnitManagingService);
+ const polled = { computingUnit: { cuid: 3 }, status: "Running" } as
unknown as DashboardWorkflowComputingUnit;
+ const getSpy = vi
+ .spyOn(managing, "getComputingUnit")
+ .mockReturnValueOnce(throwError(() => new Error("boom")))
+ .mockReturnValue(of(polled));
+ (service as any).allComputingUnitsSubject.next([mockUnit(3)]);
+ (service as any).selectedUnitSubject.next(mockUnit(3));
+
+ (service as any).startPollingSelectedUnit(3);
+ vi.advanceTimersByTime((service as any).REFRESH_INTERVAL_MS * 5);
+
+ // The failed first poll must not end the interval: every later tick
still fetches.
+ expect(getSpy).toHaveBeenCalledTimes(5);
+ expect(service.getSelectedComputingUnitValue()).toBe(polled);
+ } finally {
+ (service as any).stopPollingSelectedUnit();
+ vi.useRealTimers();
+ }
+ });
+
+ it("a failed poll keeps the poll alive and leaves the selection intact", ()
=> {
+ vi.useFakeTimers();
+ try {
+ const managing = TestBed.inject(WorkflowComputingUnitManagingService);
+ const original = mockUnit(3);
+ const polled = { computingUnit: { cuid: 3 }, status: "Running" } as
unknown as DashboardWorkflowComputingUnit;
+ const getSpy = vi
+ .spyOn(managing, "getComputingUnit")
+ .mockReturnValueOnce(throwError(() => new Error("boom")))
+ .mockReturnValue(of(polled));
+ const stopSpy = vi.spyOn(service as any, "stopPollingSelectedUnit");
+ (service as any).allComputingUnitsSubject.next([original]);
+ (service as any).selectedUnitSubject.next(original);
+
+ (service as any).startPollingSelectedUnit(3);
+ vi.advanceTimersByTime((service as any).REFRESH_INTERVAL_MS);
+ expect(getSpy).toHaveBeenCalledTimes(1);
+
+ // The failed poll leaves the selected unit object as it was and the
poll subscription open.
+ expect(service.getSelectedComputingUnitValue()).toBe(original);
+ expect(stopSpy).not.toHaveBeenCalled();
+ expect((service as any).selectedUnitPoll.closed).toBe(false);
Review Comment:
Same for the two poll tests. This one already shows the poll survives a
failed tick, leaves the selection alone, and picks up the next good result, so
the five-tick test above is mostly a subset of it.
##########
frontend/src/app/common/service/computing-unit/computing-unit-status/computing-unit-status.service.spec.ts:
##########
@@ -257,6 +257,68 @@ describe("ComputingUnitStatusService", () => {
expect(latest).toEqual(newUnits);
});
+ it("keeps refreshing the list after one failed list fetch", () => {
Review Comment:
The three list tests overlap quite a bit. The emission count test below
already shows that the stream survives a failure, emits nothing for it, and
emits the good list next. Could these be one test?
--
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]
