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

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

Closed
2 tasks done
adamstankiewicz opened this issue Jul 21, 2023 · 2 comments
Labels
bug Report of or fix for something that isn't working as intended maintenance Routine upkeep necessary for the health of the platform

Comments

@adamstankiewicz
Copy link
Member

adamstankiewicz commented Jul 21, 2023

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:

  • Understand if this issue can be reproduced in any other MFEs beyond frontend-app-learner-portal-enterprise.
  • If it can be reproduced in more than 1 MFE, consider downgrading back to Jest v26 as a patch release on frontend-build's v12 and open a new PR to upgrade to Jest v27 as a breaking change v13.
@adamstankiewicz adamstankiewicz added bug Report of or fix for something that isn't working as intended maintenance Routine upkeep necessary for the health of the platform labels Jul 21, 2023
@BilalQamar95
Copy link

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Report of or fix for something that isn't working as intended maintenance Routine upkeep necessary for the health of the platform
Projects
Status: Closed
Development

No branches or pull requests

3 participants