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

Workers in parallel worker pool should have JEST_WORKER_ID assigned in ENV #2284

Closed
evantahler opened this issue Dec 10, 2016 · 17 comments
Closed

Comments

@evantahler
Copy link

evantahler commented Dec 10, 2016

Hi!

Jest is cool. Thank you for all your hard work! I'm porting ActionHero's test suite to Jest, as some of the features you have (--watch, parallel execution, etc) make testing much better.

ActionHero is a server-side node.js project, and many of our tests rely on interacting with a database (Redis for us). To fully isolate our tests, we need to boot the server on unique ports and have separate databases for each test to work against. This allows tests to run in parallel and not pollute each other.

We can try to derive a unique ID for each worker in the pool via the PID, IE: let id = process.pid % require('os').cpus().length, but you sill may end up with collisions.

If each test-running worker was assigned a unique ID from the parent like process.env.JEST_WORKER_ID = 1, that would make this possible

@cpojer
Copy link
Member

cpojer commented Dec 12, 2016

Feel free to send a PR for this. I don't think worker-farm supports the notion of ids but you can add the state in runTest here: https://github.com/facebook/jest/blob/master/packages/jest-cli/src/runTest.js

@thymikee
Copy link
Collaborator

@evantahler are you interested in implementing this?

@evantahler
Copy link
Author

Sadly no. We've gone back to Mocha as there were a few other problems with Jest which didn't work for ActionHero.

@thymikee
Copy link
Collaborator

Sad to hear that 😞. If you have some time to point out what we could make better, please file a new issue for that. Feedback is always welcome!

@guyellis
Copy link

Are there any ideas around this topic to get a unique identifier, preferably sequential starting at 1 like @evantahler suggested? I'm running into the same problem/use-case. I'm spinning up multiple servers to do integration tests to a DB and each needs to be on a unique port. Currently I'm playing with this idea:

/**
 * Checks to see if port is being used on the system.
 * @param {number} port - the port to check
 * @returns a Promise that resolves to true or false
 */
function portInUse(port) {
  return new Promise((resolve) => {
    const server = net.createServer((socket) => {
      socket.write('Echo server\r\n');
      socket.pipe(socket);
    });

    server.listen(port, '127.0.0.1');
    server.on('error', () => {
      resolve(true);
    });
    server.on('listening', () => {
      server.close();
      resolve(false);
    });
  });
}

/**
 * Recursively checks ports 3001 to 3999 to see if they are in use
 * and returns the first port that is not in use in that range
 * @param {number} [port=3001] - the port to check
 * @returns the first unused port or throws an exception if none found
 */
async function findUnusedPort(port = 3001) {
  if (port > 3999) {
    throw new Error(`Could not find a free port. Checked up to ${port}`);
  }
  const inUse = await portInUse(port);
  if (!inUse) {
    return port;
  }

  return findUnusedPort(port + 1);
}

The problem here is the race condition that the same port number will be reported as free to two or more workers.

Being able to do something like:

const port = 3000 + process.env.JEST_WORKER_ID

would solve this.

@cpojer Is this the new location of the file that launches the workers? https://github.com/facebook/jest/blob/d1cb3959f11331f432354f1607025d60ef0667bf/packages/jest-runner/src/run_test.js

Could a local module counter be incremented in here that is passed to each new worker?

@thymikee
Copy link
Collaborator

@guyellis we're currently working on switching from worker-farm to jest-worker.
cc @mjesun if this is something we'd like to have.

@maximilianschmitt
Copy link

This would be so helpful, we're having the same issue as @guyellis

@mjesun
Copy link
Contributor

mjesun commented Feb 7, 2018

I like the idea, worker classes wrapping child processes are internally stored in an array, getting each a positionary id; so it shouldn't be hard to add an extra variable. The relevant code can be found here, for the worker instantiation and here, for the process spawn.

I'd be more than happy to accept a PR implementing it 🙂

@mjesun mjesun reopened this Feb 7, 2018
@ranyitz
Copy link
Contributor

ranyitz commented Feb 7, 2018

Hey @mjesun
I'll be happy to create a PR for this!

@SimenB
Copy link
Member

SimenB commented Feb 7, 2018

Go for it 🙂

@mjesun mjesun self-assigned this Feb 7, 2018
@mjesun
Copy link
Contributor

mjesun commented Feb 7, 2018

@ranyitz can't assign you, but I picked it so others don't re-pick it. Feel free to implement it, and thanks for the contribution! ☺️

@ranyitz
Copy link
Contributor

ranyitz commented Feb 8, 2018

Thanks @SimenB @mjesun 😊
Here it is #5494

@SimenB SimenB closed this as completed Feb 8, 2018
@guyellis
Copy link

guyellis commented Jul 4, 2018

Since @ranyitz merged this feature I've been using it and it's great - thank you for doing that!

Currently using Jest 23.2.0 and this feature seems to have become unstable inasmuch as it sometimes returns "1" for JEST_WORKER_ID for all test suites. (I saw the other commit where it will always be "1" when --runInBand and this is not the issue. i.e. not using --runInBand)

I'm going to try and reproduce this issue. Before creating an issue for this has anybody else noticed this?

@guyellis
Copy link

guyellis commented Jul 4, 2018

Here is a super simple repo that shows the unexpected output in the Readme. Strange thing is that when I first created that repo I was getting the expected output with the values of 1 and 2 for the JEST_WORKER_ID and now I'm getting those unexpected results.

@guyellis
Copy link

guyellis commented Jul 4, 2018

I've filed an issue at #6608

@evantahler
Copy link
Author

Responding back to #2284 (comment), Actionhero (and related projects) now use Jest!

Jest is also a great way to text node server apps too!

@github-actions
Copy link

This issue 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

No branches or pull requests

8 participants