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

refactor: [M3-7582] - Remove Enzyme #10160

Merged
merged 10 commits into from
Feb 8, 2024

Conversation

bnussman-akamai
Copy link
Contributor

@bnussman-akamai bnussman-akamai commented Feb 7, 2024

Description ๐Ÿ“

Removing Enzyme in favor or React Testing Library has been in the works for years now. This PR's goal is to finally get Enzyme out of here ๐Ÿ—‘๏ธ. We're really close, so I'm just going to do it all in one sweep with this PR. ๐ŸŽ‰

Notes โœ๏ธ

  • Removing Enzyme will make updating to React 18 a bit easier.
  • For the most part, I tried my best to replace each enzyme test with a React Testing library test that is equivalent. The main goal of this PR is to get rid of enzyme, so don't expect to see super perfect or extensive new tests. ๐Ÿ˜ฎโ€๐Ÿ’จ

How to test ๐Ÿงช

  • Verify unit tests pass in Github Actions
  • Optional: check out this PR and run some of the unit tests that have been touched
  • Verify that we're not losing any meaningful test coverage

As an Author I have considered ๐Ÿค”

  • ๐Ÿ‘€ Doing a self review
  • โ” Our contribution guidelines
  • ๐Ÿค Splitting feature into small PRs
  • โž• Adding a changeset
  • ๐Ÿงช Providing/Improving test coverage
  • ๐Ÿ” Removing all sensitive information from the code and PR description
  • ๐Ÿšฉ Using a feature flag to protect the release
  • ๐Ÿ‘ฃ Providing comprehensive reproduction steps
  • ๐Ÿ“‘ Providing or updating our documentation
  • ๐Ÿ•› Scheduling a pair reviewing session
  • ๐Ÿ“ฑ Providing mobile support
  • โ™ฟ Providing accessibility support

@bnussman-akamai bnussman-akamai added the Unit tests Improves unit test coverage label Feb 7, 2024
@bnussman-akamai bnussman-akamai self-assigned this Feb 7, 2024
@bnussman-akamai bnussman-akamai marked this pull request as ready for review February 7, 2024 18:49
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner February 7, 2024 18:49
@bnussman-akamai bnussman-akamai requested review from cpathipa and hkhalil-akamai and removed request for a team February 7, 2024 18:49
Copy link

github-actions bot commented Feb 7, 2024

Coverage Report: โŒ
Base Coverage: 81.19%
Current Coverage: 81.11%

@@ -199,9 +195,11 @@
"mocha-junit-reporter": "^2.2.1",
"msw": "~1.3.2",
"prettier": "~2.2.1",
"react-test-renderer": "16.14.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have to explicitly define this now that enzyme is gone

If we don't install this, we'll get the following:
Screenshot 2024-02-07 at 3 35 29 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My hope is that when we upgrade React Testing Library, we can remove this explicit definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thankfully, we don't use "Pagey" anywhere so I was able to remove all of this!

Copy link
Contributor

@hkhalil-akamai hkhalil-akamai left a comment

Choose a reason for hiding this comment

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

Great work rewriting all of these tests! I glanced over all changes and ran tests and coverage locally and everything looks good.

Copy link
Contributor

Choose a reason for hiding this comment

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

Rip Pagey, whatever it was ๐Ÿชฆ

Comment on lines -175 to -176
"enzyme": "^3.10.0",
"enzyme-adapter-react-16": "^1.14.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

โœŒ๏ธ

@bnussman-akamai bnussman-akamai added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Feb 8, 2024
Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Awesome cleanup!! Thanks for taking care of this.

Tests run locally fine โœ…
Tests equivalents look good to me and cleaner โœ…

@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Feb 8, 2024
@bnussman-akamai bnussman-akamai merged commit ef6d814 into linode:develop Feb 8, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Unit tests Improves unit test coverage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants