Skip to content

fix: disable React 17 tests #1504

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

Merged
merged 1 commit into from
Oct 12, 2023
Merged

fix: disable React 17 tests #1504

merged 1 commit into from
Oct 12, 2023

Conversation

mdjastrzebski
Copy link
Member

@mdjastrzebski mdjastrzebski commented Sep 20, 2023

Summary

Disable React 17 tests on CI, as they were always failing due to snapshot miss match as well of some unsupported features like the new ARIA props. On the other hand the way the test script has been written quietly consumed this failure, so that React 17 tests always indicated success.

I've tried to disable just this tests on the CI but could not overcome following error (local & CI):

Jest has detected the following 1 open handle potentially keeping Jest from exiting:

  ●  MESSAGEPORT

      1 | // This file and the act() implementation is sourced from react-testing-library
      2 | // https://github.com/testing-library/react-testing-library/blob/c80809a956b0b9f3289c4a6fa8b5e8cc72d6ef6d/src/act-compat.js
    > 3 | import { act as reactTestRendererAct } from 'react-test-renderer';
        | ^
      4 | import { checkReactVersionAtLeast } from './react-versions';
      5 |
      6 | type ReactAct = typeof reactTestRendererAct;

      at node_modules/scheduler/cjs/scheduler.development.js:178:17
      at Object.<anonymous> (node_modules/scheduler/cjs/scheduler.development.js:645:5)
      at Object.<anonymous> (node_modules/scheduler/index.js:6:20)
      at node_modules/react-test-renderer/cjs/react-test-renderer.development.js:19:19
      at Object.<anonymous> (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:17382:5)
      at Object.<anonymous> (node_modules/react-test-renderer/index.js:6:20)
      at Object.require (src/act.ts:3:1)
      at Object.require (src/pure.ts:1:1)
      at Object.require (jest-setup.ts:1:1)

React 17 tests could still be run locally using yarn test:react-17 script.

Test plan

@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (a9f32a6) 98.12% compared to head (b8f1d61) 98.12%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1504   +/-   ##
=======================================
  Coverage   98.12%   98.12%           
=======================================
  Files          96       96           
  Lines        5704     5704           
  Branches      885      885           
=======================================
  Hits         5597     5597           
  Misses        107      107           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mdjastrzebski mdjastrzebski changed the title fix: React 17 tests fix: disable React 17 tests Sep 20, 2023
Copy link
Collaborator

@pierrezimmermannbam pierrezimmermannbam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since they didn't work it's better that way but it means that we're only testing against react 18 now, I wonder if it means we should update the peer deps accordingly

@mdjastrzebski
Copy link
Member Author

@pierrezimmermannbam when running tests locally they pass except "open handle error" that makes them return non-zero exit code. So I think we can say we support React 17/ older versions of RN for now. That being said I think we should offically drop support for older React/RN with RNTL v13 (see #1505)

@mdjastrzebski mdjastrzebski merged commit 3be9f3b into main Oct 12, 2023
@mdjastrzebski mdjastrzebski deleted the fix/react-17-tests branch October 12, 2023 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants