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

fix: set testEnvironment to jsdom in react native example #12581

Merged
merged 2 commits into from
Mar 15, 2022

Conversation

F3n67u
Copy link
Contributor

@F3n67u F3n67u commented Mar 15, 2022

Summary

Test plan

@F3n67u F3n67u marked this pull request as draft March 15, 2022 13:10
@mrazauskas
Copy link
Contributor

mrazauskas commented Mar 15, 2022

Just tried this trick locally and it is fixing the issue I was mentioning in #12558 (comment). Let’s see what CI thinks.

Ah! CircleCI passed. Well done @F3n67u!

@F3n67u
Copy link
Contributor Author

F3n67u commented Mar 15, 2022

Interesting! Just tried this trick locally and it is fixing the issue I was mentioning in #12558 (comment). Let’s see what CI thinks.

@mrazauskas It's caused by #12553.

@F3n67u
Copy link
Contributor Author

F3n67u commented Mar 15, 2022

I set testEnvironment to jsdom when testing react-native so that react native example will not be affected by jest-environment-node. what do you think? @mrazauskas

@F3n67u F3n67u changed the title try to fix ci fix: set testEnvironment to jsdom in react native example Mar 15, 2022
@mrazauskas
Copy link
Contributor

I set testEnvironment to jsdom when testing react-native

This also solves the problem. I just think wouldn’t it be better idea to add it to react-native preset? Hm.. For reason the preset sets testEnvironment: 'node'. Hard to say why. I have zero experience with React and React Native.

@F3n67u
Copy link
Contributor Author

F3n67u commented Mar 15, 2022

I just think wouldn’t it be better idea to add it to react-native preset?

@mrazauskas I agree with you. I think it would be better to set testEnvironment: 'node' to react native preset if the jest-environment-jsdom is more reasonable to react native app, instead of just overriding testEnvironment in our example.

I haven't any experience with react-native either. I will spend some time investigating.

@F3n67u
Copy link
Contributor Author

F3n67u commented Mar 15, 2022

@SimenB created the react-native preset. do you have any advice here?

@SimenB
Copy link
Member

SimenB commented Mar 15, 2022

Thankfully, all I did was move it from json to js 🙂

facebook/react-native#22972

What they should do is create their own test environment rather than use the built-in node or jsdom ones... It's neither in practice. But I doubt that's going to happen.

The change in this PR should be made in the preset though, not in the example in this repo.

@F3n67u
Copy link
Contributor Author

F3n67u commented Mar 15, 2022

@SimenB So your advice is to file a pr to change testEnviroment to jsdom in the preset, right?

@SimenB
Copy link
Member

SimenB commented Mar 15, 2022

Yep!

@F3n67u
Copy link
Contributor Author

F3n67u commented Mar 15, 2022

Yep!

I will do it! thanks for your advice.

@SimenB
Copy link
Member

SimenB commented Mar 15, 2022

No idea if they'll accept it - as mentioned neither node or jsdom is correct for them. We can land the PR here in the meantime if it unlocks something (I'm on vacation this week, so haven't looked into anything)

@F3n67u
Copy link
Contributor Author

F3n67u commented Mar 15, 2022

No idea if they'll accept it - as mentioned neither node or jsdom is correct for them. We can land the PR here in the meantime if it unlocks something (I'm on vacation this week, so haven't looked into anything)

I think it's a good idea. The broken ci make our contributor confused. we could merge this pr first. Sorry to disturb your vacation time.

@F3n67u F3n67u marked this pull request as ready for review March 15, 2022 14:04
@SimenB SimenB merged commit 8c95952 into jestjs:main Mar 15, 2022
@F3n67u F3n67u deleted the fix/circleci branch March 15, 2022 14:05
@mrazauskas
Copy link
Contributor

@F3n67u It also might be good idea to send a PR to react-native which would bump Jest to next. If MessageChannel was already included in latest alpha release, this could potentially surface some problem on their side.

@mrazauskas
Copy link
Contributor

By the way, it might be that the issue is already fix on React side. See facebook/react#20756. The bug was inside scheduler package. For now it is released as scheduler@next. All is good. Just to wait for stable release.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants