[FLINK-40600][runtime-web] Add missing web dashboard specs and tighten thin ones - #29135
Conversation
8229b6b to
1129e92
Compare
There was a problem hiding this comment.
Suite is green on this branch: 35 files / 158 tests, lint and prettier clean, and the base is 33 / 139, so the numbers in the description hold.
I then mutated the production code these specs cover, and ten mutants survive. They're all in the spots the ticket asked to pin down, so I think it makes sense to close them here rather than in another follow-up. Comments inline. With all of them applied the ten die and the suite stays green at 159 tests.
Nothing blocking. This goes in before FLINK-40601's PR #29138.
| 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'); | ||
| }); |
There was a problem hiding this 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')?
| 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); |
There was a problem hiding this 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.
| 'applications-running': 1, | ||
| 'applications-finished': 1, | ||
| 'applications-cancelled': 0, | ||
| 'applications-failed': 0, |
There was a problem hiding this 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?
| expect(fakeDagre.flush).toHaveBeenCalledWith(mockPlan.nodes, mockPlan.links, true); | ||
| }); |
There was a problem hiding this 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?
|
|
||
| const result = await firstValueFrom(service.loadLogList('tm-1')); | ||
|
|
||
| expect(httpClient.get).toHaveBeenCalledWith(expect.stringContaining('/taskmanagers/tm-1/logs')); |
There was a problem hiding this 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?
| 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 }); |
There was a problem hiding this 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?
…n thin ones FLINK-40117 added Vitest specs for most web dashboard views, but pages/application/overview and pages/application/exceptions still had none, and several existing specs only checked that mocked data was loaded rather than what the component derived from it. Adds the two missing specs, and tightens the timeline, job overview, and cluster overview specs to assert their computed values, expands TaskManagerService coverage beyond loadManagers/loadManager, and replaces the HumanizeDatePipe format-'B' test's reliance on an Angular-internal throw with one driven by an unregistered locale. Generated-by: Claude Code (claude-sonnet-5)
MartijnVisser
left a comment
MartijnVisser
left a comment
There was a problem hiding this comment.
All six addressed, thanks. I re-ran the mutants on this head and they all die now, with the suite green at 159 tests. Approving.
3 participants
What is the purpose of the change
FLINK-40117 added Vitest specs for most of the web dashboard views, but
pages/application/overviewandpages/application/exceptionsstill had none. Some of the added specs also only checked that the mocked data was loaded, not what the component derived from it: the timeline spec didn't assert the computed vertex and subtask ranges, the job overview spec never fed a plan intongOnInit, the cluster overview spec didn't assert the derived statistics, andTaskManagerServiceonly had tests forloadManagersandloadManager. TheHumanizeDatePipetest for format'B'relied on Angular throwing for missing locale data, which an Angular upgrade could change.This adds the two missing specs and tightens the existing ones so they fail when the derived values change.
Brief change log
application-overview.component.spec.tsandapplication-exceptions.component.spec.ts.job-timeline.component.spec.tsto assert the computed vertex ranges (including the duration-derived end-time branch and filtering of not-yet-started vertices) and the per-subtask timeline ranges, instead of just checking counts/names.job-overview.component.spec.tswith a new spec that feeds an actual plan intongOnInitand asserts the derivedpendingNodes/pendingLinksand the resultingdagreComponent.flushcall.overview.component.spec.ts(cluster overview) to assert the exact derived cluster-statistics object instead of just checking for static label text.task-manager.service.spec.tsfrom covering onlyloadManagers/loadManagerto covering all public methods.humanize-date.pipe.spec.tstest that relied on Angular throwing for format'B'with one driven by an unregistered locale, which is a stable part of Angular's i18n contract rather than a version-dependent implementation detail.Verifying this change
This change added tests and can be verified as follows:
npx ng test --watch=falseinflink-runtime-web/web-dashboard: 35 spec files / 158 tests pass (up from the prior baseline of 33 files / 139 tests).npm run lintandnpx prettier --checkare clean on all changed/added files.Does this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (claude-sonnet-5)