Add v0.8 integration tests for Angular explorer#1437
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive suite of unit tests for v0.8 components in the Angular explorer and updates the test utilities to support version-specific example loading. Feedback highlights potential TypeScript compilation errors in the test utilities, unsafe type casting, and inconsistent import paths. Additionally, the reviewer pointed out fragile testing patterns, such as hardcoded wait times and selecting UI elements by index, which should be refactored to improve test reliability.
| component.version = version; | ||
| component.onVersionChange({target: {value: version}} as unknown as Event); |
There was a problem hiding this comment.
The DemoComponent class (as defined in demo.component.ts) does not appear to have a version property or an onVersionChange method. Accessing these members will cause TypeScript compilation errors. Please ensure DemoComponent is updated to include these members or verify if they were intended to be added in this PR.
| if (version === '0.8') { | ||
| if (!example) { | ||
| example = examples.find(ex => ex.name === `${exampleName} (basic)`); | ||
| } | ||
| if (!example) { | ||
| example = examples.find(ex => ex.name === `${exampleName} (minimal)`); | ||
| } | ||
| } |
There was a problem hiding this comment.
This fallback logic for finding examples can be simplified for better readability.
if (version === '0.8' && !example) {
example = examples.find(ex => ex.name === `${exampleName} (basic)`) ||
examples.find(ex => ex.name === `${exampleName} (minimal)`);
}|
|
||
| // Set version and trigger change | ||
| component.version = version; | ||
| component.onVersionChange({target: {value: version}} as unknown as Event); |
There was a problem hiding this comment.
Casting a plain object to unknown and then to Event is unsafe and bypasses type checking. If onVersionChange expects a DOM event, it's better to dispatch a real Event on the corresponding element or use a properly typed mock/interface.
| import {ComponentFixture} from '@angular/core/testing'; | ||
| import {DemoComponent} from '../../demo.component'; | ||
| import {getCanvas, loadExample, wait} from '../test_utils'; | ||
| import {A2UIClientEventMessage} from 'src/v0_8/types'; |
There was a problem hiding this comment.
The import path src/v0_8/types is inconsistent with other test files (e.g., 03_calendar-day.spec.ts) which use @a2ui/web_core/v0_8. This may lead to module resolution errors depending on your build configuration.
| import {A2UIClientEventMessage} from 'src/v0_8/types'; | |
| import {A2UIClientEventMessage} from '@a2ui/web_core/v0_8'; |
| sendBtn.click(); | ||
| fixture.detectChanges(); | ||
|
|
||
| await wait(10); |
There was a problem hiding this comment.
Using hardcoded wait(10) can lead to flaky tests, especially in environments with varying performance. Consider using a more robust synchronization method, such as polling for the expected event in eventsLog or using Angular's fakeAsync and tick() utilities.
| const buttons = fixture.nativeElement.querySelectorAll('a2ui-button button'); | ||
| expect(buttons.length).toBeGreaterThan(1); | ||
| const discardBtn = buttons[1]; |
There was a problem hiding this comment.
Selecting buttons by index (buttons[1]) is fragile and prone to breaking if the UI layout changes. It is safer to select the button based on its text content or a more specific CSS class/attribute.
1 participant
This PR adds integration tests for v0.8 examples to the Angular explorer app. It puts them in a new tests/v0_8/ directory and updates test_utils.ts to support loading examples for both versions. It also adds the missing JSON example for the modal in v0.8.