search

openedx / wg-frontend

Open edX Frontend Working Group
4 stars 0 forks source link

`@edx/frontend-build` v12.8.61 upgrades Jest v26 -> v27, but causes breaking changes for consumers #174

Closed adamstankiewicz closed 9 months ago

adamstankiewicz commented 1 year ago

When upgrading @edx/frontend-build to its latest version in at least the frontend-app-learner-portal-enterprise MFE, it was observed that CI begin failing immediately after successfully passing all the Jest tests run via npm run test:

image

It appears upgrading Jest from v26 -> v27 brought in some breaking changes (see release notes) that could impact consumers, where we may want to ensure that upgrade gets properly released as a breaking change to mitigate these concerns.

The relevant breaking change in Jest v27 I believe is impacting at least frontend-app-learner-portal-enterprise (see draft upgrade PR) is:

[jest-runner] [BREAKING] set exit code to 1 if test logs after teardown (https://github.com/facebook/jest/pull/10728)

I believe this is the case given some tests in this impacted MFE are seeing warnings such as:

●  Cannot log after tests are done. Did you forget to wait for something async in your test?
    Attempted to log "Unhandled Rejection at: Promise Promise {
      <rejected> Error: getLocale called before configuring i18n. Call configure with messages first.
          at getLocale (/home/runner/work/frontend-app-learner-portal-enterprise/frontend-app-learner-portal-enterprise/node_modules/@edx/src/i18n/lib.js:154:11)
          at ErrorPage (/home/runner/work/frontend-app-learner-portal-enterprise/frontend-app-learner-portal-enterprise/node_modules/@edx/src/react/ErrorPage.jsx:26:49)
          at renderWithHooks (/home/runner/work/frontend-app-learner-portal-enterprise/frontend-app-learner-portal-enterprise/node_modules/react-dom/cjs/react-dom.development.js:14803:18)
console.error
      Warning: An update to MarkCompleteModal 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
          in MarkCompleteModal
          in WrapperComponent

      60 |       });
      61 |     } catch (error) {
    > 62 |       setState({
         |       ^
      63 |         confirmButtonState: 'default',
      64 |         confirmError: error,
      65 |       });

      at printWarning (node_modules/react-dom/cjs/react-dom.development.js:88:30)
      at error (node_modules/react-dom/cjs/react-dom.development.js:60:5)
      at warnIfNotCurrentlyActingUpdatesInDEV (node_modules/react-dom/cjs/react-dom.development.js:23284:7)
      at setState (node_modules/react-dom/cjs/react-dom.development.js:15656:9)
      at handleConfirmButtonClick (src/components/dashboard/main-content/course-enrollments/course-cards/mark-complete-modal/MarkCompleteModal.jsx:62:7)

Now that Jest v27 sets error code to 1 if such logs occur after the test teardown, I believe this MFE fails CI. Any other MFE that upgrades to this version and has similar concerns in their tests around async operations may begin failing CI unexpectedly.

This issue unfortunately could not be replicated locally; only in CI.

I would recommend the following next steps:

BilalQamar95 commented 1 year ago

Initially we tested it for multiple different MFEs but it was working fine but you are correct here, with more thorough inspection we came across 2 more, frontend-lib-content-components & frontend-app-enterprise-public-catalog for which it's also failing. Thanks for bringing it to our attention, I have made a revert for it and we will open a new PR to upgrade to Jest as a breaking change.

arbrandes commented 9 months ago

Looks like this is done! Meaning, the upgrade was reverted and a breaking change PR made.