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 1-indexed JEST_WORKER_ID #8205

Merged
merged 3 commits into from Mar 26, 2019
Merged

Conversation

jeysal
Copy link
Contributor

@jeysal jeysal commented Mar 24, 2019

Summary

Fixes #8204
It's weird that it's 1-indexed (I looked at making our workerId 1-indexed too for consistency, but that would make our code really weird), but I guess it's documented, used to work like that and still does for runInBand 🤷‍♂️

Test plan

@codecov-io
Copy link

codecov-io commented Mar 24, 2019

Codecov Report

Merging #8205 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #8205   +/-   ##
======================================
  Coverage    62.3%   62.3%           
======================================
  Files         265     265           
  Lines       10473   10473           
  Branches     2542    2541    -1     
======================================
  Hits         6525    6525           
  Misses       3366    3366           
  Partials      582     582
Impacted Files Coverage Δ
...ages/jest-worker/src/workers/ChildProcessWorker.ts 88.05% <ø> (ø) ⬆️
...kages/jest-worker/src/workers/NodeThreadsWorker.ts 89.23% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e4e170...b042442. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Mar 26, 2019

doing +1 in both implementations feels brittle, can't we fixed the number passed into the constructors instead? Wherever the count itself is kept (in a loop somewhere?) is where it should be corrected, not spread the logic around

@jeysal
Copy link
Contributor Author

jeysal commented Mar 26, 2019

I tried but making workerId itself 1-indexed is terrible, it's used to index into an array. I'd love to just make everything 0-indexed but that's breaking

@jeysal
Copy link
Contributor Author

jeysal commented Mar 26, 2019

And 0-indexed in the pool and 1-indexed in individual workers would be way more confusing than this

@SimenB
Copy link
Member

SimenB commented Mar 26, 2019

I wasn't afraid of it being confusing, more about duplicating logic across multiple files. But if it's used as array accessors then I agree this should be fine

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.

Changelog, then we should be good

@jeysal
Copy link
Contributor Author

jeysal commented Mar 26, 2019

Added changelog 👍

@jeysal jeysal merged commit 54ce3f3 into jestjs:master Mar 26, 2019
@jeysal jeysal deleted the worker-id-starts-at-1 branch March 26, 2019 10:42
Hoishin added a commit to Hoishin/jest that referenced this pull request Mar 26, 2019
* facebook/master:
  fix 1-indexed JEST_WORKER_ID (jestjs#8205)
  remove flow leftovers (jestjs#8213)
@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 11, 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.

JEST_WORKER_ID starts with 0.
5 participants