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

Remove console-testing-library #4603

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

aryaemami59
Copy link
Contributor

This PR:

Copy link

codesandbox bot commented Sep 4, 2024

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

Copy link

codesandbox-ci bot commented Sep 4, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5ceef38:

Sandbox Source
@examples-query-react/basic Configuration
@examples-query-react/advanced Configuration
@examples-action-listener/counter Configuration
rtk-esm-cra Configuration

Copy link

netlify bot commented Sep 4, 2024

Deploy Preview for redux-starter-kit-docs ready!

Name Link
🔨 Latest commit 9a8f521
🔍 Latest deploy log https://app.netlify.com/sites/redux-starter-kit-docs/deploys/66d7c2599451250008dd3ae4
😎 Deploy Preview https://deploy-preview-4603--redux-starter-kit-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Sep 4, 2024

Deploy Preview for redux-starter-kit-docs ready!

Name Link
🔨 Latest commit 4beb3f6
🔍 Latest deploy log https://app.netlify.com/sites/redux-starter-kit-docs/deploys/66d7c36b86ba26000842448c
😎 Deploy Preview https://deploy-preview-4603--redux-starter-kit-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Sep 4, 2024

Deploy Preview for redux-starter-kit-docs ready!

Name Link
🔨 Latest commit a083371
🔍 Latest deploy log https://app.netlify.com/sites/redux-starter-kit-docs/deploys/66d7c3c2f1744800087cd52c
😎 Deploy Preview https://deploy-preview-4603--redux-starter-kit-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Sep 4, 2024

Deploy Preview for redux-starter-kit-docs ready!

Name Link
🔨 Latest commit 5ceef38
🔍 Latest deploy log https://app.netlify.com/sites/redux-starter-kit-docs/deploys/672b904677a0a20008bb1b7a
😎 Deploy Preview https://deploy-preview-4603--redux-starter-kit-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@aryaemami59 aryaemami59 force-pushed the remove-console-testing-library branch 2 times, most recently from 8a2b7fd to e65a623 Compare September 12, 2024 09:39
@EskiMojo14
Copy link
Collaborator

hmm, what's the benefit of switching away from console-testing-library?

@phryneas
Copy link
Member

It seemed to be a good idea back then but I think at this point it's more tech debt. I do have a request here though, will add something.

const noop = () => {}
vi.spyOn(console, 'error').mockImplementation(noop)
})
const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(noop)
Copy link
Member

Choose a reason for hiding this comment

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

I just wanted to suggest that we do

Suggested change
const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(noop)
using consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(noop)

inside the tests that actually need this, and skip the mockClear and mockRestore calls, but I'm not sure if vitest actually returns a Disposable here. I know jest does (because I added the functionality there) - could you check if vitest has that now, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure I can try it. The type tests will probably fail, so I'll probably exclude the runtime tests from the type tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like vitest doesn't return a Disposable, though I might try to submit a PR to add that functionality for vitest.

Copy link
Member

Choose a reason for hiding this comment

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

Will definitely be a win in the long run, even if it's not applicable to this PR in the end :)

@aryaemami59
Copy link
Contributor Author

@EskiMojo14 There is multiple reasons:

  1. like @phryneas mentioned, it's tech debt.
  2. The inline snapshots caused some issues with Prettier and ESLint in Centralize tooling configs in monorepo #4138.
  3. It caused some issues in Migrate to React 19  #4409 with React 19.
  4. It's also somewhat of an old library that hasn't been maintained in years which is why we had to patch it several times.
  5. We also had to override the version of jest-snapshot in the root package.json because of it which in turn causes the tests for all the example apps to fail locally.
  6. We can also replace what it does by spying on the console object itself easily.

@aryaemami59 aryaemami59 force-pushed the remove-console-testing-library branch 3 times, most recently from 10738c3 to bdf6dcc Compare September 24, 2024 12:54
@aryaemami59 aryaemami59 force-pushed the remove-console-testing-library branch 4 times, most recently from 6e6368f to c23567d Compare October 14, 2024 22:57
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.

3 participants