Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 26, 2025

No unit tests should be using import {fireEvent, within} from 'storybook/test'; and every functional components tests should be using renderFunctionFixture. Some of our earlier tests did not follow the right structure which is confusing for the agents. With this PR, everything will be streamlined and agents will always use renderFunctionFixture.

Additionally, fixed duplicate test names in checkbox.spec.ts to ensure each test has a unique, descriptive name:

  • "should set the correct id attribute"
  • "should use text as aria-label and value when ariaLabel is not provided"
  • "should prioritize ariaLabel over text for aria-label attribute"

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI and others added 3 commits November 26, 2025 17:23
- Update button.spec.ts: Remove manual container, use renderFunctionFixture, add page locators
- Update radio-button.spec.ts: Remove manual container, use renderFunctionFixture
- Update pager-buttons.spec.ts: Remove manual container, use renderFunctionFixture
- Update pager-navigation.spec.ts: Remove manual container, use renderFunctionFixture
- Update item-list.spec.ts: Remove manual container, use renderFunctionFixture
- Update tab-wrapper.spec.ts: Remove manual container, use renderFunctionFixture

All tests now follow the correct pattern with async rendering and proper cleanup.

Co-authored-by: alexprudhomme <78121423+alexprudhomme@users.noreply.github.com>
- Update color-facet-checkbox.spec.ts: Replace fireEvent with native events
- Update smart-snippet-feedback-banner.spec.ts: Replace fireEvent/within with native queries

All functional component tests now use proper Vitest browser utilities.

Co-authored-by: alexprudhomme <78121423+alexprudhomme@users.noreply.github.com>
- Update checkbox.spec.ts: Remove manual container, use renderFunctionFixture, replace fireEvent
- Update heading.spec.ts: Remove manual container, use renderFunctionFixture, replace within

All functional component tests in common directory now use the correct pattern.

Co-authored-by: alexprudhomme <78121423+alexprudhomme@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix functional component tests to use renderFunctionFixture Migrate functional component tests to renderFunctionFixture Nov 26, 2025
Copilot AI requested a review from alexprudhomme November 26, 2025 17:34
Copilot finished work on behalf of alexprudhomme November 26, 2025 17:34
@alexprudhomme alexprudhomme changed the title Migrate functional component tests to renderFunctionFixture test(atomic): migrate functional component tests to use renderFunctionFixture Nov 27, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR successfully migrates functional component tests to use renderFunctionFixture and removes deprecated Storybook testing utilities, standardizing test patterns across the Atomic package.

Key Changes:

  • Replaced test() with it() throughout all test files for consistency with Vitest conventions
  • Migrated from manual DOM manipulation (render() + container) to renderFunctionFixture() for functional components
  • Removed fireEvent and within imports from storybook/test and replaced with native DOM APIs

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
bindings-mixin.spec.ts Replaced test() with it() for consistency
color-facet-checkbox.spec.ts Removed fireEvent import, replaced with native .click() and .dispatchEvent()
atomic-search-box.spec.ts Replaced test() with it() in placeholder tests
tab-wrapper.spec.ts Migrated to renderFunctionFixture, removed manual container management
smart-snippet-feedback-banner.spec.ts Removed fireEvent/within from storybook, replaced with native DOM query methods
radio-button.spec.ts Fully migrated to renderFunctionFixture with page locators and native event dispatching
pager-navigation.spec.ts Migrated to renderFunctionFixture, changed beforeAll to beforeEach for proper test isolation
pager-buttons.spec.ts Migrated to renderFunctionFixture, consolidated setup in beforeAll, removed manual cleanup
item-list.spec.ts Migrated to renderFunctionFixture, removed manual container management
heading.spec.ts Migrated to renderFunctionFixture, removed within usage for element queries
checkbox.spec.ts Fully migrated with page locators, removed all storybook test utilities
button.spec.ts Fully migrated with page locators and native event dispatching
atomic-commerce-pager.spec.ts Replaced test() with it() throughout component tests
store.spec.ts Replaced test() with it() for consistency

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Renamed three tests that had the same name "should render a checkbox with the correct text attributes":
- "should set the correct id attribute"
- "should use text as aria-label and value when ariaLabel is not provided"
- "should prioritize ariaLabel over text for aria-label attribute"

Co-authored-by: alexprudhomme <78121423+alexprudhomme@users.noreply.github.com>
Copilot finished work on behalf of alexprudhomme November 27, 2025 18:49
Copilot AI temporarily deployed to Prerelease (CDN) November 27, 2025 20:04 Inactive
test('should render the icon with the correct attributes', () => {
const icon = container.querySelector('atomic-icon');
it('should render the icon with the correct attributes', async () => {
const element = await renderFunctionFixture(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we have a shared renderComponent function instead of repeating renderFunctionFixture in every test?

import {renderFunctionFixture} from '@/vitest-utils/testing-helpers/fixture';
import {type CheckboxProps, renderCheckbox as checkbox} from './checkbox';

describe('renderCheckbox', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
describe('renderCheckbox', () => {
describe('#renderCheckbox', () => {

renderPagerPreviousButton,
} from './pager-buttons';

describe('pagerButtons', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
describe('pagerButtons', () => {
describe('#pagerButtons', () => {

document.body.removeChild(container);
});
const locators = {
get button() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit out of scope, but we're only using the button locator in one test in the spec. The rest of the time we do an element.querySelector('button')... seems anti-pattern-ish.

});
const explainWhyButton = within(container).queryByText(
i18n.t('smart-snippet-feedback-explain-why')
const explainWhyButton = Array.from(container.querySelectorAll('*')).find(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quite verbose (and seems a bit brittle too!)

How about we use page.getByText(i18n.t('smart-snippet-feedback-explain-why') instead? Seems like it would do achieve the same result and be much less verbose.

In cases where multiple elements can have the same text, we could distinguish by role:

page.getByRole('button', {name: i18n.t('...')});

This seems like a better pattern to me.

The same comment applies to similar instances where within was replaced in this spec and other specs of components that render in the light DOM.

expect(onChecked).toHaveBeenCalled();
});

it('should handle keyboard navigation', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of scope (cause this is not changed by the PR), but I believe this test is giving us a false sense of security.

We assert that the radio-1, radio-2, etc. elements exist in the doc, but these elements get rendered at the beginning of the test, so I think these assertions will always pass even if keyboard navigation doesn't work.

I tested this with Copilot: we added an otherwise identical test that did no keyboard navigation and it passes.

I went a bit further and attempted to get the correct pattern for testing keyboard nav for this component with Copilot.

The following works when keyboard nav is OK and breaks when it isn't:

it('should handle keyboard navigation', async () => {
  const props = {
    groupName: 'test-group',
    selectWhenFocused: false,
  };

  const element = await renderFunctionFixture(
    html`${renderRadioButton({props: {...props, text: 'radio-1'}})}
    ${renderRadioButton({props: {...props, text: 'radio-2'}})}
    ${renderRadioButton({props: {...props, text: 'radio-3'}})}
    ${renderRadioButton({props: {...props, text: 'radio-4'}})}`
  );

  const inputs = element.querySelectorAll(
    'input[type="radio"]'
  ) as NodeListOf<HTMLInputElement>;

  inputs[0].focus();
  expect(document.activeElement).toBe(inputs[0]);

  inputs[0].dispatchEvent(
    new KeyboardEvent('keydown', {key: 'ArrowRight', bubbles: true})
  );
  expect(document.activeElement).toBe(inputs[1]);

  inputs[1].dispatchEvent(
    new KeyboardEvent('keydown', {key: 'ArrowRight', bubbles: true})
  );
  expect(document.activeElement).toBe(inputs[2]);

  inputs[2].dispatchEvent(
    new KeyboardEvent('keydown', {key: 'ArrowRight', bubbles: true})
  );
  expect(document.activeElement).toBe(inputs[3]);

  inputs[3].dispatchEvent(
    new KeyboardEvent('keydown', {key: 'ArrowRight', bubbles: true})
  );
  expect(document.activeElement).toBe(inputs[0]);

  inputs[0].dispatchEvent(
    new KeyboardEvent('keydown', {key: 'ArrowLeft', bubbles: true})
  );
  expect(document.activeElement).toBe(inputs[3]);

  inputs[3].dispatchEvent(
    new KeyboardEvent('keydown', {key: 'Tab', shiftKey: true, bubbles: true})
  );
  expect(document.activeElement).toBe(inputs[2]);
});

@fbeaudoincoveo
Copy link
Contributor

There's no linked issue / Jira in the PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://vitest.dev/api/browser/interactivity this is how we should interact, constructing User Interaction Event programatically is often a smell as it strays from what the user do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants