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

Add Global Setup/Teardown APIs #4716

Merged
merged 1 commit into from
Dec 13, 2017
Merged

Conversation

xfumihiro
Copy link
Contributor

@xfumihiro xfumihiro commented Oct 17, 2017

Summary

Adding Global Setup/Teardown APIs

Test plan

jest.config.js

module.exports = {
  globalSetup: "path/to/setup.js",
  globalTeardown: "path/to/teardown.js"
};

setup/teardown.js

module.exports = async function() {
  // setup/teardown tasks
}

### `globalSetup` [string]
Default: `undefined`

This option allows the use of a custom global setup module which exports an async function.
Copy link
Member

@SimenB SimenB Oct 19, 2017

Choose a reason for hiding this comment

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

what does "global setup" mean? (I know, but I want the docs to clearly state what the purpose of the option is)

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.

I'm not sure the tests prove anything beyond the what setup and teardown does for test environments. I'd add more test files and have a test asserting that whatever sideeffect the setup does is only done once.

Also please update the changelog

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.

Just a small suggestion for assertion, but overall this looks great! Really looking forward to seeing what we can do with puppeteer once it's properly supported in Jest. Thanks for all your help!


test('should exist setup file', () => {
const files = fs.readdirSync(DIR);
expect(files.length).toBe(1);
Copy link
Member

Choose a reason for hiding this comment

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

toHaveLength (can be used multiple places)

const fs = require('fs');
const os = require('os');

const DIR = os.tmpdir() + '/jest';
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we replace all these concats with path.join/path.resolve?

@SimenB
Copy link
Member

SimenB commented Oct 26, 2017

Will this run once per worker, or per jest process?

@thymikee
Copy link
Collaborator

It will run once per jest instance.

@xfumihiro
Copy link
Contributor Author

Should we make it run once upon all jest instance?

@SimenB
Copy link
Member

SimenB commented Oct 29, 2017

I'm not sure what makes most sense. I'm inclined to keeping it once per jest process and not per worker. @cpojer?

@cpojer
Copy link
Member

cpojer commented Oct 30, 2017

I'm not particularly happy about this. What problem is this feature solving? The way this works right now is that it runs before any tests and after any tests, but it in the parent context and not in the context of tests. Running it once per worker makes little sense, because that is for a set of 0–N tests and it's not deterministic (workers could crash and be restarted etc.). Making it once per test seems unnecessary also, because we now have async hooks for custom test environments.

@OzairP
Copy link

OzairP commented Nov 9, 2017

@cpojer This PR could solve my issue. Currently, I am testing an app that consumes a port. When Jest concurrently runs tests, all my suites attempt to consume the same port. I have to pass in the runInBand flag to prevent this, it slows down testing.

The global setup and teardown will ensure only one server will spawn to consume a port, and all tests will test off that same server.

@vajahath
Copy link

I'm not sure why someone is not particularly happy about this PR when problems like this will be solved by this PR.
This is an essential requirement. I don't think there is an alternative. 😕

const DIR = path.join(os.tmpdir(), '/jest');

module.exports = function() {
return new Promise((resolve, reject) => {

Choose a reason for hiding this comment

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

@xfumihiro Everything inside this function in synchronous. So do we really need to wrap this in a promise?
the same applies to tread-down also.

I think this is causing the test to fail.

Copy link
Contributor Author

@xfumihiro xfumihiro Dec 11, 2017

Choose a reason for hiding this comment

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

Tests only failed on windows(appveyor) so that can't be the case.
Otherwise all tests should failed also.
However, until the team approves this PR, there's no point spending more time on it.

@cpojer
Copy link
Member

cpojer commented Dec 13, 2017

@SimenB mind resolving the conflict? Happy to merge after all the people are supportive.

@SimenB
Copy link
Member

SimenB commented Dec 13, 2017

@xfumihiro can you rebase and fix the test on appveyor?

@xfumihiro
Copy link
Contributor Author

@SimenB Sure!

@xfumihiro xfumihiro force-pushed the global_setup_teardown branch 2 times, most recently from a971c18 to 0fbc771 Compare December 13, 2017 16:20
@codecov-io
Copy link

Codecov Report

Merging #4716 into master will decrease coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4716      +/-   ##
==========================================
- Coverage   60.76%   60.75%   -0.01%     
==========================================
  Files         198      198              
  Lines        6603     6607       +4     
  Branches        4        4              
==========================================
+ Hits         4012     4014       +2     
- Misses       2591     2593       +2
Impacted Files Coverage Δ
packages/jest-config/src/index.js 23.8% <ø> (ø) ⬆️
packages/jest-config/src/defaults.js 100% <ø> (ø) ⬆️
packages/jest-config/src/normalize.js 91.95% <ø> (ø) ⬆️
packages/jest-cli/src/run_jest.js 53.57% <50%> (-0.28%) ⬇️

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 f20344b...fc15a72. Read the comment docs.

@cpojer cpojer merged commit 5d24695 into jestjs:master Dec 13, 2017
@cpojer
Copy link
Member

cpojer commented Dec 13, 2017

Let's do it! Thanks everyone for chiming in.

@SimenB
Copy link
Member

SimenB commented Dec 13, 2017

An example in the docs for usage with puppeteer would be awesome! winkwinknudgenudge

@xfumihiro
Copy link
Contributor Author

xfumihiro commented Dec 14, 2017

@cpojer could you publish the beta so I can write an example for this. 😃

@mathieumg mathieumg mentioned this pull request Dec 14, 2017
@SimenB
Copy link
Member

SimenB commented Dec 16, 2017

@xfumihiro a beta has been published.

Also, see https://medium.com/@jsilvax/getting-started-with-jest-and-puppeteer-7cf6c59a2cae

@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 13, 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.

8 participants