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

Change the way setting up the framework works #4976

Merged
merged 1 commit into from
Nov 29, 2017
Merged

Change the way setting up the framework works #4976

merged 1 commit into from
Nov 29, 2017

Conversation

mjesun
Copy link
Contributor

@mjesun mjesun commented Nov 28, 2017

This is an RFC to modify how we evaluate setupTestFrameworkScriptFile. Instead of being required and injected inside the testing framework, it will be evaluated in the global context, and the child context will be passed as a parameter.

I updated the PR to be a new option, setupJasmine, that will be run outside of the context, instead of inside.

@cpojer
Copy link
Member

cpojer commented Nov 28, 2017

I don't think we can change this option right now, this is a major change to Jest that would break tons of users. We could introduce a new one and deprecate this one in Jest 22 though.

@codecov-io
Copy link

codecov-io commented Nov 28, 2017

Codecov Report

Merging #4976 into master will increase coverage by 0.04%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4976      +/-   ##
==========================================
+ Coverage   60.21%   60.25%   +0.04%     
==========================================
  Files         198      198              
  Lines        6620     6623       +3     
  Branches        4        4              
==========================================
+ Hits         3986     3991       +5     
+ Misses       2634     2632       -2
Impacted Files Coverage Δ
packages/jest-config/src/valid_config.js 100% <ø> (ø) ⬆️
packages/jest-config/src/index.js 23.8% <ø> (ø) ⬆️
packages/jest-config/src/normalize.js 92.94% <ø> (ø) ⬆️
packages/jest-jasmine2/src/index.js 5.88% <0%> (-0.28%) ⬇️
packages/jest-validate/src/warnings.js 100% <0%> (+55.55%) ⬆️

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 9043a44...05701ee. Read the comment docs.

@mjesun
Copy link
Contributor Author

mjesun commented Nov 28, 2017

Ok, that's what I thought 🙂. I've added a new option, just for that use case.

@cpojer
Copy link
Member

cpojer commented Nov 28, 2017

setupJasmine doesn't work. How about setupTestFramework? Jest isn't tied to Jasmine.

@mjesun
Copy link
Contributor Author

mjesun commented Nov 28, 2017

What about setupGlobal?

@cpojer
Copy link
Member

cpojer commented Nov 28, 2017

The intention of this feature, different from setupFiles, is that it provides a hook to insert stuff once the test framework has been loaded. I'm not sure how setupGlobal makes sense in that context. setupTestFramework does though. I'm happy to hear more suggestions though.

Ideally you can also change it to be an array, just like setupFiles.

@mjesun
Copy link
Contributor Author

mjesun commented Nov 28, 2017

Ooh, I understand what you mean now. Yeah, my initial intention was to not use framework again, but I think your plan goes through phasing out setupTestFrameworkScriptFile; which makes sense. In that case I'll go ahead with setupTestFramework.

@mjesun
Copy link
Contributor Author

mjesun commented Nov 28, 2017

Updated; I made it an Array as you mentioned, and also renamed.

@mjesun mjesun changed the title RFC: change the way setting up the framework works Change the way setting up the framework works Nov 28, 2017
@cpojer cpojer merged commit dbfd787 into jestjs:master Nov 29, 2017
@cpojer
Copy link
Member

cpojer commented Nov 29, 2017

Alright, let's do it. I'm a bit uneasy about this hook as it breaks out of the sandboxing without being explicit but I guess it'll be fine.

@@ -127,7 +127,9 @@
"transform": {
"^.+\\.js$": "<rootDir>/packages/babel-jest"
},
"setupTestFrameworkScriptFile": "<rootDir>/test_setup_file.js",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Whoops, this should stay where it was. #4994

@@ -72,7 +72,7 @@ export default ({
runTestsByPath: false,
runner: 'jest-runner',
setupFiles: ['<rootDir>/setup.js'],
setupTestFrameworkScriptFile: '<rootDir>/test_setup_file.js',
setupTestFramework: ['<rootDir>/test_setup_file.js'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

@SimenB I meant this line, sorry!

@mjesun mjesun deleted the rfc-setup-framework branch December 4, 2017 11:36
mjesun added a commit that referenced this pull request Dec 4, 2017
cpojer pushed a commit that referenced this pull request Dec 6, 2017
@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.

5 participants