MartijnVisser commented on code in PR #29135: URL: https://github.com/apache/flink/pull/29135#discussion_r3986385656
########## flink-runtime-web/web-dashboard/src/app/pages/application/overview/application-overview.component.spec.ts: ########## @@ -0,0 +1,96 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { ComponentFixture, TestBed } from '@angular/core/testing'; +import { Router } from '@angular/router'; +import { of } from 'rxjs'; + +import { ApplicationDetail, JobsItem } from '@flink-runtime-web/interfaces'; +import { JobService, StatusService } from '@flink-runtime-web/services'; +import { NzMessageService } from 'ng-zorro-antd/message'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { ApplicationOverviewComponent } from './application-overview.component'; +import { ApplicationLocalService } from '../application-local.service'; + +const runningJob = { + jid: 'job-1', + name: 'Streaming ETL', + state: 'RUNNING', + 'start-time': 200, + 'end-time': -1, + duration: 50, + completed: false +} as JobsItem; + +const finishedJob = { + jid: 'job-2', + name: 'Batch Report', + state: 'FINISHED', + 'start-time': 50, + 'end-time': 150, + duration: 100, + completed: true +} as JobsItem; + +const mockApplicationDetail = { + id: 'app-1', + jobs: [runningJob, finishedJob] +} as ApplicationDetail; + +describe('ApplicationOverviewComponent', () => { + let fixture: ComponentFixture<ApplicationOverviewComponent>; + let element: HTMLElement; + const navigate = vi.fn().mockResolvedValue(true); + + beforeEach(async () => { + navigate.mockClear(); + // JobListComponent (rendered twice by this component) depends on StatusService, JobService + // and NzMessageService; since jobData$ is passed in directly, they're never actually called. + await TestBed.configureTestingModule({ + imports: [ApplicationOverviewComponent], + providers: [ + { provide: ApplicationLocalService, useValue: { applicationDetailChanges: () => of(mockApplicationDetail) } }, + { provide: Router, useValue: { navigate } }, + { provide: StatusService, useValue: {} }, + { provide: JobService, useValue: {} }, + { provide: NzMessageService, useValue: {} } + ] + }).compileComponents(); + fixture = TestBed.createComponent(ApplicationOverviewComponent); + element = fixture.nativeElement as HTMLElement; + }); + + it('splits the application jobs into running and completed lists', () => { + fixture.detectChanges(); + + const text = element.textContent ?? ''; + expect(text).toContain('Running Job List'); + expect(text).toContain('Completed Job List'); + expect(text).toContain('Streaming ETL'); + expect(text).toContain('Batch Report'); + }); Review Comment: These assertions only check that both names appear somewhere in the component. I swapped `[completed]` between the two `flink-job-list` children and the spec stayed green, so the split it's named for isn't actually checked. Can you assert per list instead, via `element.querySelectorAll('flink-job-list')`? ########## flink-runtime-web/web-dashboard/src/app/pages/application/exceptions/application-exceptions.component.spec.ts: ########## @@ -0,0 +1,91 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { ComponentFixture, TestBed } from '@angular/core/testing'; +import { of } from 'rxjs'; + +import { ApplicationDetail, ApplicationExceptions } from '@flink-runtime-web/interfaces'; +import { ApplicationService } from '@flink-runtime-web/services'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { ApplicationExceptionsComponent } from './application-exceptions.component'; +import { ApplicationLocalService } from '../application-local.service'; + +const mockApplicationDetail = { id: 'app-1' } as ApplicationDetail; + +describe('ApplicationExceptionsComponent', () => { + let fixture: ComponentFixture<ApplicationExceptionsComponent>; + const loadExceptions = vi.fn(); + + beforeEach(async () => { + loadExceptions.mockReset(); + await TestBed.configureTestingModule({ + imports: [ApplicationExceptionsComponent], + providers: [ + { provide: ApplicationService, useValue: { loadExceptions } }, + { provide: ApplicationLocalService, useValue: { applicationDetailChanges: () => of(mockApplicationDetail) } } + ] + }).compileComponents(); + fixture = TestBed.createComponent(ApplicationExceptionsComponent); + }); + + it('formats the most recent exception with its timestamp and related job', () => { + const mockExceptions: ApplicationExceptions = { + exceptionHistory: { + entries: [ + { + exceptionName: 'java.lang.RuntimeException', + stacktrace: 'java.lang.RuntimeException: boom\n\tat com.example.Foo.bar(Foo.java:42)', + timestamp: 1_781_000_000_000, + jobId: 'job-1' + } + ] + } + }; + loadExceptions.mockReturnValue(of(mockExceptions)); + + fixture.detectChanges(); + + expect(loadExceptions).toHaveBeenCalledWith('app-1'); + expect(fixture.componentInstance.rootException).toContain('Related Job: job-1'); + expect(fixture.componentInstance.rootException).toContain('java.lang.RuntimeException: boom'); + expect(fixture.componentInstance.isLoading).toBe(false); Review Comment: The test is named for the timestamp and the most recent entry, but neither is pinned. I switched the mask to `shortTime`, and read `entries[entries.length - 1]` instead of `entries[0]`, and both kept the spec green. Could you add an older second entry and assert the shape of the first line, something like `expect(rootException.split('\n')[0]).toMatch(/^\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}$/)`? The shape rather than a value, because `formatDate` is called here without a time zone. ########## flink-runtime-web/web-dashboard/src/app/pages/overview/overview.component.spec.ts: ########## @@ -108,6 +108,26 @@ describe('OverviewComponent', () => { expect(text).toContain('Completed Application List'); }); + it('derives the cluster statistics from the applications and overview data', async () => { + fixture.detectChanges(); + + const stats = await firstValueFrom(fixture.componentInstance.statisticData$); + + expect(stats).toEqual({ + 'applications-running': 1, + 'applications-finished': 1, + 'applications-cancelled': 0, + 'applications-failed': 0, Review Comment: `applications-running` and `applications-finished` are both 1 here, and the other two both 0, so the four filters in `overview.component.ts` are interchangeable. I swapped RUNNING with FINISHED, and CANCELED with FAILED, and the spec stayed green both times. Can you add a second running application and a cancelled one to `mockApplications`, and expect 2 / 1 / 1 / 0 here? ########## flink-runtime-web/web-dashboard/src/app/pages/job/overview/job-overview.component.spec.ts: ########## @@ -98,3 +98,71 @@ describe('JobOverviewComponent', () => { ); }); }); + +describe('JobOverviewComponent with a resolved plan', () => { + // The Dagre graph relies on SVG layout APIs jsdom does not implement, so the component is + // constructed directly (bypassing TestBed/change detection) and given a fake dagreComponent, + // rather than trying to render the real child through the view. + const mockPlan: JobDetailCorrect['plan'] = { + jid: 'job-1', + name: 'Test Job', + type: 'STREAMING', + nodes: [{ id: 'vertex-a' } as NodesItemCorrect], + links: [] as NodesItemLink[], + streamNodes: [ + { id: 'node-a', job_vertex_id: 'vertex-a' } as NodesItemCorrect, + { id: 'node-b' } as NodesItemCorrect + ], + streamLinks: [{ id: 'link-1', source: 'node-a', target: 'node-b' } as NodesItemLink] + }; + + function createComponent(): { + component: JobOverviewComponent; + fakeDagre: { showPendingOperators: boolean; flush: ReturnType<typeof vi.fn>; updateNode: ReturnType<typeof vi.fn> }; + } { + const fakeDagre = { + showPendingOperators: false, + flush: vi.fn().mockResolvedValue(undefined), + updateNode: vi.fn() + }; + const component = new JobOverviewComponent( + {} as unknown as Router, + activatedRoute as unknown as ActivatedRoute, + {} as ElementRef, + { + loadMetricsWithAllAggregates: vi.fn().mockReturnValue(of({})), + loadWatermarks: vi.fn().mockReturnValue(of({ lowWatermark: NaN })) + } as unknown as MetricsService, + { + jobDetailChanges: () => of({ jid: 'job-1', plan: mockPlan } as JobDetailCorrect), + selectedVertexChanges: () => EMPTY + } as unknown as JobLocalService, + {} as unknown as JobService, + {} as unknown as NzNotificationService, + { markForCheck: vi.fn() } as unknown as ChangeDetectorRef + ); + (component as unknown as { dagreComponent: typeof fakeDagre }).dagreComponent = fakeDagre; + return { component, fakeDagre }; + } + + it('derives pending nodes and links from the streaming graph when a plan arrives', () => { + const { component } = createComponent(); + component.ngOnInit(); + + expect(component.nodes).toEqual(mockPlan.nodes); + expect(component.pendingNodes).toEqual([{ id: 'node-b' }]); + // The pending link's endpoints are remapped through the streaming-graph node ids onto + // their job-vertex ids, so 'node-a' becomes 'vertex-a' while the still-pending 'node-b' + // (no job vertex yet) is left as-is. + expect(component.pendingLinks).toEqual([ + { id: 'vertex-a-node-b', source: 'vertex-a', target: 'node-b', pending: true } + ]); + }); + + it('flushes the dagre graph with the resolved nodes and links', () => { + const { component, fakeDagre } = createComponent(); + component.ngOnInit(); + + expect(fakeDagre.flush).toHaveBeenCalledWith(mockPlan.nodes, mockPlan.links, true); + }); Review Comment: `fakeDagre.showPendingOperators` is `false` in both tests, so `refreshGraph` never takes the branch that uses the `pendingNodes`/`pendingLinks` you assert above. I dropped them from that `flush` call and the spec stayed green. Could you add a case with it set to `true`? ########## flink-runtime-web/web-dashboard/src/app/services/task-manager.service.spec.ts: ########## @@ -79,4 +80,130 @@ describe('TaskManagerService', () => { expect(emissions).toEqual([]); }); }); + + describe('loadLogList', () => { + it('extracts the logs array from the response', async () => { + const log = { name: 'jobmanager.log', size: 100, mtime: 1 } as TaskManagerLogItem; + httpClient.get.mockReturnValue(of({ logs: [log] })); + + const result = await firstValueFrom(service.loadLogList('tm-1')); + + expect(httpClient.get).toHaveBeenCalledWith(expect.stringContaining('/taskmanagers/tm-1/logs')); Review Comment: These URLs are matched by substring, so a wrong endpoint still passes. I pointed `loadLogs` at `/logs`, appended a character to `loadLog`'s `url`, and defaulted `loadThreadDump` to `?mode=lite`, and the spec stayed green each time. Can you assert the full URL off the same `ConfigService`, like `jar.service.spec.ts` does? ########## flink-runtime-web/web-dashboard/src/app/services/task-manager.service.spec.ts: ########## @@ -79,4 +80,130 @@ describe('TaskManagerService', () => { expect(emissions).toEqual([]); }); }); + + describe('loadLogList', () => { + it('extracts the logs array from the response', async () => { + const log = { name: 'jobmanager.log', size: 100, mtime: 1 } as TaskManagerLogItem; + httpClient.get.mockReturnValue(of({ logs: [log] })); + + const result = await firstValueFrom(service.loadLogList('tm-1')); + + expect(httpClient.get).toHaveBeenCalledWith(expect.stringContaining('/taskmanagers/tm-1/logs')); + expect(result).toEqual([log]); + }); + }); + + describe('loadLog', () => { + it('pairs the raw log text with the request url', async () => { + httpClient.get.mockReturnValue(of('log contents')); + + const result = await firstValueFrom(service.loadLog('tm-1', 'jobmanager.log')); + + expect(result.data).toBe('log contents'); + expect(result.url).toContain('/taskmanagers/tm-1/logs/jobmanager.log'); + }); + }); + + describe('loadThreadDump', () => { + it('joins the stringified thread infos into a single dump', async () => { + httpClient.get.mockReturnValue( + of({ threadInfos: [{ stringifiedThreadInfo: 'thread-A\n' }, { stringifiedThreadInfo: 'thread-B\n' }] }) + ); + + const result = await firstValueFrom(service.loadThreadDump('tm-1')); + + expect(result).toBe('thread-A\nthread-B\n'); + }); + + it('appends the mode query parameter when provided', async () => { + httpClient.get.mockReturnValue(of({ threadInfos: [] })); + + await firstValueFrom(service.loadThreadDump('tm-1', 'full')); + + expect(httpClient.get).toHaveBeenCalledWith(expect.stringContaining('mode=full')); + }); + }); + + describe('loadLogs', () => { + it('returns the raw log text for the given TaskManager', async () => { + httpClient.get.mockReturnValue(of('log contents')); + + const result = await firstValueFrom(service.loadLogs('tm-1')); + + expect(httpClient.get).toHaveBeenCalledWith(expect.stringContaining('/taskmanagers/tm-1/log'), expect.anything()); + expect(result).toBe('log contents'); + }); + }); + + describe('loadStdout', () => { + it('returns the raw stdout text for the given TaskManager', async () => { + httpClient.get.mockReturnValue(of('stdout contents')); + + const result = await firstValueFrom(service.loadStdout('tm-1')); + + expect(httpClient.get).toHaveBeenCalledWith( + expect.stringContaining('/taskmanagers/tm-1/stdout'), + expect.anything() + ); + expect(result).toBe('stdout contents'); + }); + }); + + describe('loadMetrics', () => { + it('parses the metric values into a numeric map keyed by id', async () => { + httpClient.get.mockReturnValue(of([{ id: 'Status.JVM.CPU.Load', value: '0.42' }])); + + const result = await firstValueFrom(service.loadMetrics('tm-1', ['Status.JVM.CPU.Load'])); + + expect(result).toEqual({ 'Status.JVM.CPU.Load': 0.42 }); Review Comment: `listOfMetricName.join(',')` isn't pinned here: the list has one element and the `get` params are never asserted, so changing the separator survives. Can you pass two metric names and assert the call? -- 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]
