Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix flaky JS tests (2/2). #10097

Open
1 task
techanvil opened this issue Jan 22, 2025 · 3 comments
Open
1 task

Fix flaky JS tests (2/2). #10097

techanvil opened this issue Jan 22, 2025 · 3 comments
Labels
P0 High priority Team M Issues for Squad 2 Type: Infrastructure Engineering infrastructure & tooling

Comments

@techanvil
Copy link
Collaborator

techanvil commented Jan 22, 2025

Feature Description

This is an issue to address the latest round of JS tests which are failing sporadically. The task has been split into two issues in order to make it easier to investigate and fix. See #10085.

The tests listed in this issue are all failing due an update to a component not being wrapped in act().

EnhancedMeasurementActivationBanner › should render the in progress step when enhanced measurement is being enabled after the user returns from the OAuth flow

Example: https://github.com/google/site-kit-wp/actions/runs/12874436082/job/35893852218?pr=9941#step:7:702

Details
FAIL assets/js/modules/analytics-4/components/dashboard/EnhancedMeasurementActivationBanner/index.test.js
  ● EnhancedMeasurementActivationBanner › should render the in progress step when enhanced measurement is being enabled after the user returns from the OAuth flow

    expect(jest.fn()).not.toHaveErrored(expected)

    Expected mock function not to be called but it was called with:
    ["Warning: An update to %s inside a test was not wrapped in act(...).·
    When testing, code that causes React state updates should be wrapped into act(...):·
    act(() => {
      /* fire events that update state */
    });
    /* assert on the output */·
    This ensures that you're testing the behavior the user would see in the browser. Learn more at https://fb.me/react-wrap-tests-with-act%s", "EnhancedMeasurementActivationBanner", "
        in EnhancedMeasurementActivationBanner (created by WhenAnalytics4Active(EnhancedMeasurementActivationBanner))
        in WhenAnalytics4Active(EnhancedMeasurementActivationBanner)
        in Router (created by Wrapper)
        in Wrapper"]

      at Object.assertExpectedCalls (node_modules/@wordpress/jest-preset-default/node_modules/@wordpress/jest-console/build/@wordpress/jest-console/src/index.js:36:4)

useEnableAudienceGroup › should automatically call onEnableGroups function when user returns from the OAuth screen

Example: https://github.com/google/site-kit-wp/actions/runs/12794669197/job/35670303767#step:7:765

Details
FAIL assets/js/modules/analytics-4/hooks/useEnableAudienceGroup.test.js
  ● useEnableAudienceGroup › should automatically call `onEnableGroups` function when user returns from the OAuth screen

    expect(jest.fn()).not.toHaveErrored(expected)

    Expected mock function not to be called but it was called with:
    ["Warning: An update to %s inside a test was not wrapped in act(...).·
    When testing, code that causes React state updates should be wrapped into act(...):·
    act(() => {
      /* fire events that update state */
    });
    /* assert on the output */·
    This ensures that you're testing the behavior the user would see in the browser. Learn more at https://reactjs.org/link/wrap-tests-with-act%s", "TestHook", "

      at fn (node_modules/@testing-library/react-hooks/lib/pure.js:39:23)
          at Suspense
      at construct (node_modules/react-router/cjs/react-router.js:99:30)
      at fn (tests/js/test-utils.js:182:22)"],["Warning: An update to %s inside a test was not wrapped in act(...).·
      When testing, code that causes React state updates should be wrapped into act(...):·
      act(() => {
        /* fire events that update state */
      });
      /* assert on the output */·
      This ensures that you're testing the behavior the user would see in the browser. Learn more at https://reactjs.org/link/wrap-tests-with-act%s", "TestHook", "
      at fn (node_modules/@testing-library/react-hooks/lib/pure.js:39:23)
          at Suspense
      at construct (node_modules/react-router/cjs/react-router.js:99:30)
      at fn (tests/js/test-utils.js:182:22)"],["Warning: An update to %s inside a test was not wrapped in act(...).·
      When testing, code that causes React state updates should be wrapped into act(...):·
      act(() => {
        /* fire events that update state */
      });
      /* assert on the output */·
      This ensures that you're testing the behavior the user would see in the browser. Learn more at https://reactjs.org/link/wrap-tests-with-act%s", "TestHook", "
      at fn (node_modules/@testing-library/react-hooks/lib/pure.js:39:23)
          at Suspense
      at construct (node_modules/react-router/cjs/react-router.js:99:30)
      at fn (tests/js/test-utils.js:182:22)"]

DashboardSharingSettings › Single Admin Environment › should not render sharing management for a single admin environment

Example: https://github.com/google/site-kit-wp/actions/runs/12632804329/job/35197172249#step:7:744

Details
FAIL assets/js/components/dashboard-sharing/DashboardSharingSettings/index.test.js
  ● DashboardSharingSettings › Single Admin Environment › should not render sharing management for a single admin environment

    expect(jest.fn()).not.toHaveErrored(expected)

    Expected mock function not to be called but it was called with:
    ["Warning: An update to %s inside a test was not wrapped in act(...).·
    When testing, code that causes React state updates should be wrapped into act(...):·
    act(() => {
      /* fire events that update state */
    });
    /* assert on the output */·
    This ensures that you're testing the behavior the user would see in the browser. Learn more at https://fb.me/react-wrap-tests-with-act%s", "ForwardRef(Tooltip)", "
        in ForwardRef(Tooltip) (created by WithStyles(ForwardRef(Tooltip)))
        in WithStyles(ForwardRef(Tooltip)) (created by Tooltip)
        in Tooltip (created by Button)
        in Button (created by ForwardRef)
        in div (created by ForwardRef)
        in ForwardRef (created by Module)
        in div (created by Module)
        in div (created by Module)
        in Module (created by DashboardSharingSettings)
        in div (created by DashboardSharingSettings)
        in div (created by DashboardSharingSettings)
        in DashboardSharingSettings
        in Router (created by Wrapper)
        in Wrapper"],["Warning: An update to %s inside a test was not wrapped in act(...).·
    When testing, code that causes React state updates should be wrapped into act(...):·
    act(() => {
      /* fire events that update state */
    });
    /* assert on the output */·
    This ensures that you're testing the behavior the user would see in the browser. Learn more at https://fb.me/react-wrap-tests-with-act%s", "ForwardRef(Popper)", "
        in ForwardRef(Popper) (created by ForwardRef(Tooltip))
        in ForwardRef(Tooltip) (created by WithStyles(ForwardRef(Tooltip)))
        in WithStyles(ForwardRef(Tooltip)) (created by Tooltip)
        in Tooltip (created by Button)
        in Button (created by ForwardRef)
        in div (created by ForwardRef)
        in ForwardRef (created by Module)
        in div (created by Module)
        in div (created by Module)
        in Module (created by DashboardSharingSettings)
        in div (created by DashboardSharingSettings)
        in div (created by DashboardSharingSettings)
        in DashboardSharingSettings
        in Router (created by Wrapper)
        in Wrapper"]

      at Object.assertExpectedCalls (node_modules/@wordpress/jest-preset-default/node_modules/@wordpress/jest-console/build/@wordpress/jest-console/src/index.js:36:4)

SettingsCardVisitorGroups › the "Display visitor groups in dashboard" switch › should track an event when toggled

Example: https://github.com/google/site-kit-wp/actions/runs/12598816789/job/35114468658#step:7:813

Details
FAIL assets/js/modules/analytics-4/components/audience-segmentation/settings/SettingsCardVisitorGroups/index.test.js
  ● SettingsCardVisitorGroups �� the "Display visitor groups in dashboard" switch › should track an event when toggled

    expect(jest.fn()).not.toHaveErrored(expected)

    Expected mock function not to be called but it was called with:
    ["Warning: An update to %s inside a test was not wrapped in act(...).·
    When testing, code that causes React state updates should be wrapped into act(...):·
    act(() => {
      /* fire events that update state */
    });
    /* assert on the output */·
    This ensures that you're testing the behavior the user would see in the browser. Learn more at https://fb.me/react-wrap-tests-with-act%s", "SetupSuccess", "
        in SetupSuccess (created by SettingsCardVisitorGroups)
        in div (created by Cell)
        in Cell (created by SettingsCardVisitorGroups)
        in div (created by Row)
        in Row (created by SettingsCardVisitorGroups)
        in div (created by Grid)
        in Grid (created by SettingsCardVisitorGroups)
        in div (created by SettingsCardVisitorGroups)
        in div (created by Layout)
        in Layout (created by SettingsCardVisitorGroups)
        in SettingsCardVisitorGroups
        in Router (created by Wrapper)
        in Wrapper"],["Warning: An update to %s inside a test was not wrapped in act(...).·
    When testing, code that causes React state updates should be wrapped into act(...):·
    act(() => {
      /* fire events that update state */
    });
    /* assert on the output */·
    This ensures that you're testing the behavior the user would see in the browser. Learn more at https://fb.me/react-wrap-tests-with-act%s", "SettingsCardVisitorGroups", "
        in SettingsCardVisitorGroups
        in Router (created by Wrapper)
        in Wrapper"]

      at Object.assertExpectedCalls (node_modules/@wordpress/jest-preset-default/node_modules/@wordpress/jest-console/build/@wordpress/jest-console/src/index.js:36:4)

CurrentSurvey › should render the share icon button if there are shareableRoles

Example: https://github.com/google/site-kit-wp/actions/runs/12905555016/job/35984998955?pr=10055#step:7:802

Details
FAIL assets/js/components/dashboard-sharing/UserRoleSelect.test.js
  ● CurrentSurvey › should render the share icon button if there are shareableRoles

    expect(jest.fn()).not.toHaveErrored(expected)

    Expected mock function not to be called but it was called with:
    ["Warning: An update to %s inside a test was not wrapped in act(...).·
    When testing, code that causes React state updates should be wrapped into act(...):·
    act(() => {
      /* fire events that update state */
    });
    /* assert on the output */·
    This ensures that you're testing the behavior the user would see in the browser. Learn more at https://fb.me/react-wrap-tests-with-act%s", "ForwardRef(Tooltip)", "
        in ForwardRef(Tooltip) (created by WithStyles(ForwardRef(Tooltip)))
        in WithStyles(ForwardRef(Tooltip)) (created by Tooltip)
        in Tooltip (created by Button)
        in Button (created by ForwardRef)
        in div (created by ForwardRef)
        in ForwardRef
        in Router (created by Wrapper)
        in Wrapper"],["Warning: An update to %s inside a test was not wrapped in act(...).·
    When testing, code that causes React state updates should be wrapped into act(...):·
    act(() => {
      /* fire events that update state */
    });
    /* assert on the output */·
    This ensures that you're testing the behavior the user would see in the browser. Learn more at https://fb.me/react-wrap-tests-with-act%s", "ForwardRef(Popper)", "
        in ForwardRef(Popper) (created by ForwardRef(Tooltip))
        in ForwardRef(Tooltip) (created by WithStyles(ForwardRef(Tooltip)))
        in WithStyles(ForwardRef(Tooltip)) (created by Tooltip)
        in Tooltip (created by Button)
        in Button (created by ForwardRef)
        in div (created by ForwardRef)
        in ForwardRef
        in Router (created by Wrapper)
        in Wrapper"]

      at Object.assertExpectedCalls (node_modules/@wordpress/jest-preset-default/node_modules/@wordpress/jest-console/build/@wordpress/jest-console/src/index.js:36:4)

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The tests listed above should be assessed for failures in CI and locally and fixed where possible.

Implementation Brief

  • Debug and fix the failing tests outlined in the issue description

Test Coverage

  • N/A

QA Brief

Changelog entry

@techanvil techanvil added P0 High priority Type: Infrastructure Engineering infrastructure & tooling labels Jan 22, 2025
@techanvil techanvil changed the title Fix flaky tests (2/2). Fix flaky JS tests (2/2). Jan 22, 2025
@zutigrm zutigrm assigned zutigrm and unassigned zutigrm Jan 31, 2025
@aaemnnosttv aaemnnosttv self-assigned this Feb 10, 2025
@aaemnnosttv
Copy link
Collaborator

  • At

      [site-kit-wp/assets/js/modules/analytics-4/hooks/useEnableAudienceGroup.test.js](https://github.com/google/site-kit-wp/blob/cca932483d1a301da22020df0ae29e6e6dbc4403/assets/js/modules/analytics-4/hooks/useEnableAudienceGroup.test.js#L216)
    
           await actHook( () => waitForTimeouts( 30 ) ); 
    

    try awaiting a bit longer 50 ms for example

@zutigrm waitForTimeouts really shouldn't be used for component tests and should probably be deprecated entirely. This was likely introduced in the past when waitForRegistry was less robust, but also, it wasn't included in the output from render for hooks until quite recently.

Generally speaking, we should find the root cause of issues where tests are failing rather than simply increasing timeouts as this can also be a result of things like secondary actions not being awaited in a control or generator. waitForRegistry is a kind of bandaid in this way too, but it's at least tied to actual state updates rather than an arbitrary length of time.

On the whole, I think IBs are not very practical for this kind of issue as it's a rather straightforward kind of "fix the tests" type task, so it shouldn't involve major changes.

With that said, I'd say go for it, but with a bit more time allocated in the estimate as there are often unforeseeable issues that come up along the way.

@aaemnnosttv aaemnnosttv assigned zutigrm and unassigned aaemnnosttv Feb 10, 2025
@zutigrm
Copy link
Collaborator

zutigrm commented Feb 13, 2025

Thanks @aaemnnosttv makes sense, it is a bit odd sometimes scoping the IB for failing tests, I updated the IB and opened a new issue #10220 to replace usage of waitForTimeouts in component and hook tests

@aaemnnosttv
Copy link
Collaborator

Thanks @zutigrm 👍

IB ✅ 😄

@aaemnnosttv aaemnnosttv removed their assignment Feb 13, 2025
@ivonac4 ivonac4 added the Team M Issues for Squad 2 label Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority Team M Issues for Squad 2 Type: Infrastructure Engineering infrastructure & tooling
Projects
None yet
Development

No branches or pull requests

4 participants