test(frontend): share workflow-editor TestBed setup, drop double-run - #5626
Conversation
….spec.ts Move the six mouse-event tests that were commented out in PR apache#5146 into a new sibling file workflow-editor.browser.spec.ts with its own TestBed setup. The .browser.spec.ts naming convention routes the file to the test-browser target (Vitest + Playwright Chromium) and excludes it from the default jsdom test target, where these tests fail because jsdom does not implement the JointJS event paths they exercise. Also remove the now-orphan imports the commented-out blocks left behind in workflow-editor.component.spec.ts (NzModalRef, mockCommentBox, createYTypeFromObject, createJQueryEvent helper, nzModalService local). Bundle the optional reviewer nit from PR apache#5146 on applyOperatorBorder: accept an optional Validation parameter so the validation stream handler can pass its already-computed result instead of triggering a recomputation. The operator-add subscription falls through to the lazy fetch path since validation hasn't run yet at that point. Closes apache#5318 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
/request-review @Xiao-zhen-Liu |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5626 +/- ##
============================================
- Coverage 53.02% 52.61% -0.42%
+ Complexity 2542 2485 -57
============================================
Files 1075 1076 +1
Lines 41743 41638 -105
Branches 4513 4490 -23
============================================
- Hits 22134 21906 -228
- Misses 18301 18446 +145
+ Partials 1308 1286 -22
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@Xiao-zhen-Liu Please review the PR. |
|
cc @Xiao-zhen-Liu for review. |
…o match convention
Xiao-zhen-Liu
left a comment
There was a problem hiding this comment.
Clean, well-scoped cleanup. Moving the shared setup into one file means the two test files can't slowly fall out of sync, and routing the mouse tests into a .browser.spec.ts file is the right way to kill the double-run — bonus that six dead commented-out tests now actually run.
One note: the diff also changes real app code in workflow-editor.component.ts (an optional validation param) that isn't in the description. Correct and harmless, but it makes a test-only PR not quite test-only — I'd surface it or split it out. Nothing blocking otherwise.
|
I resolved comments based on discussions. @Xiao-zhen-Liu it might be better for you to check and resolve comments as you posted comments and have context. Now this PR is good to go! |
…pache#5626) ### What changes were proposed in this PR? Addresses the two review comments on [apache#5318](apache#5318 follow-up implementation: 1. Share a common TestBed setup so the jsdom and browser specs don't drift. The .browser.spec.ts split created two TestBed configurations for the same component - one in workflow-editor.component.spec.ts (External Module Integration describe), one in workflow-editor.browser.spec.ts. Both configured nearly identical imports/providers arrays, which would inevitably drift over time. Extracted them into a new sibling file workflow-editor.test-utils.ts exporting two arrays: export const workflowEditorTestImports = [ ... ]; export const workflowEditorTestProviders: Provider[] = [ ... ]; This follows the existing project convention in [frontend/src/app/common/testing/test-utils.ts](vscode-webview://1epki5h79lmkghv36u4evg54fuvmk17ndjca203mcg7opnnd5sg9/frontend/src/app/common/testing/test-utils.ts) (which exports commonTestImports and commonTestProviders the same way). Each spec's TestBed now collapses to: await TestBed.configureTestingModule({ imports: workflowEditorTestImports, providers: workflowEditorTestProviders, }).compileComponents(); Adding or removing a service from either spec's setup is now a single edit in one file. 2. Drop the explicit workflow-editor.component.spec.ts entry from the test-browser include in angular.json. With the six mouse-event tests now living in workflow-editor.browser.spec.ts (picked up by the **/*.browser.spec.ts glob), the explicit listing of workflow-editor.component.spec.ts in test-browser's include array was causing the file's 25 jsdom-friendly tests to run twice, once in jsdom (the default test target) and once in real Chrome (the test-browser target). Removed that explicit entry; the test-browser target now picks up only **/*.browser.spec.ts files. Scope note. The pre-existing JointJS Paper describe block at the top of workflow-editor.component.spec.ts has its own deliberately-different TestBed setup (ContextMenuComponent instead of NzModalCommentBoxComponent, Overlay, MockComputingUnitStatusService) and was left untouched, it wasn't part of the duplication introduced by the .browser.spec.ts split. ### Any related issues, documentation, discussions? Addresses review feedback on the follow-up PR for [apache#5318](apache#5318). Closes apache#5318 Related to apache#3614 / PR apache#5146 (this PR restores tests that were commented out during PR apache#5146 as collateral from the apache#3614 fix). ### How was this PR tested? Existing test runs: Verified no test-count regressions in either target, and the double-run is gone: ng test (jsdom, full suite): 947 pass / 0 fail / 2 skipped / 1 todo across 103 files ng run gui:test-browser: 13 pass / 0 fail across 2 files (6 from workflow-editor.browser.spec.ts + 7 from the pre-existing code-editor.component.browser.spec.ts) workflow-editor.component.spec.ts under jsdom: 25/25 pass (unchanged from before this PR) Browser test count dropped from 38 -> 13, confirming the 25 jsdom-friendly tests in workflow-editor.component.spec.ts no longer double-run in real Chrome. Static checks: tsc --noEmit and eslint clean on all four changed files. ### Was this PR authored or co-authored using generative AI tooling? Co-authored-by: Claude Code (Anthropic Claude Opus 4.7) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ratorBorder (apache#5702) ### What changes were proposed in this PR? WorkflowEditorComponent.applyOperatorBorder(operatorID)` always recomputes validation as its first step: ```typescript const validation = this.validationWorkflowService.validateOperator(operatorID); ``` This is redundant when the helper is called from the validation-stream subscriber in handleOperatorValidation, the stream's emitted event already carries the Validation result that was just computed by updateValidationState. This PR: - Adds an optional validation?: Validation parameter to applyOperatorBorder. The helper uses it when provided, and falls back to validateOperator(operatorID) otherwise. - Updates handleOperatorValidation to pass value.validation from the stream into the helper. - Leaves the operator-add stream subscriber unchanged and hence it doesn't have a Validation in hand at that point, so it correctly falls through to the lazy-fetch path. - Functionally identical (the stream emits the same Validation that would have been recomputed), purely avoids the duplicate call. Originally flagged as an optional nit during the PR apache#5146 review. Attempted in PR apache#5626 but split out as per the reviewer's request so that PR could stay scoped to test restructuring; this PR completes the follow-up. ### Any related issues, documentation, discussions? Closes apache#5683 Related: PR apache#5146 (where the nit was raised), PR apache#5626 (where this was attempted and split out). ### How was this PR tested? Added one focused unit test in `workflow-editor.component.spec.ts` inside the existing `operator border restoration after navigation` describe block: `it("uses the Validation passed in instead of recomputing it", ...)`. The test clears the `validateOperator` spy after the operator-add validation chain settles, then calls `applyOperatorBorder` directly with a `Validation` argument and asserts `validateOperator` is not called. Verified locally: - `tsc --noEmit`: clean - `eslint`: clean - `ng test` (jsdom): 26/26 pass in the editor spec (was 25, new test adds one) - `ng run gui:test-browser`: 13/13 pass ### Was this PR authored or co-authored using generative AI tooling? This PR was co-authored using Claude Code (Anthropic Claude Opus 4.7)
What changes were proposed in this PR?
Addresses the two review comments on #5318's follow-up implementation:
The .browser.spec.ts split created two TestBed configurations for the same component - one in workflow-editor.component.spec.ts (External Module Integration describe), one in workflow-editor.browser.spec.ts. Both configured nearly identical imports/providers arrays, which would inevitably drift over time.
Extracted them into a new sibling file workflow-editor.test-utils.ts exporting two arrays:
export const workflowEditorTestImports = [ ... ];
export const workflowEditorTestProviders: Provider[] = [ ... ];
This follows the existing project convention in frontend/src/app/common/testing/test-utils.ts (which exports commonTestImports and commonTestProviders the same way). Each spec's TestBed now collapses to:
await TestBed.configureTestingModule({
imports: workflowEditorTestImports,
providers: workflowEditorTestProviders,
}).compileComponents();
Adding or removing a service from either spec's setup is now a single edit in one file.
With the six mouse-event tests now living in workflow-editor.browser.spec.ts (picked up by the **/.browser.spec.ts glob), the explicit listing of workflow-editor.component.spec.ts in test-browser's include array was causing the file's 25 jsdom-friendly tests to run twice, once in jsdom (the default test target) and once in real Chrome (the test-browser target). Removed that explicit entry; the test-browser target now picks up only **/.browser.spec.ts files.
Scope note. The pre-existing JointJS Paper describe block at the top of workflow-editor.component.spec.ts has its own deliberately-different TestBed setup (ContextMenuComponent instead of NzModalCommentBoxComponent, Overlay, MockComputingUnitStatusService) and was left untouched, it wasn't part of the duplication introduced by the .browser.spec.ts split.
Any related issues, documentation, discussions?
Addresses review feedback on the follow-up PR for #5318.
Closes #5318
Related to #3614 / PR #5146 (this PR restores tests that were commented out during PR #5146 as collateral from the #3614 fix).
How was this PR tested?
Existing test runs: Verified no test-count regressions in either target, and the double-run is gone:
ng test (jsdom, full suite): 947 pass / 0 fail / 2 skipped / 1 todo across 103 files
ng run gui:test-browser: 13 pass / 0 fail across 2 files (6 from workflow-editor.browser.spec.ts + 7 from the pre-existing code-editor.component.browser.spec.ts)
workflow-editor.component.spec.ts under jsdom: 25/25 pass (unchanged from before this PR)
Browser test count dropped from 38 -> 13, confirming the 25 jsdom-friendly tests in workflow-editor.component.spec.ts no longer double-run in real Chrome.
Static checks: tsc --noEmit and eslint clean on all four changed files.
Was this PR authored or co-authored using generative AI tooling?
Co-authored-by: Claude Code (Anthropic Claude Opus 4.7)