Repository navigation
Jetpack: run the component tests in CI - #52056
Conversation
`jest.config.gui.js` matched `_inc/client/**/test/component.js`. Every component test under `_inc/client/components/` is named `component.jsx`, and `jest.config.client.js` excludes `components/` from its roots, so 20 files ran in neither suite. PR #52015 broke seven of them and CI stayed green for six commits. Matching `.jsx` as well takes the GUI suite from 19 files and 203 tests to 54 and 487. The list stays curated rather than becoming a bare glob, for the reason the comment above it already gives. Two tests in `jetpack-connection-errors.test.jsx` failed once visible, both since before #52015. It rendered through `@testing-library/react` directly, so there was no store for the `reconnect` action's connected `NoticeActionReconnect`; it now uses the shared helper and supplies the state that selector reads. And an assertion that no reconnect CTA is present used a substring match, which the notice's own message satisfies — it now names the action. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NfGbCrDbhYmgwY5hW68VLA
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
github-actions
Bot
added
[Plugin] Jetpack
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 138 files. Only the first 5 are listed here.
|
dhasilva
left a comment
dhasilva
left a comment
There was a problem hiding this comment.
Review depth: standard (20 lines, 1 project) — auto-detect would have picked quick; raised on request so the config change got read rather than skimmed.
PR Review: #52056 — Jetpack: run the component tests in CI
Summary
Widens jest.config.gui.js's testMatch so the ~35 component.jsx files under _inc/client/ — which matched neither the GUI suite (component.js only) nor jest.config.client.js (whose roots exclude components/) — actually run, and fixes the two pre-existing failures that become visible as a result. The diagnosis and the numbers hold up.
I reproduced the coverage claim against the head config with micromatch: the resulting testMatch selects 54 of the 81 files under _inc/client/**/test/. The 27 it leaves out are exactly the intended ones — 8 fixtures.js files, the 18 _inc/client/state/ suites that jest.config.client.js owns, and _inc/client/test/test-utils.jsx (a helper, not a suite). That matches the description's "54 files" exactly, with no accidental double-running.
I also confirmed the fix reaches CI rather than only a local script: composer.json test-js → pnpm:test-adminpage → test-client && test-gui, so the GUI config is executed by the "JS tests" job, which passes on the head commit. No || true, no continue-on-error, no silently-skipped script.
Affected Projects
projects/plugins/jetpack(all three files; nothing under.github/)
PR Description
Good — accurate, specific testing instructions with before/after suite counts, a stated repro for the invisible-breakage claim, data/privacy answered, no private WordPress.com URLs.
Changelog
Present and correct. projects/plugins/jetpack/changelog/jetpack-2543-component-tests-in-ci uses Significance: patch / Type: other with an empty entry and a Comment: — the right shape for a test-config change with no user-facing effect, and other is a valid type for plugins/jetpack. No other project is touched, so no dependent-plugin entries are needed.
Bugs
-
[blocker] The replacement assertion can never fail — it is a no-op that looks like a check.
jetpack-connection-errors.test.jsx#L91-L94The old
queryByText( 'Reconnect', { exact: false } )could never pass, exactly as the description says. ButqueryByRole( 'link', { name: 'Restore Connection' } )can never fail, because the CTA it is looking for is not alink.NoticeActionReconnectrenders<NoticeAction icon onClick>with nohref, andNoticeActionemits a bare<a>. An<a>withouthrefmaps to rolegeneric, notlink(aria-queryhas{name: 'a'}→genericand onlya[href]→link).Rendering the real markup and querying it directly:
<a class="dops-notice__action"><span>Restore Connection</span></a> queryByRole( 'link', { name: 'Restore Connection' } ) -> null queryByRole( 'button' ) -> null queryByText( 'Restore Connection' ) -> <span>Restore Connection</span> getRoles( container ) -> [ 'generic' ]So both assertions on those two lines return
nulleven when the Restore Connection CTA is fully rendered. The test now has no witness for the thing it claims to be asserting the absence of: if a regression made the'none'action render the reconnect CTA, this test would still pass. Given that this PR exists to stop tests from silently failing to catch regressions, swapping a never-passing assertion for a never-failing one is worth fixing here rather than later.Suggested fix — go back to a text query, but exact (the default), which is what the accompanying comment is already describing:
expect( screen.queryByText( 'Restore Connection' ) ).not.toBeInTheDocument();
exact: trueis the default, and the notice's own message ("The connection owner needs to reconnect their account.") does not contain the literal stringRestore Connection, so this passes for the'none'case and fails if the CTA appears. I verified it resolves to a single node (the inner<span>) —getNodeTextonly reads direct child text nodes, so the wrapping<a>is not a competing match.Two smaller things that ride along with this:
- The comment on L92 ("Matched exactly: the notice's own message contains the word 'reconnect'.") explains a text-matching nuance the current code no longer uses — it is a role query. It becomes accurate again under the fix above, so it is worth keeping only if the assertion goes back to
queryByText. - Worth adding a positive control so this can't silently rot again: the
'should handle multiple errors correctly'test already renders areconnectaction, so anexpect( screen.getByText( 'Restore Connection' ) ).toBeInTheDocument();there proves the query used in the negative test can actually see the CTA.
- The comment on L92 ("Matched exactly: the notice's own message contains the word 'reconnect'.") explains a text-matching nuance the current code no longer uses — it is a role query. It becomes accurate again under the fix above, so it is worth keeping only if the assertion goes back to
Convention Issues
-
[suggestion] The new
*.test.jsxpattern is scoped tocomponents/, which leaves the same gap open everywhere else.'<rootDir>/_inc/client/components/**/test/*.test.jsx'only rescues the two.test.jsxfiles that happen to live undercomponents/. Afoo.test.jsx(or.test.js) added under_inc/client/traffic/,at-a-glance/,recommendations/… still runs in neither suite — the precise failure mode this PR is fixing, just relocated.Broadening is safe today, and I checked rather than assumed: there are no
*.test.jsor*.test.jsxfiles anywhere under_inc/client/outsidecomponents/jetpack-notices/, and none under_inc/client/state/, so nothing new is picked up and nothing double-runs withjest.config.client.js. It also doesn't violate the rationale in the comment above the list, since the excluded files are namedfixtures.js, not*.test.js.'<rootDir>/_inc/client/**/test/*.test.{js,jsx}',
-
[suggestion] Seven entries in the list are now dead — fully shadowed by the two new globs.
Jest dedupes, so there is no double-run — but they are now config that says nothing, and a curated list is only worth its curation if every line still earns its place. These are each matched by
_inc/client/**/test/component.jsxor_inc/client/components/**/test/*.test.jsx:- L15
ai/features/test/component.jsx - L18
ai/overview/assistant-banner/test/component.jsx - L19
ai/overview/test/component.jsx - L24
at-a-glance/boost/test/component.jsx - L25
components/jetpack-notices/test/state-notices.test.jsx - L26
sharing/test/component.jsx - L27
traffic/test/component.jsx
Dropping them takes the hand-maintained remainder down to the eight genuinely irregular filenames (
ai-admin.jsx,allowlist-updated.jsx,use-mcp-settings.jsx,index.jsx,use-scheduled-tasks.js,main.jsx,tracks.js,chart-bar-range.js), which makes the list's actual job legible. - L15
Test Quality
The test/test-utils swap is right and matches how the other 20 GUI suites already render. The initialState shape is correct — isReconnectingSite reads state.jetpack.connection.requests.reconnectingSite, which is exactly what the fixture supplies. Both added comment blocks are two lines and within the AGENTS.md budget; the budget script reports nothing on this diff.
Other Checks
Security, performance, backward compatibility, cross-package version skew, Phan suppressions, translations, a11y/RTL, dependencies, feature gating, data/privacy: not applicable — no PHP, no CSS, no user-facing strings, no dependency or public-API changes.
Test Results
- JS: not run locally (shared checkout). CI's "JS tests" job passes on
ba15eb2, which independently confirms the 54-suite / 487-test claim. - PHP: n/a — no PHP changed.
- Phan: n/a — no PHP changed.
Verdict
Needs changes before merge. The config change is correct, well-scoped, and verifiably does what it says — I'd happily take it as-is on that front. The blocker is small and one line: the assertion introduced at L94 cannot fail, which is the same defect class the PR is here to eliminate.
Reviewed 3 files, 20 lines changed. Checked: changelog, PR description, conventions, bugs, test quality, comment budget, CI wiring, glob coverage.
Generated by Claude.
The replacement assertion could not fail either. `NoticeAction` renders an
`<a>` with no `href` when given `onClick`, and an anchor without `href` has no
implicit link role, so `queryByRole( 'link' )` returns null whether or not the
reconnect CTA is present. The neighbouring `queryByRole( 'button' )` was vacuous
for the same shape: every variant of this notice passes `showDismiss={ false }`,
so none of them renders a button. Match the CTA by its label instead, and give
the test the store state the connected `NoticeActionReconnect` needs, so a
regression renders rather than crashes.
Broaden the `.test.{js,jsx}` pattern from `components/` to all of `_inc/client`,
so a file added under `traffic/` or `at-a-glance/` cannot land in neither suite
the same way. `_inc/client` holds exactly two such files today, both already
selected; the 20 others live under `jest.config.client.js`'s roots, outside this
config's `_inc/client/**` anchor.
Drop the seven entries the name globs already match. `jest --listTests` selects
the same 54 files before and after.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NfGbCrDbhYmgwY5hW68VLA
…ing it
The original swap turned `display={ false }` from "render, then hide with
`is-hidden`" into an early `return null`. That unmounts the subtree, and
`JetpackNotices` passes `display={ ! isReconnectingSite }` down to the connection
errors — whose `NoticeActionReconnect` records
`jetpack_termination_error_notice_view` from a mount effect. A failed reconnect
therefore unmounted and remounted the notice and recorded the view twice, on top
of discarding `useRestoreConnection`'s own state mid-flight.
The class comes back, with the rule unlayered because `Notice.Root` sets
`display: grid` inside `@layer wp-ui`, which `[hidden]` cannot override.
Also clears the props that survived the migration without a job. Nothing passes
`icon` to `SimpleNotice` any more, and `isValidElement` silently dropped a string
anyway; `NoticeAction`'s `icon` and `variant` never reached the rendered action.
A stale value should fail in review rather than do nothing quietly, so the
declarations and their last two call sites go together. Whether a secondary
action needs its own treatment is still open, and is recorded on the PR.
`JetpackConnectionErrors` returns null when nothing renders, which is the empty
flex child that `JetpackStateNotices` and `DismissableNotices` were already
fixed for, and `Notice.Description` renders a div, since several call sites hand
it block content and it defaults to a span.
The wrapper now has tests. It had none, which is why the `display` regression
survived nine commits: both new display cases fail against the old behaviour.
They run once the component suites reach CI in #52056.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NfGbCrDbhYmgwY5hW68VLA
| '<rootDir>/_inc/client/**/test/component.js', | ||
| '<rootDir>/_inc/client/**/test/component.jsx', |
There was a problem hiding this comment.
Should we do like
| '<rootDir>/_inc/client/**/test/component.js', | |
| '<rootDir>/_inc/client/**/test/component.jsx', | |
| '<rootDir>/_inc/client/**/test/component.{js,jsx}', |
as with the one below?
Matches the `*.test.{js,jsx}` glob on the line below. Four `component.js` files
remain under `_inc/client`, so both extensions are still needed.
`jest --listTests` selects the same 54 files before and after.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NfGbCrDbhYmgwY5hW68VLA
The 'none' case asserts the reconnect CTA is absent. Nothing proved that query could see the CTA when it is present, so the assertion could rot back into a no-op without anyone noticing. The multiple-errors test already renders a `reconnect` action, so it can carry the control. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NfGbCrDbhYmgwY5hW68VLA
) * Jetpack notices: back SimpleNotice with the @wordpress/ui Notice SimpleNotice and NoticeAction now wrap `Notice.Root` and its action subcomponents, so all 14 files that render a Jetpack admin notice move to the design system at once without touching their call sites. SimpleNotice keeps its two-slot contract: with `text` set, children are the actions and go into `Notice.Actions`; without it, children are the body. `NoticeAction` maps `href` to `Notice.ActionLink` with `external` becoming `openInNewTab`, which also draws the external-link arrow the Gridicon used to supply, and falls back to `Notice.ActionButton` when there is no href. The `dops-notice` classes are gone from the component, so the old rules cannot fight the design system's styles. Two consequences handled here: `AdminNotices` still rebuilds server-rendered core notices into that chassis with jQuery, so its stylesheet is now loaded from `scss/style.scss`; and the floating notice stack targets its children rather than a notice class, because `Notice.Root`'s CSS-module class name is hashed. Known gaps, none of which block the swap: `isCompact` has no design system equivalent, a Gridicon name passed as `icon` is dropped in favour of the intent's own icon, and `NoticeAction`'s `variant` and `icon` props are no longer honoured at their single call sites each. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017AnRQ7ZE68VGgGVQB4pFND * Jetpack notices: restore the spacing, underline and padding the class carried Wrapping SimpleNotice over `@wordpress/ui` dropped the `dops-notice` class, and three behaviours went with it, none of them visible in that diff. `SimpleNotice` now always contributes a `jp-notice` class alongside whatever the caller passes, because `Notice.Root`'s own class name is a CSS-module hash and page styles need something stable to target. The stacked notices lost their 24px gap, which `.dops-notice` supplied as `margin-bottom`. Both containers that stack notices use `Stack` instead: `JetpackNotices`, which was a bare `<div aria-live="polite">`, and `JetpackConnectionErrors`, which returned a bare array. `gap="xl"` is the same 24px, applied as an inline style, so nothing on the page can override it. Links inside notices lost their underline to `.jetpack-pagestyles a`, which is unlayered and so beats every `@layer wp-ui` rule regardless of specificity; `Link` sets no `text-decoration` of its own and relies on the UA underline. A scoped counter-rule restores it, to be removed once the `@wordpress/ui` global CSS defense grows a `text-decoration` bridge. Inside a settings card the notices were laid out for the old chassis, which was a dark full-bleed banner with square corners. A rounded, bordered design system notice cannot sit flush against the card edge, so both here get the padding the rest of the card uses. Account protection's notices also move to real JSX children rather than a `children` prop, which any actual children would beat. The Like buttons notice moves out of its `SettingsGroup`. That group paints an 80% white scrim over its children in offline mode, which left the notice below readable contrast and its link under an overlay that takes pointer events — on a notice whose whole purpose is to explain why the controls are unavailable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NfGbCrDbhYmgwY5hW68VLA * Jetpack notices: give SimpleNotice a title slot and use it where one exists `@wordpress/ui` `Notice` renders a title as `heading-md` and a description as `body-md`. SimpleNotice only ever filled the description, so callers that wanted a heading built one by hand. A `title` prop feeds `Notice.Title`. Four call sites already had a title in their markup and now pass it instead: the deprecation notice drops an inline `fontWeight: 600` div and hands over the `title` prop it already accepted, the static warning splits its two server-substituted placeholders, and both SEO banners drop a `<strong>`. This is extraction only — no copy changes and no new translated strings. The remaining notices either carry a single sentence, where a title would have to be written, or take their text from the server, where it would have to be derived from an error code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NfGbCrDbhYmgwY5hW68VLA * Jetpack notices: return null from the notice components that render nothing `JetpackStateNotices` and `DismissableNotices` each wrapped their output in a `<div>`, so both left an empty element in the notice stack whenever they had nothing to show. `DismissableNotices` always does: its `renderNotices` has only a `default: return false`. That was free while each notice carried its own `margin-bottom`, because an empty div has no margin. The stack is now a flex `Stack`, and `gap` applies between every child, empty or not — so a lone notice sat between two 24px bands. Both return null instead. The other siblings already returned false, which puts no node in the DOM. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NfGbCrDbhYmgwY5hW68VLA * Jetpack notices: stop the toasts collapsing on desktop `Notice.Root` sets `container-type: inline-size` for its own container query, which means it is sized as if it had no contents. Above 660px the toast stack is a fixed container with `left: auto` and no width, so it shrinks to fit — and a size-contained child contributes nothing. The container collapsed and the icon and text spilled outside the tinted box. The legacy notice was a plain flex element, so it fed that calculation. Turning containment off for the toasts restores it, with no width invented. The query this disables only applies to a notice with a title, which a toast never has. The `text-align` pair goes with it: that was how the legacy notice aligned itself inside a full-width container, and the container now anchors itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NfGbCrDbhYmgwY5hW68VLA * Jetpack notices: draw the toast shadow at the design system's elevation The toast carried a Calypso-era shadow: two layers at alpha 0.2 and 0.15, the second a 56px blur at zero offset. Under a light tinted card that halo reads as a smudge rather than as elevation. `@wordpress/ui` has no shadow token. Its components define a private `--_wp-ui-elevation-*` locally instead, and a floating transient overlay belongs at the same level as a popover or a menu, so this copies that stack. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NfGbCrDbhYmgwY5hW68VLA * Jetpack notices: stop notices announcing themselves, and pad the two remaining flush ones Review found four things the migration missed. `Notice.Root` defaults `spokenMessage` to its children, so every notice now called `speak()` on mount — including permanent ones like the SEO banners and the block-theme notice, which announced on page load, and the ones inside the `aria-live` stack, which announced twice. The legacy notice never announced, and the notices that should are already inside a live region, so this passes `spokenMessage={ null }`. Two more notices are direct `SettingsCard` children and so ran to the card's edges: the SEO opt-in banner and the reader's WordPress.com notice. Both get the same padded wrapper as account protection. The Sharing buttons block-theme notice moves out of its `SettingsGroup` for the same reason the Like buttons one did: the group paints an opaque scrim over its children in offline mode, which takes pointer events. Three test assertions were reading the old chassis: two on the `dops-notice` class, one on a link's accessible name, which now carries the design system's "(opens in a new tab)" suffix. None of these suites run in CI — `jest.config.gui.js` matches `test/component.js`, not `.jsx`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NfGbCrDbhYmgwY5hW68VLA * Jetpack notices: lift the conflicting-SEO-plugin notice out of the scrim too A sweep for the same shape found one more: the "Your SEO settings are managed by the following plugin" notice sits inside a `SettingsGroup disableInOfflineMode`, so an offline site with Yoast or Rank Math installed renders it under the scrim. Same treatment as the Like buttons and Sharing buttons notices — outside the group, in the card's padding. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NfGbCrDbhYmgwY5hW68VLA * Jetpack notices: don't render the SEO banner's padded wrapper when there is no banner `seoOptInBanner()` returns null unless the SEO package reports the opt-in is available, which is most sites. The padded wrapper around it was unguarded, so those sites got 32px of empty space above the card's description. The call is hoisted so the guard does not run it twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NfGbCrDbhYmgwY5hW68VLA * Jetpack notices: drop the broken-link icon from the connection errors Four connection-error notices passed `icon="link-break"`. `Notice.Root` takes an icon element rather than a Gridicon name, so the design system's error icon has been rendering there since the migration and the prop did nothing. Standardising on the intent's icon is the decision, so the props go rather than being converted to elements. `SimpleNotice` still honours an icon passed as an element, which the SocialLogo in `jetpack-notices/index.jsx` relies on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NfGbCrDbhYmgwY5hW68VLA * Jetpack notices: keep `display` hiding the notice rather than unmounting it The original swap turned `display={ false }` from "render, then hide with `is-hidden`" into an early `return null`. That unmounts the subtree, and `JetpackNotices` passes `display={ ! isReconnectingSite }` down to the connection errors — whose `NoticeActionReconnect` records `jetpack_termination_error_notice_view` from a mount effect. A failed reconnect therefore unmounted and remounted the notice and recorded the view twice, on top of discarding `useRestoreConnection`'s own state mid-flight. The class comes back, with the rule unlayered because `Notice.Root` sets `display: grid` inside `@layer wp-ui`, which `[hidden]` cannot override. Also clears the props that survived the migration without a job. Nothing passes `icon` to `SimpleNotice` any more, and `isValidElement` silently dropped a string anyway; `NoticeAction`'s `icon` and `variant` never reached the rendered action. A stale value should fail in review rather than do nothing quietly, so the declarations and their last two call sites go together. Whether a secondary action needs its own treatment is still open, and is recorded on the PR. `JetpackConnectionErrors` returns null when nothing renders, which is the empty flex child that `JetpackStateNotices` and `DismissableNotices` were already fixed for, and `Notice.Description` renders a div, since several call sites hand it block content and it defaults to a span. The wrapper now has tests. It had none, which is why the `display` regression survived nine commits: both new display cases fail against the old behaviour. They run once the component suites reach CI in #52056. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NfGbCrDbhYmgwY5hW68VLA * Jetpack notices: correct what happens to the underline rule upstream The comment said to drop this rule once the design system grows a `text-decoration` bridge. That would regress three notices: the upstream defense only reaches elements carrying wp-ui's `Link` class, and the offline-mode notice and two state notices interpolate a raw `<a>` that nothing else underlines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NfGbCrDbhYmgwY5hW68VLA * Jetpack notices: trim the review nits before merge Four small items left from review, none behavioural: - The changelog entry capitalises after the component prefix, per AGENTS.md. - Two comment blocks come back inside the budget. The `.global-notices > *` block carried three unrelated rationales in one six-line essay; each trap now sits on the declaration it explains. The underline rule's seven-line note loses the upstream detail that duplicates the linked issue. - `dismissText` is read in `render()`, so it is declared in `propTypes`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SWVs5oUDimen1FeGJhJAkx * Jetpack notices: say why the toast elevation is copied `--_wp-ui-elevation-md` is not a token: `@wordpress/ui` redeclares it on each component's own element (popover's `.surface`, item-popup's `.popup`), inside `@layer wp-ui`, and never publishes it. Referencing it here would resolve to nothing and drop the shadow silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SWVs5oUDimen1FeGJhJAkx --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 participants
Fixes JETPACK-2543
Proposed changes
projects/plugins/jetpack/tests/jest.config.gui.jsmatched_inc/client/**/test/component.js. Every component test under_inc/client/components/is namedcomponent.jsx, andjest.config.client.jsexcludescomponents/from itsroots, so 20 files ran in neither suite.PR #52015 broke seven of them and CI stayed green through six commits. They were only found by running the files by hand.
**/test/component.jsis broadened to**/test/component.{js,jsx}— fourcomponent.jsfiles still exist, so both extensions are needed — and**/test/*.test.{js,jsx}is added for the two files following the other naming convention. Together they take the GUI suite from 19 files and 203 tests to 54 files and 487 tests. Both are anchored at_inc/client/**, so a test added undertraffic/orat-a-glance/cannot fall in the same gap. Seven explicit entries the new patterns already match are removed;jest --listTestsselects the same 54 files with and without them.When this broke
#50418 ("Clean up JSX-in-
.js", merged 2026-07-10) renamed 768 files from.jsto.jsxand did not update thistestMatch. These suites have been dead since that date.No other project is affected. Most use
tools/js-tools/jest/config.base.js, which sets notestMatchat all, so jest's defaults apply and those cover.jsx— which is why, for example,js-packages/api/test/rest-api.jsxstill runs. Four configs do use.js-only patterns (js-packages/config,packages/menu-badges,js-packages/critical-css-gen,wpcomsh/tests/e2e); each was checked for stranded.jsxtests and none have any.js-packages/configdocuments why it deviates.The list still matches by filename rather than by a bare
**/test/*, for the reason the comment above it gives: the bare pattern also picks up the manytest/fixtures.jsfiles, which contain no tests, and re-runs the_inc/client/state/suites thatjest.config.client.jsalready owns.Two pre-existing failures
Both fail identically at
a67f5be8ec6~1, so neither comes from #52015. They only had to be fixed here because the config change makes them visible._inc/client/components/jetpack-notices/test/jetpack-connection-errors.test.jsxshould handle multiple errors correctlyrendered through@testing-library/reactdirectly, so there was no<Provider>. One of its errors carriesaction: 'reconnect', which reaches the connectedNoticeActionReconnectand threwCould not find "store" in the context of…. It now uses the sharedtest/test-utilshelper and supplies the sliceisReconnectingSitereads.should render an informational notice with no action for the 'none' actionassertedqueryByText( 'Reconnect', { exact: false } )was absent.exact: falseis a case-insensitive substring match, and the notice's own message is "The connection owner needs to reconnect their account.", so the assertion could never pass. It now matches the CTA by its label,Restore Connection. Matching it by role would be no better than the substring:NoticeActionrenders an<a>with nohrefwhen given anonClick, and an anchor withouthrefhas no implicit link role. The test also supplies the store state now, so making the'none'case render the reconnect CTA fails the assertion instead of crashing the render.The
'none'assertion now has a positive control:should handle multiple errors correctlyalready renders areconnectaction, so it asserts the CTA is visible. That proves the query used in the negative test can see the thing whose absence it asserts, so it cannot quietly become a no-op again.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
projects/plugins/jetpack, runpnpm run test-gui.pnpm run test-clientis unchanged at 47 suites and 442 tests — this touches only the GUI config.To confirm the
'none'assertion guards something, edit thecase 'none':arm of_inc/client/components/jetpack-notices/jetpack-connection-errors.jsxto returnErrorNoticeCycleConnectionwithaction={ 'reconnect' }, as thedefaultarm does.pnpm run test-guithen fails withexpected document not to contain element, found <span>Restore Connection</span> instead.🤖 Generated with Claude Code
https://claude.ai/code/session_01NfGbCrDbhYmgwY5hW68VLA