jestjs / jest

Delightful JavaScript Testing.
https://jestjs.io
MIT License
44.2k stars 6.46k forks source link

Run globalSetup and globalTeardown only once in watch mode #6800

Closed sbekrin closed 2 years ago

sbekrin commented 6 years ago

🚀 Feature Proposal

Hey there 👋 I'd like to propose running globalSetup and globalTeardown functions only once before actually entering watch mode and then after exiting it. I can't think of any side-effects atm, it could be opt-in/out configuration flag if there's any.

Motivation

Global setup hooks are great to prepare performance-wise expensive environment, and it would be helpful to run it just once before starting and stopping Jest. Currently Jest re-runs whole global setup/teardown on each change detected in watch mode.

Example

puppeteer and any other expensive server-kind processes, e.g. webpack-serve are good examples. Having single instance running in background during --watch mode would be amazing performance boost.

palmerj3 commented 6 years ago

It sounds more appropriate to automate this with bash (however you invoke jest watch) instead of adding to jest. I can imagine it be misleading to devs if globalSetup/globalTeardown were only called once.

sbekrin commented 6 years ago

Hey @palmerj3 thanks for the tip. Given I'd like to integrate that into custom preset, how'd you suggest to keep process in background only for --watch mode? I can think of spawning detached process but not sure how to catch then Jest is stopped to kill that process.

palmerj3 commented 6 years ago

I think you could likely achieve this with pre and post npm scripts.

So something like:

prewatch: './the_thing', watch: 'jest --watch', postwatch: './kill_the_thing'

sbekrin commented 6 years ago

@palmerj3 I need to run same setup and teardown in non-watch mode, also this holds us from moving it into custom preset. Do you think having conditional setup in global setup/teardown is feasible? Like process.on('exit', teardown) (but somehow in a sync way)

jnv commented 6 years ago

I second that. It would be useful for integration / E2E / black box tests. There could be a key prompt to run teardown + setup manually if needed.

fringd commented 5 years ago

same here. puppeteer.

samwyma commented 5 years ago

I've just faced this problem and noticed that the first argument to both globalSetup and globalTeardown comes with a watch and watchAll property. You can check if either property is true and then not do your teardown (your setup function has to cache it's result so it doesn't execute more than once). You can then use the 'signal-exit' npm package to do proper teardown.

It's a bit of a work-around, but I'm running a test elasticsearch cluster locally using this method and it saves a bunch of time in not setting that up each test run

rickhanlonii commented 5 years ago

@samwyma thanks for checking into this!

I think that's the behavior we're going to run with for supporting this use case - rather than opting-into watch mode setup/teardown, you can opt out 👍

fringd commented 5 years ago

I'm still not clear how to opt into this. Can we get it documented?

On Wed, Nov 21, 2018, 4:37 PM Rick Hanlon II notifications@github.com wrote:

@samwyma https://github.com/samwyma thanks for checking into this!

I think that's the behavior we're going to run with for supporting this use case - rather than opting-into watch mode setup/teardown, you can opt out 👍

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/facebook/jest/issues/6800#issuecomment-440817647, or mute the thread https://github.com/notifications/unsubscribe-auth/AAQpRZTJlJZ-EbGEmnENqLs-V6af3W7Oks5uxcd-gaJpZM4Vr_xT .

rickhanlonii commented 5 years ago

Something like:

// jest.config.js
module.exports = {
  globalSetup = 'setup.js'
};

// setup.js
module.exports = async (config) => {
  if (config.watch || config.watchAll) return;
  // ...
};
fringd commented 5 years ago

Thanks for the followup! It didn't do what the OP was asking though.

Hey there 👋 I'd like to propose running globalSetup and globalTeardown functions
only **once** before actually entering watch mode and then after exiting it.

emphasis mine.

I tried The above and it makes the setup and teardown run zero times.

I guess I'd have to store a global variable or something?

How exactly can I opt into this behavior.

Just to push you to explain better, why would you want to opt into this? If your tests are independent and the setup is only needed before any tests and the teardown after all tests the same test running multiple times should not need an extra teardown/setup just like multiple tests in the same run don't need it.

basically. I think the original reporter is asking for an intuitive change that would make jest --watch faster for everybody, and that shouldn't hurt anybody using jest in a reasonable way.

samwyma commented 5 years ago

Yeah, the above isn't exactly what you need.

Here's pretty much the setup I'm using for elasticsearch (but some other database or service should work too):

// setup.js
const { startServer, ES_PORT } = require("./elasticsearch");
module.exports = async () => {
  await startServer();

  process.env.AWS_ES_ENDPOINT = `http://localhost:${ES_PORT}`;
}

// teardown.js
const { destroyCluster } = require("./elasticsearch");

module.exports = async function(opts) {
  if (!opts.watch && !opts.watchAll) {
    await destroyCluster();
  }
};

// jest.config.js
module.exports = {
  globalSetup: "<rootDir>/setup.js",
  globalTeardown: "<rootDir>/teardown.js",
};

// elasticsearch.js
const onExit = require("signal-exit");

let clusterSetupPromise;
async function setupServer() {
  let cluster = createCluster(); //This uses the package esvm

  onExit(function() {
    cluster.shutdown().then(function() {
      process.exit();
    });
  });

  // ...setup elasticsearch...

  runningCluster = cluster;

  return cluster;
}

module.exports = {
  async startServer() {
    if (clusterSetupPromise) {
      return clusterSetupPromise;
    } else {
      clusterSetupPromise = setupServer();
      return clusterSetupPromise;
    }
  },
  async destroyCluster() {
    if (runningCluster) {
      await runningCluster.shutdown();
      clusterSetupPromise = null;
      runningCluster = null;
    }
  }
};

I apologise that the example is somewhat verbose, but there are a few key things to it:

I have to admit this seems like a bit of a hack. I'd advocate for something like a flag to be set on the options given to the setup/teardown the first time setup is run and the last time teardown will be run. Not sure how possible this is, but it would make my code clearer.

But, for the purposes of just getting on with what you're doing, this is working well for me :).

Hopefully this helps! Feel free to ask any questions

fringd commented 5 years ago

thanks everybody for the help. I think I've got something working for myself. Any chance we can re-open this feature request to make doing this easier to opt into at least?

rickhanlonii commented 5 years ago

Good to hear @fringd - I'm curious, what would be easier than checking config.watch and specifying the behavior you want?

samwyma commented 5 years ago

If I understand correctly, it's mainly that we can't know when the program is going to exit and therefore when we should actually tear down our services, which makes us have to use signal-exit when then makes the code unclear (same goes to the first run of the setup script, but that's easier to get around).

So, if there was a flag or something saying "program exiting" or whatnot, that would make things a bit clearer. You could also do the same with program starting

fringd commented 5 years ago

yeah globalSetup and globalTeardown are not needed between tests, so why do they run between tests in watch mode?

On Tue, Nov 27, 2018 at 11:27 AM Sam Wyma notifications@github.com wrote:

If I understand correctly, it's mainly that we can't know when the program is going to exit and therefore when we should actually tear down our services, which makes us have to use signal-exit when then makes the code unclear (same goes to the first run of the setup script, but that's easier to get around)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/facebook/jest/issues/6800#issuecomment-442122742, or mute the thread https://github.com/notifications/unsubscribe-auth/AAQpRaqkb2Ay24zfeOaypdkmesmr2OXYks5uzWfmgaJpZM4Vr_xT .

fringd commented 5 years ago

If that doesn't make sense to anybody but me and reporter, at least give us an easy way to test if the setup or teardown is actually the real first setup/last teardown.

On Tue, Nov 27, 2018 at 12:01 PM Ram Dobson fringd@gmail.com wrote:

yeah globalSetup and globalTeardown are not needed between tests, so why do they run between tests in watch mode?

On Tue, Nov 27, 2018 at 11:27 AM Sam Wyma notifications@github.com wrote:

If I understand correctly, it's mainly that we can't know when the program is going to exit and therefore when we should actually tear down our services, which makes us have to use signal-exit when then makes the code unclear (same goes to the first run of the setup script, but that's easier to get around)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/facebook/jest/issues/6800#issuecomment-442122742, or mute the thread https://github.com/notifications/unsubscribe-auth/AAQpRaqkb2Ay24zfeOaypdkmesmr2OXYks5uzWfmgaJpZM4Vr_xT .

fringd commented 5 years ago

or another superGlobalSetup superGlobalTeardown pair

fringd commented 5 years ago

rick, maybe you can explain why you wanted this to be opt-in? is there some implementation difficulty or use-case that would be broken if globalSetup/Teardown was not run with each watch run?

rickhanlonii commented 5 years ago

Hey @fringd @samwyma, thanks for explaining this more!

I understand this better now and I think I was wrong to close it since it's not as easy to opt-out of running setup/teardown in watch mode as I thought. Having to attach to process.exit to opt-out is not a great experience

@fringd it's hard to say who is using this and how, so I think we would need a pretty compelling reason to flip the behavior. I'm happy to discuss opt-out/opt-in more though after we have a good solution for either

Also interested in getting @SimenB, @thymikee and @xfumihiro's thoughts as they were more involved in creating the feature originally (maybe we didn't mean for it to run in watch mode this way)

SimenB commented 5 years ago

Running them exactly once makes sense to me, but I also bet people will say "I want to create a test DB and populate it when tests starts to run, and cleaning it after tests run. Only running it once will keep data in it from the last run". Maybe a somewhat bad example, but something like it is bound to come up.

One thing we kinda could do is

function globalSetup() {}

globalSetup.once = true;

module.exports = globalSetup;

but I reaaaally don't like that sort of API

jnv commented 5 years ago

I understand the current behavior since Jest provides strong guarantees of test isolation, so having option to opt-out or separate steps in the lifecycle (like superGlobalSetup mentioned above) would be helpful.

To illustrate another use case, we use Jest for blackbox testing of an AMQP service. In our case, I have completely given up on globalSetup and globalTeardown due to this behaivor and wrapped it all into a shell script:

#!/bin/sh
set -e

# setup env variables ...
PORT_READINESS=$(npx get-port)

node e2e/setup.js &
PID=$!
trap 'kill $PID; node e2e/teardown.js' EXIT
npx wait-port http://:$PORT_READINESS
echo "Server ready"
npx jest "$@"

This lets us run integration tests in watch mode without restarting the service and still run the teardown code on exit. On the other hand, now I need to think about running the shell script (or npm test) rather than npx jest directly.

SimenB commented 5 years ago

If that doesn't make sense to anybody but me and reporter, at least give us an easy way to test if the setup or teardown is actually the real first setup/last teardown.

Since we use require on the files, they're cached. So you can keep track of that yourself.

let hasRun = false;

module.exports = () => {
  if (hasRun) return;

  hasRun = true;

  // ...blabla
};

https://github.com/facebook/jest/blob/b502c07e4b5f24f491f6bf840bdf298a979ec0e7/packages/jest-cli/src/runJest.js#L259 https://github.com/facebook/jest/blob/b502c07e4b5f24f491f6bf840bdf298a979ec0e7/packages/jest-cli/src/runJest.js#L282

I'm not sure I would rely 100% on that working forever, but it should work for now (I haven't tested it)

fringd commented 5 years ago

@SimenB that doesn't work for teardown. in teardown we need to know if it will run again in the future. which is harder to guess from my perspective. I want it to run when the user hits ctrl-c or q

fringd commented 5 years ago

Running them exactly once makes sense to me, but I also bet people will say "I want to create a test DB and populate it when tests starts to run, and cleaning it after tests run. Only running it once will keep data in it from the last run". Maybe a somewhat bad example, but something like it is bound to come up.

This is sort of a bad use case though because the tests aren't independent, right? Can we maybe deprecate support for this and change the behavior of globalSetup/globalTeardown in the next major version? Users can still support that use case with

describe('needs fresh data', () => {
  beforeAll(setupData);
  ...
});
fringd commented 5 years ago

or just add a bit to jest config "setupTeardownOnceInWatchMode" or something

esimons commented 5 years ago

Looks like the suggested approach of

let hasRun = false;

module.exports = () => {
  if (hasRun) return;

  hasRun = true;

  // ...blabla
};

is no longer viable in Jest 24, I'm guessing as a side-effect of moving to transpile the setup/teardown hooks. (It worked well in 23, though, so thanks for the suggestion!) As I'm not aware of any potential workaround short of dumping a flag to disk, do you think this issue might gain any traction?

littlemaneuver commented 5 years ago

with the approach on preventing globalTeardown to run in a watch mode you could just add to your script another teardown that could be called from the bash and then just do jest ... --watch && node ./teardown-script.js which should be just calling your globalTeardown after you hit q or ctrl+c

fringd commented 5 years ago

That shell script would probably work. Again, I'm sure somebody will complain if we change it, but shouldn't we just change it to the right thing and let them catch up? Tests should be independent. If you are doing something like depending on the order that your test suites run, your life is already a living hell and probably you won't notice.

fringd commented 4 years ago

another gentle ping on this almost a year later. Maybe changing the behavior of globalTeardown/Setup could be rolled into the next major release?

hjr3 commented 4 years ago

I currently rely on this behavior in my test suite. In jest.setup.ts, I create a node server that listens for API requests. Each test then sends requests to that node server. If I change the API code, then I want the node server to be recreated before running the tests again.

If this does change, I suggest we also add a way to setup/teardown per watch iteration as well.

github-actions[bot] commented 2 years ago

This issue is stale because it has been open for 1 year with no activity. Remove stale label or comment or this will be closed in 14 days.

github-actions[bot] commented 2 years ago

This issue was closed because it has been stalled for 7 days with no activity. Please open a new issue if the issue is still relevant, linking to this one.

github-actions[bot] commented 2 years ago

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.