-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
jest-circus: support globals #6283
Conversation
@@ -50,10 +50,8 @@ export const makeTest = ( | |||
parent: DescribeBlock, | |||
timeout: ?number, | |||
): TestEntry => { | |||
let _mode; | |||
if (!fn) { | |||
_mode = 'skip'; // skip test if no fn passed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We throw if fn
is not defined
packages/jest-circus/src/utils.js
Outdated
@@ -272,3 +270,31 @@ export const invariant = (condition: *, message: string) => { | |||
throw new Error(message); | |||
} | |||
}; | |||
|
|||
// See: https://github.com/facebook/jest/pull/5154 | |||
function convertDescriptorToString(descriptor) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hate this, but compat 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a place we could put it in jest and share between both jasmine and circus?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could add this to jest-util
, not sure if we want it though (requires another dep for circus)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 1 dep > duplicated code 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm fine with both though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixd
10a10c7
to
0abbc50
Compare
packages/jest-circus/src/utils.js
Outdated
@@ -272,3 +270,31 @@ export const invariant = (condition: *, message: string) => { | |||
throw new Error(message); | |||
} | |||
}; | |||
|
|||
// See: https://github.com/facebook/jest/pull/5154 | |||
function convertDescriptorToString(descriptor) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a place we could put it in jest and share between both jasmine and circus?
@@ -35,7 +35,7 @@ const handler: EventHandler = (event, state): void => { | |||
} | |||
case 'finish_describe_definition': { | |||
const {currentDescribeBlock} = state; | |||
invariant(currentDescribeBlock, `currentDescribeBlock mest to be there`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what i was even trying to say :)
@@ -32,7 +32,6 @@ exports[`cannot test with no implementation 1`] = ` | |||
4 | test('test, no implementation'); | |||
5 | | |||
|
|||
at packages/jest-jasmine2/build/jasmine/Env.js:<<LINE>>:<<COLUMN>> | |||
at __tests__/only-constructs.test.js:<<LINE>>:<<COLUMN>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pretty sure we don't need the LINE
COLUMN
stuff now that we have proper source maps. not relevant to this PR, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed anyway, thanks
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. |
Summary
Part of #4362
Test plan
Globals are green