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

Setup before tests but after framework loads #7119

Merged
merged 19 commits into from Oct 21, 2018

Conversation

ddruker
Copy link
Contributor

@ddruker ddruker commented Oct 8, 2018

Summary

Currently the setupTestFrameworkScriptFile option only allows for a single module to be run before each test file is run but after the test framework has been loaded. The option only accepts a string, which is limited and could force someone to import multiple modules into a single file.

This PR will create a new configuration called setupFilesAfterEnv, which is by default an empty array, that can accept multiple paths.

Please note setupTestFrameworkScriptFile is being deprecated.

Closes #6942

Sample configuration

module.exports = {
  testRunner: "jest-circus/runner",
  setupFiles: ["./before/setupJestBefore.js"],
  setupFilesAfterEnv: [
    "./after/setupAfter1.js",
    "./after/setupAfter2.js"
  ]
};

Test plan

Updated snapshot tests, unit tests and integration tests for configuration.

@ddruker ddruker changed the title Run multiple files before each test but after framework is loaded Setup before tests but after framework loads Oct 8, 2018
@@ -77,6 +77,12 @@ const jestAdapter = async (
runtime.requireModule(config.setupTestFrameworkScriptFile);
}

if (config.setupTestsAfterJest.length) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code will run setup files after any file set in setupTestFrameworkScriptFile. I wasn't sure how we want to handle this. Here are some ideas.

  1. Add details in the docs stating that the files set in the new config will run after that old option.
  2. In preparation for the deprecation of setupTestFrameworkScriptFile, only run the new config if set.

Copy link
Contributor

@wsmd wsmd Oct 8, 2018

Choose a reason for hiding this comment

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

Hmm.. you bring up a good point! I lean more towards option 2 since both serve the same purpose. I can't think of a case where I would have both set in my configuration.

With that, another idea I'd like to suggest is that if both are set, we could log a warning indicating that setupTestsAfterJest is taking precedence, and that setupTestFrameworkScriptFile is being ignored.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, how's the behaviour like if you pass in a wrong path to a file? Is that error handled or is that handled by the runtime. It would be good to gracefully handle that scenario

Copy link
Member

Choose a reason for hiding this comment

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

What about changing line 77 to:

config.setupTestsAfterJest.push(config.setupTestFrameworkScriptFile);

Copy link
Contributor Author

@ddruker ddruker Oct 14, 2018

Choose a reason for hiding this comment

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

@rickhanlonii Awesome. We would need to include info in the docs that mentions the module set using setupTestFrameworkScriptFile will run after any code set using the new configuration. If I do that, we should probably remove the original implementation of setupTestFrameworkScriptFile in the lines above. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah agreed, can probably add it to the doocs here https://jestjs.io/docs/en/configuration#setuptestframeworkscriptfile-string

What's the original implementation? Do you mean just line 77?

@thymikee
Copy link
Collaborator

thymikee commented Oct 8, 2018

How about naming this "setupFilesAfterJest"?

@SimenB
Copy link
Member

SimenB commented Oct 8, 2018

IMO this should at least print a deprecation warning for setupTestFrameworkScriptFile, and maybe even remove it as well. No reason to support 2 ways of doing the same thing, silently or otherwise. And the next release is a major, so we're good to make breaking changes

@mattphillips
Copy link
Contributor

Yup I agree with @SimenB let’s remove setupTestFrameworkScriptFile as part of this change

@ddruker
Copy link
Contributor Author

ddruker commented Oct 8, 2018

@thymikee I like it, although there was some consensus around setupTestsAfterJest in the issue so I moved forward with the name. Your suggestion may make more sense. @SimenB, what do you think?

@SimenB What are the steps to deprecate the setupTestFrameworkScriptFile option? I don't want to make too many assumptions about that process.

@rickhanlonii
Copy link
Member

Good stuff @ddruker

+1 for either setupFilesAfterJest or setupFilesAfterEnv

These have consistency with the existing setupFiles option. I'd recommend "AfterEnv" over "AfterJest" as it's more specific to the usage and it's not really clear when "after jest" is

@SimenB
Copy link
Member

SimenB commented Oct 14, 2018

What are the steps to deprecate the setupTestFrameworkScriptFile option?

Sorry, didn't notice the question! It's simply a matter of adding the option you want to deprecate in here, and add some appropiate help text for the user: https://github.com/facebook/jest/blob/f00fbecb4b29b1f22582255d13591344a862b88e/packages/jest-config/src/Deprecated.js

THen just remove the old option from the code base

@ddruker
Copy link
Contributor Author

ddruker commented Oct 14, 2018

@SimenB What do you think about this suggestion by @rickhanlonii? Just want to make sure we're all on the same page about this.

https://github.com/facebook/jest/pull/7119/files#r224995890

@SimenB
Copy link
Member

SimenB commented Oct 14, 2018

I think we should just remove it, but I won't be difficult about it if people wants it to remain for compat (as long as we print a warning)

@mattphillips
Copy link
Contributor

I agree with just removing it, we could perhaps remove it and leave an error message when used that points to the new API docs

@SimenB
Copy link
Member

SimenB commented Oct 14, 2018

Yeah, printing the deprecation message is a must, it's widely used

@rickhanlonii
Copy link
Member

+1 removing it since the next release is a major - I think the confusion is that we were saying both "deprecate" (which would keep the option with a warning) and remove it (which would remove it with a message)

@SimenB
Copy link
Member

SimenB commented Oct 15, 2018

Yeah, good point. Let me rephrase 🙂

Basically, see how scriptPreprocessor is used throughout jest-config for how we want to handle it.

When setupTestFrameworkScriptFile is used:

  • print a warning
  • coerce it into setupTestsAfterJest (or whatever we call it), by wrapping it in an array
  • delete setupTestFrameworkScriptFile from the config

If both setupTestFrameworkScriptFile and setupTestsAfterJest is used, throw an error.

https://github.com/facebook/jest/blob/f00fbecb4b29b1f22582255d13591344a862b88e/packages/jest-config/src/Deprecated.js#L39-L54
https://github.com/facebook/jest/blob/3abfa18d705c3999820b9124b0ecb8be2f3f7be4/packages/jest-config/src/normalize.js#L208-L242

Don't worry too much about the name (setupTestsAfterJest or setupTestsAfterEnv etc), that's easy enough to do a search & replace for later.


As an aside, we can probably remove a few of the deprecations that's been there for multiple versions.

@bambielli
Copy link
Contributor

Hello all, I was directed here by @thymikee

#7174 I opened this issue yesterday, I think the documentation for setupFiles is somewhat misleading. Check the issue for more context.

I'm happy to open a PR if you think it's needed.

@SimenB
Copy link
Member

SimenB commented Oct 16, 2018

A PR clarifying the docs is very much welcome 🙂 Behavior-wise the only change in this PR is allowing an array instead of just a single for setupTestFrameworkFile, setupFiles is unchanged (except we might rename it). This PR will only benefit from clearer docs either way!

docs/Configuration.md Outdated Show resolved Hide resolved
@SimenB
Copy link
Member

SimenB commented Oct 20, 2018

LGTM! @thymikee @rickhanlonii mind taking a look?

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Code-wise looks good, I've left some comments and suggestions on the copy.

docs/Configuration.md Outdated Show resolved Hide resolved
packages/jest-cli/src/cli/args.js Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@ddruker
Copy link
Contributor Author

ddruker commented Oct 21, 2018

@thymikee Thanks for the great feedback. I made those copy changes to make the language more consistent throughout.

@codecov-io
Copy link

Codecov Report

Merging #7119 into master will increase coverage by 0.03%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #7119      +/-   ##
=========================================
+ Coverage   67.27%   67.3%   +0.03%     
=========================================
  Files         248     248              
  Lines        9652    9657       +5     
  Branches        4       3       -1     
=========================================
+ Hits         6493    6500       +7     
+ Misses       3158    3156       -2     
  Partials        1       1
Impacted Files Coverage Δ
packages/jest-config/src/ValidConfig.js 100% <ø> (ø) ⬆️
packages/jest-config/src/Defaults.js 100% <ø> (ø) ⬆️
packages/jest-config/src/index.js 19.35% <ø> (ø) ⬆️
packages/jest-jasmine2/src/index.js 0% <0%> (ø) ⬆️
...ircus/src/legacy_code_todo_rewrite/jest_adapter.js 0% <0%> (ø) ⬆️
packages/jest-config/src/normalize.js 85.77% <100%> (+0.35%) ⬆️
packages/jest-config/src/Deprecated.js 66.66% <100%> (+6.66%) ⬆️

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 ad5b040...cd8e2ae. Read the comment docs.

@SimenB SimenB merged commit 3f30a46 into jestjs:master Oct 21, 2018
@SimenB
Copy link
Member

SimenB commented Oct 21, 2018

What a fantastic first contribution to Jest @ddruker! Thank you 🎉

@kentcdodds
Copy link
Contributor

Woo! Thanks! This is a great change! Looks like I'll have to update a few videos on testingjavascript.com when this is published 😅 any rough idea about when the next major release could be?

@SimenB
Copy link
Member

SimenB commented Oct 21, 2018

We've published an alpha this week, so the ball has started rolling. The milestone still has quite a bit on it though: https://github.com/facebook/jest/milestone/8

A very tentative timeline would be December 1st, I'd say

@thymikee
Copy link
Collaborator

I'm pretty sure the deprecation message is descriptive enough not to make you (or anybody else) alter any tutorials 😉. We're also planning on simplifying the config sometime next year so stay tuned!

@ddruker
Copy link
Contributor Author

ddruker commented Oct 22, 2018 via email

@devinmorgan
Copy link

Hey there!

Wanted to know if this feature has been released yet?

I'm interested in using both jest-extended and enzyme but each one requires access to the setupTestFrameworkScriptFile field in the jest configuration and this seems to be the fix that I need.

Any clarification here would be great ❤.

@SimenB
Copy link
Member

SimenB commented Jan 21, 2019

It's available in jest@beta (currently 24.0.0-alpha.12).

However, if you're unable to upgrade, you can do this:

{
  "jest": {
    "setupTestFrameworkScriptFile": "<rootDir>/my-setup.js"
  }
}
// my-setup.js
import 'jest-extended';
import 'jest-enzyme';

NickyMeuleman added a commit to harshit-test-org/javascript-af that referenced this pull request Jan 29, 2019
arcticicestudio added a commit to nordtheme/web that referenced this pull request Feb 8, 2019
This is the regular batch update for outdated dependencies.

It includes the long awaited React version 16.8.0 (1) that finally
brings the stable Hooks API (2) as well as great performance
improvements and bug fixes!

Updated Jest's configuration since version `>=24.0.0` deprecated
`setupTestFrameworkScriptFile` in favor of the new
`setupFilesAfterEnv` (3) option.

The ESLint plugin `eslint-plugin-react` includes a lot of improvements
and bug fixes regarding the parsing of code structures for better prop
validation and display name detection. This resulted in the requirement
to implement currently missing prop types in various SFC and class
components in the project as well globally (temporally) disabling the
`react/display-name` rule to prevent warning message noise.

Gatsby and all plugins have been updated to the latest versions to
include the latest improvements and bug fixes. `gatsby-plugin manifest`
now sets the `legacy` option to `true` by default (4) to include Apple
touch icons since the assumption was wrong that iOS supports the icons
from the web manifest.

As of version 3.3 the "Inter UI" font typeface has been renamed and is
now "Inter" (5), without the "UI" part. This change has been ported to
the used inter-ui (6) package. The import has been adjusted to match the
renamed `inter.css` main file and all references to the "Inter UI" name
have been adjusted by removing "UI".

Prettier version 1.16.0 (7) comes with support for React Hooks and
features for TypeScript and HTML as well as many other improvements and
bug fixes.

React Pose includes an important fix that also animates all other
children when items are moving (8) when using the `PoseGroup` component.

>>>>>> Production Dependencies

- gatsby `2.0.75` -> `2.0.117`
- gatsby-plugin-canonical-urls `2.0.8` -> `2.0.10`
- gatsby-plugin-catch-links `2.0.9` -> `2.0.10`
- gatsby-plugin-google-gtag `1.0.8` -> `1.0.13`
- gatsby-plugin-manifest `2.0.12` -> `2.0.17`
- gatsby-plugin-netlify `2.0.6` -> `2.0.9`
- gatsby-plugin-no-sourcemaps `2.0.1` -> `2.0.2`
- gatsby-plugin-offline `2.0.20` -> `2.0.23`
- gatsby-plugin-react-helmet `3.0.5` -> `3.0.6`
- gatsby-plugin-remove-trailing-slashes `2.0.6` -> `2.0.7`
- gatsby-plugin-robots-txt `1.3.0` -> `1.4.0`
- gatsby-plugin-sitemap `2.0.3` -> `2.0.5`
- gatsby-plugin-styled-components `3.0.4` -> `3.0.5`
- gatsby-source-filesystem `2.0.12` -> `2.0.20`
- gatsby-transformer-yaml `2.1.6` -> `2.1.8`
- inter-ui `3.1.0` -> `3.3.2`
- polished `2.3.1` -> `2.3.3`
- react `16.7.0` -> `16.8.1`
- react-dom `16.7.0` -> `16.8.1`
- react-pose `4.0.4` -> `4.0.6`
- typeface-source-code-pro `0.0.54` -> `0.0.71`

>>>>>> Development Dependencies

- @babel/plugin-proposal-class-properties `7.2.3` -> `7.3.0`
- babel-jest `23.6.0` -> `24.1.0`
- babel-plugin-transform-react-remove-prop-types `0.4.21` -> `0.4.24`
- babel-preset-gatsby `0.1.6` -> `0.1.7`
- eslint `5.11.0` -> `5.13.0`
- eslint-plugin-import `2.14.0` -> `2.16.0`
- eslint-plugin-jsx-a11y `6.1.2` -> `6.2.1`
- eslint-plugin-prettier `3.0.0` -> `3.0.1`
- eslint-plugin-react `7.11.1` -> `7.12.4`
- husky `1.2.1` -> `1.3.1`
- jest `23.6.0` -> `24.1.0`
- jest-dom `3.0.0` -> `3.0.2`
- jest-junit `5.2.0` -> `6.2.1`
- lint-staged `8.1.0` -> `8.1.3`
- prettier `1.15.3` -> `1.16.4`
- react-testing-library `5.4.2` -> `5.5.3`

References:
  (1) https://reactjs.org/blog/2019/02/06/react-v16.8.0.html
  (2) https://reactjs.org/docs/hooks-intro.html
  (3) jestjs/jest#7119
  (4) gatsbyjs/gatsby#11203
  (5) https://github.com/rsms/inter/releases/tag/v3.3
  (6) https://www.npmjs.com/package/inter-ui
  (7) https://prettier.io/blog/2019/01/20/1.16.0.html
  (8) Popmotion/popmotion#682

Resolves GH-120
@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 12, 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.

Allow setupTestFrameworkScriptFile to accept an array (or add a new config option that does)