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

use already read cpus variable only once and improve code little usi… #10842

Merged
merged 2 commits into from
Mar 4, 2021

Conversation

sktguha
Copy link
Contributor

@sktguha sktguha commented Nov 18, 2020

…ng Math.max

Summary

This PR improves the code of getMaxWorkers.ts a little bit, use already read cpus variable only once and improve code little using Math.max

Test plan

Unfortunately I am getting a typescript issue at the yarn build stage,


18 export declare function watch(path: string, since: number, handler: WatchHandler);

So unable to run tests. Could someone who has the environment set up checkout this branch and run the tests ?
Anyways the code change is really small

Thanks
Saikat Guha
sktguha@gmail.com

@sktguha
Copy link
Contributor Author

sktguha commented Nov 18, 2020

getting mercurial not installed in the CI. not sure why. I definitely didn't make any changes in that regard.

@sktguha
Copy link
Contributor Author

sktguha commented Nov 26, 2020

Hello could anyone take a look at this please ?

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

thanks!

packages/jest-config/src/getMaxWorkers.ts Outdated Show resolved Hide resolved
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
@sktguha
Copy link
Contributor Author

sktguha commented Nov 26, 2020

Hello thanks for approval,

made the requested changes,
Could you merge if ok now, however as mentioned earlier , unable to test on my machine due to above mentioned error in the test plan section.
Let's hope the CI runs all the checks

@SimenB
Copy link
Member

SimenB commented Nov 26, 2020

To be able to run locally, you can apply the diff from https://github.com/fsevents/fsevents/pull/348/files in you node_modules

@sktguha
Copy link
Contributor Author

sktguha commented Nov 26, 2020

All right sure thanks will do that and update soon

@SimenB
Copy link
Member

SimenB commented Dec 5, 2020

@sktguha I've rolled back the version of fsevents in this repo, so rebasing and running yarn will give you a working version of fsevents

@sktguha
Copy link
Contributor Author

sktguha commented Dec 5, 2020

@SimenB oh that's great. Thanks let me try it out and run the tests locally and update here

@SimenB
Copy link
Member

SimenB commented Feb 18, 2021

@sktguha ping 🙂

@sktguha
Copy link
Contributor Author

sktguha commented Feb 19, 2021

oh hi sorry, forgot. lemme update PR 🙂

@sktguha
Copy link
Contributor Author

sktguha commented Feb 19, 2021

I am getting the following errors on running yarn test after rebasing with master . But it doesn't seem to be related to my changes. @SimenB
image

@sktguha
Copy link
Contributor Author

sktguha commented Feb 23, 2021

@SimenB

@sktguha
Copy link
Contributor Author

sktguha commented Mar 3, 2021

@SimenB bump 😀

@SimenB
Copy link
Member

SimenB commented Mar 3, 2021

Hey! SOrry about the slow response.

Linting is passing on CI on master. Did you run yarn? I'd attempt to push - you haven't touched those files so it won't matter.

If you want to just run tests you can run yarn jest (instead of test)

@sktguha
Copy link
Contributor Author

sktguha commented Mar 4, 2021

Yesy changes are already pushed. don't have anything else to push 🙂

@SimenB
Copy link
Member

SimenB commented Mar 4, 2021

This PR does not fail lint

image

So there is something with your local clone in this case

@sktguha
Copy link
Contributor Author

sktguha commented Mar 4, 2021

Huh alright. Actually I think it can be merged. there is nothing wrong with the part of the code I have changed. The issues are not related to that part of code.

@SimenB SimenB merged commit 1d8f955 into jestjs:master Mar 4, 2021
@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 May 10, 2021
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.

3 participants