MartijnVisser commented on code in PR #29135: URL: https://github.com/apache/flink/pull/29135#discussion_r3986385667
########## 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: there is one entry, and the `yyyy-MM-dd HH:mm:ss` mask is never asserted, so switching it to `shortTime` or reading `entries[entries.length - 1]` both survive. An older second entry plus the shape of the first line closes both — the shape rather than a value, because `formatDate` is called without a time zone: ```suggestion 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' }, { exceptionName: 'java.lang.IllegalStateException', stacktrace: 'java.lang.IllegalStateException: older', timestamp: 1_780_000_000_000, jobId: 'job-0' } ] } }; loadExceptions.mockReturnValue(of(mockExceptions)); fixture.detectChanges(); const rootException = fixture.componentInstance.rootException; expect(loadExceptions).toHaveBeenCalledWith('app-1'); // First line is the timestamp; assert its shape rather than a value that depends on the // machine's time zone. expect(rootException.split('\n')[0]).toMatch(/^\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}$/); expect(rootException).toContain('Related Job: job-1'); expect(rootException).toContain('java.lang.RuntimeException: boom'); expect(rootException).not.toContain('older'); expect(fixture.componentInstance.isLoading).toBe(false); ``` ########## 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: The URLs are matched by substring throughout this file, so a wrong endpoint still passes: pointing `loadLogs` at `/logs`, or appending a character to `loadLog`'s `url`, keeps the spec green. `loadThreadDump` without a mode asserts no URL at all, so defaulting it to `?mode=lite` survives too. Assert the full URL off the same `ConfigService`, the way `jar.service.spec.ts` does — hold it in a variable in `beforeEach` (`configService = new ConfigService()`), then: ```suggestion expect(httpClient.get).toHaveBeenCalledWith(`${configService.BASE_URL}/taskmanagers/tm-1/logs`); ``` and the same for `loadLog`, `loadThreadDump` (both cases), `loadLogs`, `loadStdout`, `loadProfilingList`, `createProfilingInstance` and `loadProfilingResult`. ########## 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` asserted above — dropping them from that `flush` call keeps this spec green. One more case covers it: ```suggestion expect(fakeDagre.flush).toHaveBeenCalledWith(mockPlan.nodes, mockPlan.links, true); }); it('appends the pending nodes and links to the flush when pending operators are shown', () => { const { component, fakeDagre } = createComponent(); fakeDagre.showPendingOperators = true; component.ngOnInit(); expect(fakeDagre.flush).toHaveBeenCalledWith( [...mockPlan.nodes, { id: 'node-b' }], [{ id: 'vertex-a-node-b', source: 'vertex-a', target: 'node-b', pending: true }], true ); }); ``` ########## 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 and the other two both 0, so the four `filter` predicates in `overview.component.ts` are interchangeable: swapping RUNNING with FINISHED, or CANCELED with FAILED, keeps this spec green. Please make the counts distinct by adding to `mockApplications`: ```ts { id: 'app-3', name: 'second-running-app', status: 'RUNNING', 'start-time': 120, 'end-time': -1, duration: 5, completed: false, jobs: { ...emptyJobStatus, RUNNING: 1 } }, { id: 'app-4', name: 'cancelled-app', status: 'CANCELED', 'start-time': 10, 'end-time': 20, duration: 10, completed: true, jobs: { ...emptyJobStatus, CANCELED: 1 } } ``` and expecting 2 / 1 / 1 / 0 here. Both swaps then fail. ########## 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, so swapping `[completed]` between the two `flink-job-list` children keeps the spec green — the split it is named for is not checked. Assert per list: ```suggestion it('feeds the application jobs into a running and a completed job list', () => { fixture.detectChanges(); const [running, completed] = Array.from(element.querySelectorAll('flink-job-list')); expect(running.textContent).toContain('Running Job List'); expect(running.textContent).toContain('Streaming ETL'); expect(running.textContent).not.toContain('Batch Report'); expect(completed.textContent).toContain('Completed Job List'); expect(completed.textContent).toContain('Batch Report'); expect(completed.textContent).not.toContain('Streaming ETL'); }); ``` ########## 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(',')` is unpinned: the list has one element and the `get` params are never asserted, so changing the separator survives. Two names and an assertion on the call: ```suggestion httpClient.get.mockReturnValue( of([ { id: 'Status.JVM.CPU.Load', value: '0.42' }, { id: 'Status.JVM.Memory.Heap.Used', value: '1024' } ]) ); const result = await firstValueFrom( service.loadMetrics('tm-1', ['Status.JVM.CPU.Load', 'Status.JVM.Memory.Heap.Used']) ); expect(httpClient.get).toHaveBeenCalledWith(`${configService.BASE_URL}/taskmanagers/tm-1/metrics`, { params: { get: 'Status.JVM.CPU.Load,Status.JVM.Memory.Heap.Used' } }); expect(result).toEqual({ 'Status.JVM.CPU.Load': 0.42, 'Status.JVM.Memory.Heap.Used': 1024 }); ``` -- 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]
