-
Notifications
You must be signed in to change notification settings - Fork 11
refactor(dashboards-ng): remove initCompleted output event
#1167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Documentation. Coverage Reports: |
WalkthroughThe PR makes three focused changes: (1) SiGridComponent.addWidgetInstance signature extended to accept an optional Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts (1)
71-78: Same clock/whenStable considerations as previous testThis test uses the same
jasmine.clock().install(); tick(0); await fixture.whenStable();pattern. The same comments apply here:
- Wrap clock usage in try/finally to guarantee
uninstall()even on failure.- Consider adding an explicit
markForCheck()+detectChanges()after stabilization to avoid reliance on implicit initial change detection.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts(1 hunks)projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts(2 hunks)projects/dashboards-ng/src/components/widget-host/si-widget-host.component.ts(0 hunks)projects/dashboards-ng/test/test-widget/test-widget.ts(1 hunks)
💤 Files with no reviewable changes (1)
- projects/dashboards-ng/src/components/widget-host/si-widget-host.component.ts
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: chintankavathia
Repo: siemens/element PR: 1135
File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:953-964
Timestamp: 2025-12-04T05:50:17.637Z
Learning: In the siemens/element repository, when making tree-view tests zoneless-ready, jasmine.clock() should be used to mimic setTimeouts even in zoneless mode, as the component implementation relies on setTimeout calls that need to be controlled in tests.
📚 Learning: 2025-12-08T11:24:45.247Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts:67-85
Timestamp: 2025-12-08T11:24:45.247Z
Learning: In select lazy options tests (projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts), jasmine.clock() cannot control RxJS debounceTime in zoneless mode. Real setTimeout waits must be used instead for tests involving search debouncing.
Applied to files:
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.tsprojects/dashboards-ng/test/test-widget/test-widget.ts
📚 Learning: 2025-12-05T08:00:38.407Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1151
File: projects/element-ng/typeahead/si-typeahead.directive.spec.ts:114-121
Timestamp: 2025-12-05T08:00:38.407Z
Learning: In the typeahead test suite (projects/element-ng/typeahead/si-typeahead.directive.spec.ts), jasmine.clock() must be installed in beforeAll and uninstalled in afterAll (not beforeEach/afterEach) because using per-test clock installation causes schedulerFunction-related issues in these specific tests.
Applied to files:
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.tsprojects/dashboards-ng/test/test-widget/test-widget.ts
📚 Learning: 2025-12-04T05:50:17.637Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1135
File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:953-964
Timestamp: 2025-12-04T05:50:17.637Z
Learning: In the siemens/element repository, when making tree-view tests zoneless-ready, jasmine.clock() should be used to mimic setTimeouts even in zoneless mode, as the component implementation relies on setTimeout calls that need to be controlled in tests.
Applied to files:
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.tsprojects/dashboards-ng/test/test-widget/test-widget.ts
📚 Learning: 2025-12-08T11:25:51.566Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/si-select.component.spec.ts:511-516
Timestamp: 2025-12-08T11:25:51.566Z
Learning: In the select component overflow tests (projects/element-ng/select/si-select.component.spec.ts), jasmine.clock() cannot be used to control the timing for overflow item detection. Real setTimeout waits must be used instead, likely due to ResizeObserver or debounced layout calculations that jasmine.clock() cannot control in zoneless mode.
Applied to files:
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
📚 Learning: 2025-12-01T14:12:11.111Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1104
File: projects/element-ng/datepicker/components/si-day-selection.component.spec.ts:198-202
Timestamp: 2025-12-01T14:12:11.111Z
Learning: In zoneless Angular tests (using provideZonelessChangeDetection), prefer `await fixture.whenStable()` over `fixture.detectChanges()` after dispatching events or triggering async operations, as `whenStable()` waits for all asynchronous operations to complete and better mimics the production environment.
Applied to files:
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.tsprojects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-01T04:01:27.365Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1099
File: projects/element-ng/accordion/si-accordion.component.spec.ts:39-43
Timestamp: 2025-12-01T04:01:27.365Z
Learning: With Angular 20 and esbuild, synchronous `TestBed.configureTestingModule(...).compileComponents()` calls work correctly even when components use external templates (templateUrl), because esbuild pre-compiles templates at build time. The async/await pattern is not required in this scenario.
Applied to files:
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.tsprojects/dashboards-ng/test/test-widget/test-widget.ts
📚 Learning: 2025-12-04T05:50:38.735Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1135
File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:1244-1245
Timestamp: 2025-12-04T05:50:38.735Z
Learning: In zoneless Angular tests with OnPush components, calling `await runOnPushChangeDetection(fixture)` followed by `await fixture.whenStable()` is NOT redundant. The `runOnPushChangeDetection` helper forces change detection to run for OnPush components, while `whenStable()` waits for all async operations to complete. Both are necessary and serve different purposes.
Applied to files:
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.tsprojects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-08T11:25:20.839Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/si-select.component.spec.ts:204-205
Timestamp: 2025-12-08T11:25:20.839Z
Learning: In zoneless Angular tests (using provideZonelessChangeDetection), calling fixture.changeDetectorRef.markForCheck() before fixture.detectChanges() is required even for components using the default change detection strategy (not OnPush). This differs from zone.js-based testing where markForCheck is primarily needed only for OnPush components.
Applied to files:
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
🧬 Code graph analysis (1)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (1)
projects/dashboards-ng/test/test-widget/test-widget.ts (1)
TEST_WIDGET(17-35)
🔇 Additional comments (4)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (1)
218-235: Passing TEST_WIDGET.payload into addWidgetInstance is consistent with the new APIUsing:
component.addWidgetInstance({ widgetId: TEST_WIDGET.id, payload: TEST_WIDGET.payload });correctly exercises the new
payloadsupport onaddWidgetInstanceand keeps the test data aligned withTEST_WIDGET. No issues spotted here.projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts (3)
121-135: setupEditable() test with widget edit actions looks solidThis test:
- Drives async initialization with
jasmine.clock().tick(0)+await fixture.whenStable(), which is appropriate for setTimeout-based flows in zoneless mode (per prior learnings).- Explicitly calls
fixture.changeDetectorRef.markForCheck(); fixture.detectChanges();before invokingcomponent.setupEditable(true).- Verifies both the merged primaryActions (including the widget’s own edit action) and the
widgetInstance!.editableflag.The sequence and assertions look correct and in line with the component’s expected behavior.
137-150: setupEditable() test without widget edit actions correctly covers fallback behaviorBy clearing
component.widgetInstance!.primaryEditActionsand then callingsetupEditable(true), this test validates the fallback to only the grid’s default edit/remove actions and checkswidgetInstance!.editable:
- Async + clock handling matches the previous test.
- Assertions on
primaryActions.length === 2and ordering (editAction, removeAction) look appropriate.The non-null assertion on
widgetInstance!is acceptable here given prior initialization in the same setup path, but if that initialization flow is ever made lazier, this test is where a failure would show up first.
63-69: Clock + whenStable pattern is reasonable; consider explicit detectChanges and try/finallyUsing
jasmine.clock().install(); jasmine.clock().tick(0); await fixture.whenStable();is a sane way to drive setTimeout-based initialization in zoneless tests and aligns with patterns used elsewhere in this repo.Two small robustness suggestions:
- Make the clock teardown exception-safe:
jasmine.clock().install(); try { jasmine.clock().tick(0); await fixture.whenStable(); expect(component.widgetHost().length).toBe(1); } finally { jasmine.clock().uninstall(); }
- You currently never call
fixture.changeDetectorRef.markForCheck(); fixture.detectChanges();in this test. If the widget host instantiation ever depends on an explicit change detection pass instead of just the initial one fromcreateComponent, this may turn flaky. Consider adding an explicit detectChanges after stabilization.⛔ Skipped due to learnings
Learnt from: chintankavathia Repo: siemens/element PR: 1135 File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:953-964 Timestamp: 2025-12-04T05:50:17.637Z Learning: In the siemens/element repository, when making tree-view tests zoneless-ready, jasmine.clock() should be used to mimic setTimeouts even in zoneless mode, as the component implementation relies on setTimeout calls that need to be controlled in tests.Learnt from: chintankavathia Repo: siemens/element PR: 1163 File: projects/element-ng/select/si-select.component.spec.ts:511-516 Timestamp: 2025-12-08T11:25:51.566Z Learning: In the select component overflow tests (projects/element-ng/select/si-select.component.spec.ts), jasmine.clock() cannot be used to control the timing for overflow item detection. Real setTimeout waits must be used instead, likely due to ResizeObserver or debounced layout calculations that jasmine.clock() cannot control in zoneless mode.Learnt from: chintankavathia Repo: siemens/element PR: 1104 File: projects/element-ng/datepicker/components/si-day-selection.component.spec.ts:198-202 Timestamp: 2025-12-01T14:12:11.111Z Learning: In zoneless Angular tests (using provideZonelessChangeDetection), prefer `await fixture.whenStable()` over `fixture.detectChanges()` after dispatching events or triggering async operations, as `whenStable()` waits for all asynchronous operations to complete and better mimics the production environment.Learnt from: chintankavathia Repo: siemens/element PR: 1151 File: projects/element-ng/typeahead/si-typeahead.directive.spec.ts:114-121 Timestamp: 2025-12-05T08:00:38.407Z Learning: In the typeahead test suite (projects/element-ng/typeahead/si-typeahead.directive.spec.ts), jasmine.clock() must be installed in beforeAll and uninstalled in afterAll (not beforeEach/afterEach) because using per-test clock installation causes schedulerFunction-related issues in these specific tests.Learnt from: chintankavathia Repo: siemens/element PR: 1163 File: projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts:67-85 Timestamp: 2025-12-08T11:24:45.247Z Learning: In select lazy options tests (projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts), jasmine.clock() cannot control RxJS debounceTime in zoneless mode. Real setTimeout waits must be used instead for tests involving search debouncing.Learnt from: chintankavathia Repo: siemens/element PR: 1135 File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:1244-1245 Timestamp: 2025-12-04T05:50:38.735Z Learning: In zoneless Angular tests with OnPush components, calling `await runOnPushChangeDetection(fixture)` followed by `await fixture.whenStable()` is NOT redundant. The `runOnPushChangeDetection` helper forces change detection to run for OnPush components, while `whenStable()` waits for all async operations to complete. Both are necessary and serve different purposes.Learnt from: chintankavathia Repo: siemens/element PR: 1163 File: projects/element-ng/select/si-select.component.spec.ts:204-205 Timestamp: 2025-12-08T11:25:20.839Z Learning: In zoneless Angular tests (using provideZonelessChangeDetection), calling fixture.changeDetectorRef.markForCheck() before fixture.detectChanges() is required even for components using the default change detection strategy (not OnPush). This differs from zone.js-based testing where markForCheck is primarily needed only for OnPush components.Learnt from: chintankavathia Repo: siemens/element PR: 1099 File: projects/element-ng/accordion/si-accordion.component.spec.ts:39-43 Timestamp: 2025-12-01T04:01:27.365Z Learning: With Angular 20 and esbuild, synchronous `TestBed.configureTestingModule(...).compileComponents()` calls work correctly even when components use external templates (templateUrl), because esbuild pre-compiles templates at build time. The async/await pattern is not required in this scenario.
532e964 to
79b97cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
♻️ Duplicate comments (2)
projects/dashboards-ng/test/test-widget/test-widget.ts (1)
6-11: Changes already reviewed.The typo fix and static import approach were already covered in a previous review comment.
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts (1)
71-78: Verify initialization without explicit detectChanges.Similar to the previous test, this test doesn't explicitly call
fixture.detectChanges()before checking the widget host state. Verify that the absence of widgets is correctly detected without explicit change detection triggering.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts(1 hunks)projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts(2 hunks)projects/dashboards-ng/src/components/widget-host/si-widget-host.component.ts(0 hunks)projects/dashboards-ng/test/test-widget/test-widget.ts(1 hunks)
💤 Files with no reviewable changes (1)
- projects/dashboards-ng/src/components/widget-host/si-widget-host.component.ts
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-12-04T05:50:38.735Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1135
File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:1244-1245
Timestamp: 2025-12-04T05:50:38.735Z
Learning: In zoneless Angular tests with OnPush components, calling `await runOnPushChangeDetection(fixture)` followed by `await fixture.whenStable()` is NOT redundant. The `runOnPushChangeDetection` helper forces change detection to run for OnPush components, while `whenStable()` waits for all async operations to complete. Both are necessary and serve different purposes.
Applied to files:
projects/dashboards-ng/src/components/grid/si-grid.component.spec.tsprojects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
📚 Learning: 2025-12-01T14:12:11.111Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1104
File: projects/element-ng/datepicker/components/si-day-selection.component.spec.ts:198-202
Timestamp: 2025-12-01T14:12:11.111Z
Learning: In zoneless Angular tests (using provideZonelessChangeDetection), prefer `await fixture.whenStable()` over `fixture.detectChanges()` after dispatching events or triggering async operations, as `whenStable()` waits for all asynchronous operations to complete and better mimics the production environment.
Applied to files:
projects/dashboards-ng/src/components/grid/si-grid.component.spec.tsprojects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
📚 Learning: 2025-12-05T08:00:38.407Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1151
File: projects/element-ng/typeahead/si-typeahead.directive.spec.ts:114-121
Timestamp: 2025-12-05T08:00:38.407Z
Learning: In the typeahead test suite (projects/element-ng/typeahead/si-typeahead.directive.spec.ts), jasmine.clock() must be installed in beforeAll and uninstalled in afterAll (not beforeEach/afterEach) because using per-test clock installation causes schedulerFunction-related issues in these specific tests.
Applied to files:
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.tsprojects/dashboards-ng/test/test-widget/test-widget.ts
📚 Learning: 2025-12-04T05:50:17.637Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1135
File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:953-964
Timestamp: 2025-12-04T05:50:17.637Z
Learning: In the siemens/element repository, when making tree-view tests zoneless-ready, jasmine.clock() should be used to mimic setTimeouts even in zoneless mode, as the component implementation relies on setTimeout calls that need to be controlled in tests.
Applied to files:
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.tsprojects/dashboards-ng/test/test-widget/test-widget.ts
📚 Learning: 2025-12-08T11:24:45.247Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts:67-85
Timestamp: 2025-12-08T11:24:45.247Z
Learning: In select lazy options tests (projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts), jasmine.clock() cannot control RxJS debounceTime in zoneless mode. Real setTimeout waits must be used instead for tests involving search debouncing.
Applied to files:
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.tsprojects/dashboards-ng/test/test-widget/test-widget.ts
📚 Learning: 2025-12-08T11:25:51.566Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/si-select.component.spec.ts:511-516
Timestamp: 2025-12-08T11:25:51.566Z
Learning: In the select component overflow tests (projects/element-ng/select/si-select.component.spec.ts), jasmine.clock() cannot be used to control the timing for overflow item detection. Real setTimeout waits must be used instead, likely due to ResizeObserver or debounced layout calculations that jasmine.clock() cannot control in zoneless mode.
Applied to files:
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
📚 Learning: 2025-12-01T04:01:27.365Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1099
File: projects/element-ng/accordion/si-accordion.component.spec.ts:39-43
Timestamp: 2025-12-01T04:01:27.365Z
Learning: With Angular 20 and esbuild, synchronous `TestBed.configureTestingModule(...).compileComponents()` calls work correctly even when components use external templates (templateUrl), because esbuild pre-compiles templates at build time. The async/await pattern is not required in this scenario.
Applied to files:
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.tsprojects/dashboards-ng/test/test-widget/test-widget.ts
📚 Learning: 2025-12-08T11:25:20.839Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/si-select.component.spec.ts:204-205
Timestamp: 2025-12-08T11:25:20.839Z
Learning: In zoneless Angular tests (using provideZonelessChangeDetection), calling fixture.changeDetectorRef.markForCheck() before fixture.detectChanges() is required even for components using the default change detection strategy (not OnPush). This differs from zone.js-based testing where markForCheck is primarily needed only for OnPush components.
Applied to files:
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
🧬 Code graph analysis (1)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (1)
projects/dashboards-ng/test/test-widget/test-widget.ts (1)
TEST_WIDGET(17-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts (1)
121-135: Async pattern looks good.The test correctly:
- Installs Jasmine clock and advances to trigger initialization
- Waits for stability to allow widget instantiation to complete
- Explicitly triggers change detection with
markForCheck()anddetectChanges()- Calls
setupEditable(true)after initialization is complete- Verifies the edit actions are properly configured
This pattern properly handles the removal of the
initCompletedevent by relying on stabilization instead.
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
Show resolved
Hide resolved
79b97cb to
1216a94
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (1)
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts (1)
138-151: Add initialfixture.detectChanges()and address unresolved past comment on explicit change detection.This test has two issues:
Missing initial
fixture.detectChanges(): The test omits the initial change detection call before installing the Jasmine clock, inconsistent with the first test pattern (line 64).Unaddressed past comment: The previous review correctly noted that explicit change detection should be added after
await fixture.whenStable()and before mutatingcomponent.widgetInstance!.primaryEditActionsat line 142. This ensures the component view/model are synchronized before state mutations and subsequent assertions.Apply this diff to address both issues:
it('should setup default edit actions without widgets edit actions', async () => { + fixture.detectChanges(); jasmine.clock().install(); jasmine.clock().tick(0); await fixture.whenStable(); + fixture.changeDetectorRef.markForCheck(); + fixture.detectChanges(); component.widgetInstance!.primaryEditActions = undefined; expect(component.primaryActions.length).toBe(0); component.setupEditable(true); expect(component.primaryActions.length).toBe(2); expect(component.primaryActions[0]).toBe(component.editAction); expect(component.primaryActions[1]).toBe(component.removeAction); expect(component.widgetInstance!.editable).toBeTrue(); jasmine.clock().uninstall(); });Based on learnings, in zoneless Angular tests, explicit change detection is important for state synchronization even with default change detection strategy components.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts(1 hunks)projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts(2 hunks)projects/dashboards-ng/src/components/widget-host/si-widget-host.component.ts(0 hunks)projects/dashboards-ng/test/test-widget/test-widget.ts(1 hunks)
💤 Files with no reviewable changes (1)
- projects/dashboards-ng/src/components/widget-host/si-widget-host.component.ts
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-12-01T04:01:27.365Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1099
File: projects/element-ng/accordion/si-accordion.component.spec.ts:39-43
Timestamp: 2025-12-01T04:01:27.365Z
Learning: With Angular 20 and esbuild, synchronous `TestBed.configureTestingModule(...).compileComponents()` calls work correctly even when components use external templates (templateUrl), because esbuild pre-compiles templates at build time. The async/await pattern is not required in this scenario.
Applied to files:
projects/dashboards-ng/test/test-widget/test-widget.tsprojects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
📚 Learning: 2025-12-08T11:24:45.247Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts:67-85
Timestamp: 2025-12-08T11:24:45.247Z
Learning: In select lazy options tests (projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts), jasmine.clock() cannot control RxJS debounceTime in zoneless mode. Real setTimeout waits must be used instead for tests involving search debouncing.
Applied to files:
projects/dashboards-ng/test/test-widget/test-widget.tsprojects/dashboards-ng/src/components/grid/si-grid.component.spec.tsprojects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
📚 Learning: 2025-12-04T05:50:17.637Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1135
File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:953-964
Timestamp: 2025-12-04T05:50:17.637Z
Learning: In the siemens/element repository, when making tree-view tests zoneless-ready, jasmine.clock() should be used to mimic setTimeouts even in zoneless mode, as the component implementation relies on setTimeout calls that need to be controlled in tests.
Applied to files:
projects/dashboards-ng/test/test-widget/test-widget.tsprojects/dashboards-ng/src/components/grid/si-grid.component.spec.tsprojects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
📚 Learning: 2025-12-08T11:25:51.566Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/si-select.component.spec.ts:511-516
Timestamp: 2025-12-08T11:25:51.566Z
Learning: In the select component overflow tests (projects/element-ng/select/si-select.component.spec.ts), jasmine.clock() cannot be used to control the timing for overflow item detection. Real setTimeout waits must be used instead, likely due to ResizeObserver or debounced layout calculations that jasmine.clock() cannot control in zoneless mode.
Applied to files:
projects/dashboards-ng/src/components/grid/si-grid.component.spec.tsprojects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
📚 Learning: 2025-12-04T05:50:38.735Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1135
File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:1244-1245
Timestamp: 2025-12-04T05:50:38.735Z
Learning: In zoneless Angular tests with OnPush components, calling `await runOnPushChangeDetection(fixture)` followed by `await fixture.whenStable()` is NOT redundant. The `runOnPushChangeDetection` helper forces change detection to run for OnPush components, while `whenStable()` waits for all async operations to complete. Both are necessary and serve different purposes.
Applied to files:
projects/dashboards-ng/src/components/grid/si-grid.component.spec.tsprojects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
📚 Learning: 2025-12-01T14:12:11.111Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1104
File: projects/element-ng/datepicker/components/si-day-selection.component.spec.ts:198-202
Timestamp: 2025-12-01T14:12:11.111Z
Learning: In zoneless Angular tests (using provideZonelessChangeDetection), prefer `await fixture.whenStable()` over `fixture.detectChanges()` after dispatching events or triggering async operations, as `whenStable()` waits for all asynchronous operations to complete and better mimics the production environment.
Applied to files:
projects/dashboards-ng/src/components/grid/si-grid.component.spec.tsprojects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
📚 Learning: 2025-12-05T08:00:38.407Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1151
File: projects/element-ng/typeahead/si-typeahead.directive.spec.ts:114-121
Timestamp: 2025-12-05T08:00:38.407Z
Learning: In the typeahead test suite (projects/element-ng/typeahead/si-typeahead.directive.spec.ts), jasmine.clock() must be installed in beforeAll and uninstalled in afterAll (not beforeEach/afterEach) because using per-test clock installation causes schedulerFunction-related issues in these specific tests.
Applied to files:
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
📚 Learning: 2025-12-08T11:25:20.839Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/si-select.component.spec.ts:204-205
Timestamp: 2025-12-08T11:25:20.839Z
Learning: In zoneless Angular tests (using provideZonelessChangeDetection), calling fixture.changeDetectorRef.markForCheck() before fixture.detectChanges() is required even for components using the default change detection strategy (not OnPush). This differs from zone.js-based testing where markForCheck is primarily needed only for OnPush components.
Applied to files:
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
📚 Learning: 2025-12-02T06:50:25.778Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1104
File: projects/element-ng/datepicker/components/si-day-selection.component.spec.ts:19-19
Timestamp: 2025-12-02T06:50:25.778Z
Learning: The `runOnPushChangeDetection` helper is located at `projects/element-ng/test-helpers/change-detection.helper.ts` and is shared across the element-ng project, not scoped under individual feature modules like datepicker.
Applied to files:
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
🧬 Code graph analysis (1)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (1)
projects/dashboards-ng/test/test-widget/test-widget.ts (1)
TEST_WIDGET(17-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts (1)
63-70: LGTM! Proper initialization pattern established.This test correctly calls
fixture.detectChanges()before installing the Jasmine clock, ensuring component initialization runs before async timing control begins. This pattern should be followed consistently across all tests in this suite.
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
Show resolved
Hide resolved
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
Show resolved
Hide resolved
spike-rabbit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
`initCompleted` has no functional use but it was used just for handling tests. adjusted tests with immediate promise resolver so we don't have to rely on explicit `initCompleted` event subscriptions.
1216a94 to
d39cd13
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/dashboards-ng/test/test-widget/test-widget.ts (1)
5-15: TightenmoduleLoadertyping (avoidPromise<any>) to catch export drift early.Right now
loaderFunctionreturnsPromise<any>, which can hide broken exports fromprojects/dashboards-ng/test/test-widget/index. Consider typing it to the module namespace.-import { Widget, WidgetConfig } from '@siemens/dashboards-ng'; +import { Widget, WidgetConfig } from '@siemens/dashboards-ng'; import * as testWidgetModule from 'projects/dashboards-ng/test/test-widget/index'; -const loaderFunction = (name: string): Promise<any> => { +type TestWidgetModule = typeof testWidgetModule; +const loaderFunction = (name: string): Promise<TestWidgetModule> => { if (name === 'TestWidgetComponent' || name === 'TestWidgetEditorComponent') { // immediately resolve so we don't have to deal with timers in tests return Promise.resolve(testWidgetModule); } else { throw new Error(`Unknown component to be loaded ${name}`); } };
♻️ Duplicate comments (3)
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts (3)
72-79: Add initialfixture.detectChanges()before clock (zoneless init won’t run otherwise).Without an initial
fixture.detectChanges(),ngOnInit/widget attachment may never execute in zoneless tests, making this test prone to false positives/flakes.it('should not create widget instance without widget', async () => { gridService.widgetCatalog.set([]); + fixture.detectChanges(); jasmine.clock().install(); jasmine.clock().tick(0); await fixture.whenStable(); expect(component.widgetHost().length).toBe(0); jasmine.clock().uninstall(); });
122-136: Run initial change detection before installing clock in#setupEditable()tests.These tests depend on widget instantiation; in zoneless mode that typically requires an initial
fixture.detectChanges()before any clock/tick/whenStable flow.it('should setup default edit actions with widgets edit actions', async () => { + fixture.detectChanges(); jasmine.clock().install(); jasmine.clock().tick(0); await fixture.whenStable(); expect(component.primaryActions.length).toBe(0); fixture.changeDetectorRef.markForCheck(); fixture.detectChanges(); component.setupEditable(true); expect(component.primaryActions.length).toBe(3); expect((component.primaryActions[0] as MenuItem).title).toBe('Hello User'); expect(component.primaryActions[1]).toBe(component.editAction); expect(component.primaryActions[2]).toBe(component.removeAction); expect(component.widgetInstance!.editable).toBeTrue(); jasmine.clock().uninstall(); });
138-151: Add CD sync before mutatingwidgetInstancein zoneless mode (and add initial detectChanges).This test mutates
component.widgetInstance!.primaryEditActionsafterwhenStable()but without syncing change detection; in zoneless runs this can be flaky. Also add the initialfixture.detectChanges()like the other tests.it('should setup default edit actions without widgets edit actions', async () => { + fixture.detectChanges(); jasmine.clock().install(); jasmine.clock().tick(0); await fixture.whenStable(); + fixture.changeDetectorRef.markForCheck(); + fixture.detectChanges(); component.widgetInstance!.primaryEditActions = undefined; expect(component.primaryActions.length).toBe(0); component.setupEditable(true); expect(component.primaryActions.length).toBe(2); expect(component.primaryActions[0]).toBe(component.editAction); expect(component.primaryActions[1]).toBe(component.removeAction); expect(component.widgetInstance!.editable).toBeTrue(); jasmine.clock().uninstall(); });
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts(1 hunks)projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts(2 hunks)projects/dashboards-ng/src/components/widget-host/si-widget-host.component.ts(0 hunks)projects/dashboards-ng/test/test-widget/test-widget.ts(1 hunks)
💤 Files with no reviewable changes (1)
- projects/dashboards-ng/src/components/widget-host/si-widget-host.component.ts
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: chintankavathia
Repo: siemens/element PR: 1135
File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:953-964
Timestamp: 2025-12-04T05:50:17.637Z
Learning: In the siemens/element repository, when making tree-view tests zoneless-ready, jasmine.clock() should be used to mimic setTimeouts even in zoneless mode, as the component implementation relies on setTimeout calls that need to be controlled in tests.
📚 Learning: 2025-12-11T10:08:54.597Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1151
File: projects/element-ng/auto-collapsable-list/si-auto-collapsable-list.directive.spec.ts:83-104
Timestamp: 2025-12-11T10:08:54.597Z
Learning: In the auto-collapsable-list directive tests (projects/element-ng/auto-collapsable-list/si-auto-collapsable-list.directive.spec.ts), jasmine.clock() successfully controls the directive's setTimeout-based layout logic in zoneless mode. Mock timers work fine here, unlike in select component overflow tests where real setTimeout waits are required.
Applied to files:
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.tsprojects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-08T11:24:45.247Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts:67-85
Timestamp: 2025-12-08T11:24:45.247Z
Learning: In select lazy options tests (projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts), jasmine.clock() cannot control RxJS debounceTime in zoneless mode. Real setTimeout waits must be used instead for tests involving search debouncing.
Applied to files:
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.tsprojects/dashboards-ng/src/components/grid/si-grid.component.spec.tsprojects/dashboards-ng/test/test-widget/test-widget.ts
📚 Learning: 2025-12-08T11:25:51.566Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/si-select.component.spec.ts:511-516
Timestamp: 2025-12-08T11:25:51.566Z
Learning: In the select component overflow tests (projects/element-ng/select/si-select.component.spec.ts), jasmine.clock() cannot be used to control the timing for overflow item detection. Real setTimeout waits must be used instead, likely due to ResizeObserver or debounced layout calculations that jasmine.clock() cannot control in zoneless mode.
Applied to files:
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.tsprojects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-04T05:50:17.637Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1135
File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:953-964
Timestamp: 2025-12-04T05:50:17.637Z
Learning: In the siemens/element repository, when making tree-view tests zoneless-ready, jasmine.clock() should be used to mimic setTimeouts even in zoneless mode, as the component implementation relies on setTimeout calls that need to be controlled in tests.
Applied to files:
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.tsprojects/dashboards-ng/src/components/grid/si-grid.component.spec.tsprojects/dashboards-ng/test/test-widget/test-widget.ts
📚 Learning: 2025-12-01T14:12:11.111Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1104
File: projects/element-ng/datepicker/components/si-day-selection.component.spec.ts:198-202
Timestamp: 2025-12-01T14:12:11.111Z
Learning: In zoneless Angular tests (using provideZonelessChangeDetection), prefer `await fixture.whenStable()` over `fixture.detectChanges()` after dispatching events or triggering async operations, as `whenStable()` waits for all asynchronous operations to complete and better mimics the production environment.
Applied to files:
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.tsprojects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-05T08:00:38.407Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1151
File: projects/element-ng/typeahead/si-typeahead.directive.spec.ts:114-121
Timestamp: 2025-12-05T08:00:38.407Z
Learning: In the typeahead test suite (projects/element-ng/typeahead/si-typeahead.directive.spec.ts), jasmine.clock() must be installed in beforeAll and uninstalled in afterAll (not beforeEach/afterEach) because using per-test clock installation causes schedulerFunction-related issues in these specific tests.
Applied to files:
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
📚 Learning: 2025-12-04T05:50:38.735Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1135
File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:1244-1245
Timestamp: 2025-12-04T05:50:38.735Z
Learning: In zoneless Angular tests with OnPush components, calling `await runOnPushChangeDetection(fixture)` followed by `await fixture.whenStable()` is NOT redundant. The `runOnPushChangeDetection` helper forces change detection to run for OnPush components, while `whenStable()` waits for all async operations to complete. Both are necessary and serve different purposes.
Applied to files:
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.tsprojects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-01T04:01:27.365Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1099
File: projects/element-ng/accordion/si-accordion.component.spec.ts:39-43
Timestamp: 2025-12-01T04:01:27.365Z
Learning: With Angular 20 and esbuild, synchronous `TestBed.configureTestingModule(...).compileComponents()` calls work correctly even when components use external templates (templateUrl), because esbuild pre-compiles templates at build time. The async/await pattern is not required in this scenario.
Applied to files:
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.tsprojects/dashboards-ng/test/test-widget/test-widget.ts
📚 Learning: 2025-12-08T11:25:20.839Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/si-select.component.spec.ts:204-205
Timestamp: 2025-12-08T11:25:20.839Z
Learning: In zoneless Angular tests (using provideZonelessChangeDetection), calling fixture.changeDetectorRef.markForCheck() before fixture.detectChanges() is required even for components using the default change detection strategy (not OnPush). This differs from zone.js-based testing where markForCheck is primarily needed only for OnPush components.
Applied to files:
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
🧬 Code graph analysis (2)
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts (2)
playwright/support/test-helpers.ts (1)
expect(18-18)projects/element-ng/docs.ts (1)
MenuItem(111-111)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (1)
projects/dashboards-ng/test/test-widget/test-widget.ts (1)
TEST_WIDGET(17-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (1)
218-235: Passingpayloadhere is justified; ensure widget templates tolerate missing payload elsewhere.This test now triggers full widget rendering (
await fixture.whenStable()), so providingTEST_WIDGET.payloadprevents template errors. Just ensure production widgets handlepayloadbeing absent (since it’s optional in the API) and avoid implicitly making it required via template access without null guards.
initCompletedhas no functional use but it was used just for handling tests. adjusted tests with immediate promise resolver so we don't have to rely on explicitinitCompletedevent subscriptions.Refactor: Remove
initCompletedoutput eventRemoves the unused
initCompletedoutput event fromSiWidgetHostComponentthat served no functional purpose and was only used for testing. Tests have been refactored to eliminate their dependency on this event by using Jasmine clock ticks,fixture.whenStable(), and Promise-based control flow instead of event subscriptions.Changes:
initCompletedoutput declaration and its two emission sites fromSiWidgetHostComponentfixture.whenStable(), and explicit change detection instead of subscribing to initialization eventsfixture.whenStable()for better control flowPromise.resolve()for synchronous immediate resolution instead of dynamic imports