React add integration tests#1625
Conversation
Adds a few classnames to some of the basic catalog components so they can be located in tests in a similar way to those on Lit and Angular: Card, ChoicePicker, Modal and Tabs. Adds handling for the DateTimeInput value so it conforms with HTML5 values.
There was a problem hiding this comment.
Code Review
This pull request adds missing CSS classes to several basic catalog components (Card, ChoicePicker, Modal, and Tabs) to align with other implementations, fixes value rendering in DateTimeInput, uses structuredClone for message processing in the explorer app, refactors test utilities to handle React's act blocks correctly, and introduces a comprehensive suite of integration tests for v0_9 examples. The review feedback highlights a parsing bug in normalizeDateTimeValue when handling date-only strings for datetime-local inputs, identifies test assertions in 32_advanced-form-validator.test.tsx that check a stale textContent variable instead of the live DOM, and recommends replacing several unsafe non-null assertions (!) on lookup results with optional chaining (?.) in the test files.
| function normalizeDateTimeValue(value: string | null | undefined, type: string): string { | ||
| if (!value) return ''; | ||
|
|
||
| const hasT = value.includes('T'); | ||
| const split = value.split('T'); | ||
|
|
||
| const datePart = (hasT ? split[0] : value)?.substring(0, 10) ?? ''; | ||
| const timePart = (hasT ? split[1] : value)?.substring(0, 5) ?? ''; | ||
|
|
||
| switch (type) { | ||
| case 'date': | ||
| return datePart; | ||
| case 'time': | ||
| return timePart; | ||
| case 'datetime-local': | ||
| return `${datePart}T${timePart}`; | ||
| } | ||
| return ''; | ||
| } |
There was a problem hiding this comment.
The current implementation of normalizeDateTimeValue has a bug when handling date-only strings (e.g., "2025-12-28") for a datetime-local input type. Since hasT is false, timePart is evaluated as value.substring(0, 5) which results in "2025-". This produces a malformed normalized value of "2025-12-28T2025-", causing the input element to fail to render the value correctly.
We should parse and separate the date and time parts more robustly based on whether the string contains date separators (-) or time separators (:).
function normalizeDateTimeValue(value: string | null | undefined, type: string): string {
if (!value) return '';
const hasT = value.includes('T');
let datePart = '';
let timePart = '';
if (hasT) {
const split = value.split('T');
datePart = split[0];
timePart = split[1];
} else if (value.includes('-')) {
datePart = value;
} else if (value.includes(':')) {
timePart = value;
}
datePart = datePart.substring(0, 10);
timePart = timePart.substring(0, 5);
switch (type) {
case 'date':
return datePart;
case 'time':
return timePart;
case 'datetime-local':
return datePart && timePart ? `${datePart}T${timePart}` : datePart ? `${datePart}T00:00` : '';
}
return '';
}
| it('should show error for invalid email', async () => { | ||
| const inputs = Array.from(surface.querySelectorAll('input')) as HTMLInputElement[]; | ||
| const textInputs = inputs.filter(i => i.type === 'text' || !i.type); | ||
|
|
||
| const emailInput = textInputs[0]; | ||
| await act(async () => { | ||
| emailInput.value = 'invalid-email'; | ||
| emailInput.dispatchEvent(new Event('input')); | ||
| await whenSettled(); | ||
| }); | ||
|
|
||
| expect(textContent).toContain('Invalid email format'); | ||
| }); | ||
|
|
||
| it('should show error for invalid phone', async () => { | ||
| const inputs = Array.from(surface.querySelectorAll('input')) as HTMLInputElement[]; | ||
| const textInputs = inputs.filter(i => i.type === 'text' || !i.type); | ||
|
|
||
| const phoneInput = textInputs[1]; | ||
| await act(async () => { | ||
| phoneInput.value = '123'; | ||
| phoneInput.dispatchEvent(new Event('input')); | ||
| await whenSettled(); | ||
| }); | ||
|
|
||
| expect(textContent).toContain('Invalid phone format'); | ||
| }); | ||
|
|
||
| it('should show error for invalid zip', async () => { | ||
| const inputs = Array.from(surface.querySelectorAll('input')) as HTMLInputElement[]; | ||
| const textInputs = inputs.filter(i => i.type === 'text' || !i.type); | ||
|
|
||
| const zipInput = textInputs[2]; | ||
| await act(async () => { | ||
| zipInput.value = '1234'; | ||
| zipInput.dispatchEvent(new Event('input')); | ||
| await whenSettled(); | ||
| }); | ||
|
|
||
| expect(textContent).toContain('Must be exactly 5 digits'); | ||
| }); |
There was a problem hiding this comment.
The assertions in these test cases are checking the stale textContent variable that was populated in beforeEach before the input values were changed. Since textContent is a primitive string, it does not automatically update when the DOM changes.
To fix this, assert against surface.textContent directly so that the updated DOM state is checked.
it('should show error for invalid email', async () => {
const inputs = Array.from(surface.querySelectorAll('input')) as HTMLInputElement[];
const textInputs = inputs.filter(i => i.type === 'text' || !i.type);
const emailInput = textInputs[0];
await act(async () => {
emailInput.value = 'invalid-email';
emailInput.dispatchEvent(new Event('input'));
await whenSettled();
});
expect(surface.textContent).toContain('Invalid email format');
});
it('should show error for invalid phone', async () => {
const inputs = Array.from(surface.querySelectorAll('input')) as HTMLInputElement[];
const textInputs = inputs.filter(i => i.type === 'text' || !i.type);
const phoneInput = textInputs[1];
await act(async () => {
phoneInput.value = '123';
phoneInput.dispatchEvent(new Event('input'));
await whenSettled();
});
expect(surface.textContent).toContain('Invalid phone format');
});
it('should show error for invalid zip', async () => {
const inputs = Array.from(surface.querySelectorAll('input')) as HTMLInputElement[];
const textInputs = inputs.filter(i => i.type === 'text' || !i.type);
const zipInput = textInputs[2];
await act(async () => {
zipInput.value = '1234';
zipInput.dispatchEvent(new Event('input'));
await whenSettled();
});
expect(surface.textContent).toContain('Must be exactly 5 digits');
}
| const tab = tabs.find(t => t.textContent.includes('Ingredients'))!; | ||
| expect(tab).withContext('Should find Ingredients tab').toBeTruthy(); | ||
|
|
||
| await act(async () => { | ||
| tab.click(); | ||
| await whenSettled(); |
There was a problem hiding this comment.
Avoid using the non-null assertion operator (!) on lookup results (such as Array.prototype.find) that may fail. Instead, allow the variable to be nullable and handle the undefined case downstream using optional chaining (?.).
const tab = tabs.find(t => t.textContent.includes('Ingredients'));
expect(tab).withContext('Should find Ingredients tab').toBeTruthy();
await act(async () => {
tab?.click();
await whenSettled();
}
References
- Avoid using the non-null assertion operator (
!) on lookup results (such asArray.prototype.find) that may fail. Instead, allow the variable to be nullable and handle theundefinedcase downstream using optional chaining (?.) or nullish coalescing (??).
| const tab = tabs.find(t => t.textContent.includes('Instructions'))!; | ||
| expect(tab).withContext('Should find Instructions tab').toBeTruthy(); | ||
|
|
||
| await act(async () => { | ||
| tab.click(); | ||
| await whenSettled(); |
There was a problem hiding this comment.
Avoid using the non-null assertion operator (!) on lookup results (such as Array.prototype.find) that may fail. Instead, allow the variable to be nullable and handle the undefined case downstream using optional chaining (?.).
const tab = tabs.find(t => t.textContent.includes('Instructions'));
expect(tab).withContext('Should find Instructions tab').toBeTruthy();
await act(async () => {
tab?.click();
await whenSettled();
}
References
- Avoid using the non-null assertion operator (
!) on lookup results (such asArray.prototype.find) that may fail. Instead, allow the variable to be nullable and handle theundefinedcase downstream using optional chaining (?.) or nullish coalescing (??).
| const ballroomChip = chips.find(el => el.textContent.trim() === 'Grand Ballroom'); | ||
| expect(ballroomChip).toBeTruthy(); | ||
|
|
||
| await act(async () => { | ||
| ballroomChip!.click(); | ||
| await whenSettled(); | ||
| }); |
There was a problem hiding this comment.
Avoid using the non-null assertion operator (!) on lookup results (such as Array.prototype.find) that may fail. Instead, allow the variable to be nullable and handle the undefined case downstream using optional chaining (?.).
const ballroomChip = chips.find(el => el.textContent.trim() === 'Grand Ballroom');
expect(ballroomChip).toBeTruthy();
await act(async () => {
ballroomChip?.click();
await whenSettled();
}
References
- Avoid using the non-null assertion operator (
!) on lookup results (such asArray.prototype.find) that may fail. Instead, allow the variable to be nullable and handle theundefinedcase downstream using optional chaining (?.) or nullish coalescing (??).
| const terraceChip = chips.find(el => el.textContent.trim() === 'Sunset Terrace'); | ||
| expect(terraceChip).toBeTruthy(); | ||
|
|
||
| await act(async () => { | ||
| terraceChip!.click(); | ||
| await whenSettled(); | ||
| }); |
There was a problem hiding this comment.
Avoid using the non-null assertion operator (!) on lookup results (such as Array.prototype.find) that may fail. Instead, allow the variable to be nullable and handle the undefined case downstream using optional chaining (?.).
const terraceChip = chips.find(el => el.textContent.trim() === 'Sunset Terrace');
expect(terraceChip).toBeTruthy();
await act(async () => {
terraceChip?.click();
await whenSettled();
}
References
- Avoid using the non-null assertion operator (
!) on lookup results (such asArray.prototype.find) that may fail. Instead, allow the variable to be nullable and handle theundefinedcase downstream using optional chaining (?.) or nullish coalescing (??).
Description
(Ignore the changes to the
reactadapter, those are being reviewed in #1624)This PR adds the remaining integration tests to the React adapter.
There are some minor changes to the react integration testing harness to improve the interaction with elements that require some more than a "click".
Pre-launch Checklist
If you need help, consider asking for advice on the discussion board.