chore: skip flaky VRTs #5823

rubencarvalho · 2025-10-22T20:07:12Z

Description

Skipped three consistently flaky visual regression tests (VRTs) by adding swc_vrt: { skip: true } configuration:

Action Menu: selects story
Picker: iconsNone story
Picker: iconsOnly story

Added unit test coverage for the Picker icons attribute functionality (to cover what VRTs skipped above)

Tests icons="none" hides icon in button while preserving it in menu items
Tests icons="only" hides label text when value is selected
Tests icons="only" shows label text when no value is selected

Motivation and context

I looked into CircleCI insights and cross-checked with our notes to confirm these 3 VRT tests have been consistently failing with flaky behavior.

I confirmed that for the Action Menu selects, it already was covered by existing unit tests in in packages/action-menu/test/index.ts (lines 330-416 and 734-843) - covering change events, selection behavior, and submenu interactions.

For the Picker iconsNone and iconsOnly, there previously was NO unit test coverage. I added new unit tests in packages/picker/test/index.ts that verify all functional behavior of the icons attribute as per our documentation.

With the added tests in place, I feel better about skipping these flaky VRTs.

Author's checklist

I have read the CONTRIBUTING and PULL_REQUESTS documents.
I have reviewed at the Accessibility Practices for this feature, see: Aria Practices
I have added automated tests to cover my changes.
I have included a well-written changeset if my change needs to be published.
I have included updated documentation if my change required it.

Reviewer's checklist

Includes a Github Issue with appropriate flag or Jira ticket number without a link
Includes thoughtfully written changeset if changes suggested include patch, minor, or major features
Automated tests cover all use cases and follow best practices for writing
Validated on all supported browsers
All VRTs are approved before the author can update Golden Hash

changeset-bot · 2025-10-22T20:07:16Z

⚠️ No Changeset found

Latest commit: b51da07

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

github-actions · 2025-10-22T20:09:09Z

📚 Branch Preview

🔍 Visual Regression Test Results

When a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:

Deployed to Azure Blob Storage: pr-5823

If the changes are expected, update the current_golden_images_cache hash in the circleci config to accept the new images. Instructions are included in that file.
If the changes are unexpected, you can investigate the cause of the differences and update the code accordingly.

github-actions · 2025-10-22T20:12:19Z

Tachometer results

Currently, no packages are changed by this PR...

coveralls · 2025-10-22T20:17:30Z

Pull Request Test Coverage Report for Build 18789721554

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 97.966%

Totals
Change from base Build 18782705146:	0.0%
Covered Lines:	34249
Relevant Lines:	34778

💛 - Coveralls

caseyisonit

LGTM - just needs the golden hash updated before merge

packages/picker/test/index.ts

Rajdeepc

Most of the tests are logically correct. Thanks for doing this.
A few suggestions and feedback and if you can add the below check that would be great:

"updates icon visibility" test can you add non-null checks for iconSpan/labelSpan before reading .hidden or .classList to avoid confusing runtime errors and make failures easier to diagnose.

Rajdeepc · 2025-10-23T09:09:21Z

packages/picker/test/index.ts

+                '#icon'
+            ) as HTMLElement;
+            expect(iconSpan).to.not.be.null;
+            expect(iconSpan.hidden, 'icon span should be hidden').to.be.true;


This assertion is not required since you are already checking iconSpan for non-null

In the #icon span in the Picker, there is this:

<span id="icon" ?hidden=${this.icons === 'none'}> ${this.selectedItemContent.icon} </span>

There are 2 things I am asserting here: 1) that the icon span exists in the DOM (sanity check, if you will) and 2) that it is set to "hidden" because of the property icons="none" being set. Without the second assertion, we wouldn't catch if the ?hidden binding was broken.
LMK what you think!

Rajdeepc · 2025-10-23T09:09:35Z

packages/picker/test/index.ts

+                '.label'
+            ) as HTMLElement;
+            expect(labelSpan).to.not.be.null;
+            expect(


This assertion is not required since you are already checking labelSpan for non-null

In this case, we're checking if icons="none" (not "only"), the label does not get the "visually-hidden" class applied (https://github.com/adobe/spectrum-web-components/blob/ruben/skip-flaky-vrts/packages/picker/src/Picker.ts#L471-L472).
Maybe this is testing more of the implementation details as I would like, but that's the most-straightforward way I found to test the elements' visibility to cover what the VRTs were checking. What do you thinkg?

Rajdeepc · 2025-10-23T09:10:59Z

packages/picker/test/index.ts

+            const menuItems = el.querySelectorAll('sp-menu-item');
+            menuItems.forEach((item) => {
+                const icon = item.querySelector('[slot="icon"]');
+                expect(icon, `menu item ${item.value} should have icon`).to.not


Can you add an assertion that you actually found menu items, e.g. expect(menuItems.length).to.be.greaterThan(0) — otherwise the loop would be vacuously passing if no items found.

Just a suggestion - How do you feel doing a check if the icon element is the expected tag sp-icon-edit or sp-icon-copy to make the test more tighter?

Rajdeepc · 2025-10-23T09:14:13Z

packages/picker/test/index.ts

+                '.label'
+            ) as HTMLElement;
+            expect(labelSpan).to.not.be.null;
+            expect(


Do you think this is required? You are already checking for its existence in the above assertion

Rajdeepc · 2025-10-23T09:14:28Z

packages/picker/test/index.ts

+                '#icon'
+            ) as HTMLElement;
+            expect(iconSpan).to.not.be.null;
+            expect(iconSpan.hidden, 'icon should be visible').to.be.false;


Give this a thought too if you need it

I think we want it. Ultimately we're testing the icons functionality which specifically includes checking for the icon's visibility state depending on the icons prop (https://opensource.adobe.com/spectrum-web-components/components/picker/#icons).
An element can exist in the DOM but still be hidden (via the hidden attribute). The component's behavior is:
icons="none" - icon span exists but is hidden
icons="only" - icon span exists and is visible
default - icon span exists and is visible

LMK what you think

Rajdeepc · 2025-10-23T09:14:49Z

packages/picker/test/index.ts

+                '.label'
+            ) as HTMLElement;
+            expect(labelSpan).to.not.be.null;
+            expect(


Same like above

I replied above, LMK what you think!

packages/picker/test/index.ts

graynorton

LGTM!

Dismissing review to unblock merging

rubencarvalho added 2 commits October 22, 2025 21:43

chore: skip flaky VRTs

1f8c201

chore: add tests to cover skipping VRTs

f09ca0b

rubencarvalho added Component: Tooling Issue or PR dealing with scripts, workflows, automation, etc. Component prefix is for Jira Status: Ready for review PR ready for review or re-review. labels Oct 22, 2025

rubencarvalho marked this pull request as ready for review October 22, 2025 20:16

rubencarvalho requested a review from a team as a code owner October 22, 2025 20:16

caseyisonit approved these changes Oct 22, 2025

View reviewed changes

Merge branch 'main' into ruben/skip-flaky-vrts

8289535

Rajdeepc reviewed Oct 23, 2025

View reviewed changes

packages/picker/test/index.ts Outdated Show resolved Hide resolved

Rajdeepc previously requested changes Oct 23, 2025

View reviewed changes

chore: address PR feedback

3473063

rubencarvalho requested a review from Rajdeepc October 24, 2025 14:11

Merge branch 'main' into ruben/skip-flaky-vrts

9476440

graynorton approved these changes Oct 24, 2025

View reviewed changes

caseyisonit added the High priority PR review PR is a high priority and should be reviewed ASAP label Oct 24, 2025

chore: update current_golden_images_hash

b51da07

caseyisonit added Status: Ready for merge PR has 2 approvals, all tests pass, and is ready to merge and removed Status: Ready for review PR ready for review or re-review. labels Oct 24, 2025

rubencarvalho enabled auto-merge (squash) October 24, 2025 19:23

rubencarvalho merged commit 1bf524a into main Oct 24, 2025
29 checks passed

rubencarvalho deleted the ruben/skip-flaky-vrts branch October 24, 2025 19:24

chore: skip flaky VRTs #5823

chore: skip flaky VRTs #5823

Uh oh!

Conversation

rubencarvalho commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and context

Author's checklist

Reviewer's checklist

Uh oh!

changeset-bot bot commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

github-actions bot commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📚 Branch Preview

🔍 Visual Regression Test Results

Uh oh!

github-actions bot commented Oct 22, 2025

Tachometer results

Uh oh!

coveralls commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 18789721554

Details

💛 - Coveralls

Uh oh!

caseyisonit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Rajdeepc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rajdeepc Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

graynorton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rubencarvalho commented Oct 22, 2025 •

edited

Loading

changeset-bot bot commented Oct 22, 2025 •

edited

Loading

github-actions bot commented Oct 22, 2025 •

edited

Loading

coveralls commented Oct 22, 2025 •

edited

Loading

Rajdeepc Oct 23, 2025 •

edited

Loading