Skip to content

fix(ui): add missing cy.testA11y() coverage to Cypress page objects and tests#2793

Open
manaswinidas wants to merge 7 commits into
kubeflow:mainfrom
manaswinidas:fix/cypress-a11y-coverage
Open

fix(ui): add missing cy.testA11y() coverage to Cypress page objects and tests#2793
manaswinidas wants to merge 7 commits into
kubeflow:mainfrom
manaswinidas:fix/cypress-a11y-coverage

Conversation

@manaswinidas

@manaswinidas manaswinidas commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Description

Add accessibility tests in all Cypress tests. This is just test change - not frontend changes.

How Has This Been Tested?

All existing tests should pass

Merge criteria:

  • All the commits have been signed-off (To pass the DCO check)
  • The commits have meaningful messages
  • Automated tests are provided as part of the PR for major new functionalities; testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work.
  • Code changes follow the kubeflow contribution guidelines.
  • For first time contributors: Please reach out to the Reviewers to ensure all tests are being run, ensuring the label ok-to-test has been added to the PR.

If you have UI changes

  • The developer has added tests or explained why testing cannot be added.
  • Included any necessary screenshots or gifs if it was a UI change.
  • Verify that UI/UX changes conform the UX guidelines for Kubeflow.

@google-oss-prow google-oss-prow Bot requested review from chambridge and fege June 5, 2026 10:51
@google-oss-prow

google-oss-prow Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from manaswinidas. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@github-actions github-actions Bot added Area/UI and removed size/M labels Jun 5, 2026
@manaswinidas

manaswinidas commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

/retest

ppadti
ppadti suggested changes Jun 8, 2026
View reviewed changes

@ppadti ppadti left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there any reason for adding cy.testA11y() / appChrome.waitForA11y() to only some tests and not all?
These tests still navigates without a11y check:(few tests have and not all)
modelVersionsCard.cy.ts
modelDetailsCard.cy.ts
mcpServerDetails.cy.ts
modelRegistry.cy.ts
modelCatalogDetails.cy.ts
modelCatalogTabs.cy.ts
modelCatalogCard.cy.ts

Comment thread ...ui/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionsCard.cy.ts
}),
);
cy.visit('/model-registry/modelregistry-sample/registered-models/1/overview');
appChrome.waitForA11y();

@ppadti ppadti Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are we just adding appChrome.waitForA11y(); to just this test and the rest uses cy.visit() without a11y

Comment thread .../ui/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelDetailsCard.cy.ts

it('displays model details correctly', () => {
cy.visit('/model-registry/modelregistry-sample/registered-models/1/overview');
appChrome.waitForA11y();

@ppadti ppadti Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same here as https://github.com/kubeflow/hub/pull/2793/changes#r3372059170

manaswinidas and others added 6 commits June 12, 2026 13:27
@manaswinidas
fix(ui): add missing cy.testA11y() coverage to Cypress page objects a…
b0788b9 
…nd tests

Signed-off-by: manaswinidas <dasmanaswini10@gmail.com>
@manaswinidas
Fix accessibility violations
2867ad1 
Signed-off-by: manaswinidas <dasmanaswini10@gmail.com>
@manaswinidas
Fix accessibility violations
04cdc6d 
Signed-off-by: manaswinidas <dasmanaswini10@gmail.com>
@manaswinidas @cursoragent
fix(ui): use subtle text color instead of disabled for empty artifact…
ff7f760 
… state

Assisted-by: Claude
Signed-off-by: manaswinidas <dasmanaswini10@gmail.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@manaswinidas @cursoragent
fix(ui): skip a11y on archive version list pending CI investigation
ada1fc5 
CI consistently reports a color-contrast violation (#dca614 on h4)
on this page that does not reproduce locally. Skipping testA11y
on visitArchiveModelVersionList until the CI-specific rendering
issue is identified.

Assisted-by: Claude
Signed-off-by: manaswinidas <dasmanaswini10@gmail.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@manaswinidas
Add a11y tests for remaining files and use appChrome
e999b11 
Signed-off-by: manaswinidas <dasmanaswini10@gmail.com>
@manaswinidas manaswinidas force-pushed the fix/cypress-a11y-coverage branch from 56f107a to e999b11 Compare June 12, 2026 07:59
@google-oss-prow google-oss-prow Bot added size/L and removed size/M labels Jun 12, 2026
@manaswinidas
Fix a11y violations
0fa8091 
Signed-off-by: manaswinidas <dasmanaswini10@gmail.com>
manaswinidas
manaswinidas commented Jun 12, 2026
View reviewed changes
Comment thread ...i/frontend/src/__tests__/cypress/cypress/tests/mocked/modelCatalog/modelCatalogDetails.cy.ts
modelCatalog.findLoadingState().should('not.exist');
modelCatalog.findModelCatalogDetailLink().first().click();
// Skip appChrome.waitForA11y() - blocked by focus trap a11y violation
// on the disabled Register model button (Popover → Tooltip fix in #2817).

@manaswinidas manaswinidas Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we should enable this in #2817 else here if that goes in first to double check

@manaswinidas

manaswinidas commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

/retest

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

Reviewers

@chambridge chambridge Awaiting requested review from chambridge

@fege fege Awaiting requested review from fege

1 more reviewer

@ppadti ppadti ppadti requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ppadti ppadti

Labels

Area/UI size/L

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@manaswinidas @ppadti