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

chore: use mocked from jest-mock in tests #1340

Merged
merged 2 commits into from
Mar 29, 2022
Merged

chore: use mocked from jest-mock in tests #1340

merged 2 commits into from
Mar 29, 2022

Conversation

0xf11nix
Copy link
Contributor

Description

This PR migrates the deprecated mocked test helper from ts-jest/utils to the suggested replacement from jest-mock in order to fix the large number of deprecation warnings, etc, being generated when running yarn test.

Most of the output refers to a single deprecation warning.
Documentation related to the source of this warning can be found in the ts-jest guides

I have added jest-mock as a devDependency in package.json, but since it is already a dependency of a number of existing packages, it does not actually result in changes to the yarn.lock file.

Notice

  • Have you followed the guidelines in our Contributing guide?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Pull Request Type

  • 🐛 Bug fix (Non-breaking Change: Fixes an issue)
  • 🛠️ Chore (Non-breaking Change: Doc updates, pkg upgrades, typos, etc..)
  • 💅 New Feature (Breaking/Non-breaking Change)

Issue (if applicable)

Closes #1293

Risk

There is some potential risk of regression in the testing environment, though all existing test are passing, so I would expect this risk to be fairly low at this point in time.

Since this is a dev/test dependency only, the possibility of this change affecting the production environment seems highly unlikely.

Testing

run yarn test and observe the lack of rank spew (example in screenshot below)

Screenshots (if applicable)

yarn-test-output

@0xf11nix 0xf11nix marked this pull request as ready for review March 28, 2022 23:46
@0xf11nix 0xf11nix requested review from a team and 0xApotheosis as code owners March 28, 2022 23:46
Copy link
Contributor

@0xApotheosis 0xApotheosis left a comment

Choose a reason for hiding this comment

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

Tested: does what it says on the tin, nice one.

I'll leave it to your discretion after you speak with @cjthompson about whether we should kill the ts-jest dependency, or if it's needed for jest tests in some way we haven't thought of - comment your learnings before merging 👌

Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

LGTM @0xf11nix! Tested locally and this removes the log spew.

As @0xApotheosis mentioned, it looks like ts-jest dependency should be able to go away. It's not used anywhere, isn't even present in yarn.lock as a transient dependency of any package, and yarn local-ci runs without any issue without it but please double-check before we merge this.

@0xf11nix
Copy link
Contributor Author

Thank you for you reviews @0xApotheosis and @gomesalexandre.

Yes, ts-jest is a weird one. While it has been, for some time, considered the standard (and required) preprocessor for Typescript in Jest, Jest@24 changes the game slightly. While there does still seem to be some uncertainty about both the future of this this package and it's relevance (see here), our current configurations, mostly the TS build and lint pipelines, mean we seem to be adequately covered for type-checking with or without this dependency, at this point in time.

The removal of ts-jest would slightly increase the chance that we may see some regression or unexpected behavior within the test env when making changes to seemingly unrelated dependencies, such as core TS/tsc, babel/babel plugins, or even jest, itself.

While this dependency has been in the codebase since initial commit, and its (now) deprecated test helpers have only actually been referenced in tests since #606, I do think it's probably reasonable to remove it.

There is still seems to be some uncertainty about where (if anywhere) ts-jest is heading, but while our tests are passing, and type-checking as expected, we can probably afford to wear the very slight risk of potential short term consequences.

Short term? Well, yes. -ish. I would expect that there will be somewhat of a backoff in risk as TS support in both core Jest and the Babel transpiler improves. Until then, we'll kind of just need to keep an eye on it.

Bonus: One less "unmet peer dependency" warning on install

@0xf11nix 0xf11nix closed this Mar 29, 2022
@0xf11nix 0xf11nix reopened this Mar 29, 2022
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Mar 29, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: ee93cf4
Status: ✅  Deploy successful!
Preview URL: https://6fb94533.web-29e.pages.dev

View logs

@gomesalexandre
Copy link
Contributor

There is still seems to be some uncertainty about where (if anywhere) ts-jest is heading, but while our tests are passing, and type-checking as expected, we can probably afford to wear the very slight risk of potential short term consequences.

Bonus: One less "unmet peer dependency" warning on install

Thanks for the detailed comment! Agreed, the risk is very low compared to having an unused dependency in package.json, we will not likely not meet any issue. Waiting for @cjthompson green light on this one before this is merged, but I agree with the rationale and everything works as intended locally so well done @0xf11nix !

@0xdef1cafe 0xdef1cafe merged commit 8a3754f into develop Mar 29, 2022
@0xdef1cafe 0xdef1cafe deleted the fix-log-spew branch March 29, 2022 16:29
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.

Fix log spew
5 participants