-
-
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
Add error throwing to 'it'/'test' for incorrect arguments. #5558
Changes from 102 commits
6e41bcd
b551940
1de43af
f4a012e
a84007f
85587bd
325fe11
363e590
bf07249
918ad6d
d2a3668
b489b37
a873a75
119130c
1d6e12c
29a0f0a
d8eac51
e09303f
5106800
84d9a9b
7815602
0728d30
b10ad04
1ba971c
19d8d88
af8efd7
30ccac2
1842a5d
17d3050
754bf38
5464098
42ef481
fdc0dc3
e986609
a6ab7b0
5453e9f
1520b94
193cdf6
b2ac1c5
5ad1768
0312138
ca714e7
1a4dd3a
a1ea320
ef31915
5e0aabd
30aa554
735bbc2
9514c32
edc9231
d5aa613
0a560ed
911c6e1
07a51fb
281100c
ca02d0a
6e351e7
7eca76f
f85f956
e64357e
f2359e0
4aa3101
139973b
c0c478e
ba03582
061dab3
7195e05
e411607
5163ee3
e0ff615
29a3d64
85f359e
426cb07
895f57e
e6f8d6a
4547c55
5c1d541
dd36839
873671a
b36f4fb
07154b3
2b664de
17d2f77
b867548
b27c0b5
58e973b
f1df10b
7648643
c612bca
feaaf58
7378e67
6e2d789
bf4e287
ba0c286
8e746b4
f92e725
7ab85b1
c1753b3
b5985e1
e03c1a7
06875a8
661496b
f3f1213
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -159,7 +159,9 @@ const extractSummary = (stdout: string) => { | |
// unifies their output to make it possible to snapshot them. | ||
// TODO: Remove when we drop support for node 4 | ||
const cleanupStackTrace = (output: string) => { | ||
return output.replace(/^.*at.*[\s][\(]?(\S*\:\d*\:\d*).*$/gm, ' at $1'); | ||
return output | ||
.replace(/.*(?=packages)/g, ' at ') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @SimenB , that's to truncate the overly long relative path that jest-message-util generates between the
and like this without it:
It's needed for snapshots in the CI tests, so it can pass on any computer. I can add a comment in there explaining if you think it warrants it? Or try a different approach if you'd rather. |
||
.replace(/^.*at.*[\s][\(]?(\S*\:\d*\:\d*).*$/gm, ' at $1'); | ||
}; | ||
|
||
module.exports = { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
/** | ||
* Copyright (c) 2015-present, Facebook, Inc. All rights reserved. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
* | ||
* *@flow | ||
*/ | ||
|
||
'use strict'; | ||
|
||
let circusIt; | ||
let circusTest; | ||
|
||
//using jest-jasmine2's 'it' to test jest-circus's 'it'. Had to differentiate | ||
//the two with this aliaser. | ||
|
||
const aliasCircusIt = () => { | ||
const {it, test} = require('../index.js'); | ||
circusIt = it; | ||
circusTest = test; | ||
}; | ||
|
||
aliasCircusIt(); | ||
|
||
//A few of these tests require incorrect types to throw errors and thus pass | ||
//the test. The typechecks on jest-circus would prevent that, so | ||
//this file has been listed in the .flowconfig ignore section. | ||
|
||
describe('test/it error throwing', () => { | ||
it(`it doesn't throw an error with valid arguments`, () => { | ||
expect(() => { | ||
circusIt('test1', () => {}); | ||
}).not.toThrowError(); | ||
}); | ||
it(`it throws error with missing callback function`, () => { | ||
expect(() => { | ||
circusIt('test2'); | ||
}).toThrowError('Missing second argument. It must be a callback function.'); | ||
}); | ||
it(`it throws an error when first argument isn't a string`, () => { | ||
expect(() => { | ||
circusIt(() => {}); | ||
}).toThrowError(`Invalid first argument, () => {}. It must be a string.`); | ||
}); | ||
it('it throws an error when callback function is not a function', () => { | ||
expect(() => { | ||
circusIt('test4', 'test4b'); | ||
}).toThrowError( | ||
`Invalid second argument, test4b. It must be a callback function.`, | ||
); | ||
}); | ||
it(`test doesn't throw an error with valid arguments`, () => { | ||
expect(() => { | ||
circusTest('test5', () => {}); | ||
}).not.toThrowError(); | ||
}); | ||
it(`test throws error with missing callback function`, () => { | ||
expect(() => { | ||
circusTest('test6'); | ||
}).toThrowError('Missing second argument. It must be a callback function.'); | ||
}); | ||
it(`test throws an error when first argument isn't a string`, () => { | ||
expect(() => { | ||
circusTest(() => {}); | ||
}).toThrowError(`Invalid first argument, () => {}. It must be a string.`); | ||
}); | ||
it('test throws an error when callback function is not a function', () => { | ||
expect(() => { | ||
circusTest('test8', 'test8b'); | ||
}).toThrowError( | ||
`Invalid second argument, test8b. It must be a callback function.`, | ||
); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,8 +38,22 @@ const beforeAll = (fn: HookFn) => _addHook(fn, 'beforeAll'); | |
const afterEach = (fn: HookFn) => _addHook(fn, 'afterEach'); | ||
const afterAll = (fn: HookFn) => _addHook(fn, 'afterAll'); | ||
|
||
const test = (testName: TestName, fn?: TestFn) => | ||
dispatch({fn, name: 'add_test', testName}); | ||
const test = (testName: TestName, fn: TestFn) => { | ||
if (typeof testName !== 'string') { | ||
throw new Error( | ||
`Invalid first argument, ${testName}. It must be a string.`, | ||
); | ||
} | ||
if (fn === undefined) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is legal to not pass second argument, I use it as test placeholders. Might be a a good idea to have an explicit |
||
throw new Error('Missing second argument. It must be a callback function.'); | ||
} | ||
if (typeof fn !== 'function') { | ||
throw new Error( | ||
`Invalid second argument, ${fn}. It must be a callback function.`, | ||
); | ||
} | ||
return dispatch({fn, name: 'add_test', testName}); | ||
}; | ||
const it = test; | ||
test.skip = (testName: TestName, fn?: TestFn) => | ||
dispatch({fn, mode: 'skip', name: 'add_test', testName}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
/** | ||
* Copyright (c) 2015-present, Facebook, Inc. All rights reserved. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
* | ||
*/ | ||
|
||
'use strict'; | ||
|
||
describe('test/it error throwing', () => { | ||
it(`it throws error with missing callback function`, () => { | ||
expect(() => { | ||
it('test1'); | ||
}).toThrowError('Missing second argument. It must be a callback function.'); | ||
}); | ||
it(`it throws an error when first argument isn't a string`, () => { | ||
expect(() => { | ||
it(() => {}); | ||
}).toThrowError(`Invalid first argument, () => {}. It must be a string.`); | ||
}); | ||
it('it throws an error when callback function is not a function', () => { | ||
expect(() => { | ||
it('test3', 'test3b'); | ||
}).toThrowError( | ||
`Invalid second argument, test3b. It must be a callback function.`, | ||
); | ||
}); | ||
test(`test throws error with missing callback function`, () => { | ||
expect(() => { | ||
test('test4'); | ||
}).toThrowError('Missing second argument. It must be a callback function.'); | ||
}); | ||
test(`test throws an error when first argument isn't a string`, () => { | ||
expect(() => { | ||
test(() => {}); | ||
}).toThrowError(`Invalid first argument, () => {}. It must be a string.`); | ||
}); | ||
test('test throws an error when callback function is not a function', () => { | ||
expect(() => { | ||
test('test6', 'test6b'); | ||
}).toThrowError( | ||
`Invalid second argument, test6b. It must be a callback function.`, | ||
); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -424,6 +424,21 @@ export default function(j$) { | |
}; | ||
|
||
this.it = function(description, fn, timeout) { | ||
if (typeof description !== 'string') { | ||
throw new Error( | ||
`Invalid first argument, ${description}. It must be a string.`, | ||
); | ||
} | ||
if (fn === undefined) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, cool. So to clarify: add an
but still throw the error for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think "yes", but it's too breaking. But I think we should do it for the next version. (I'm all for adding @cpojer thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fair enough 🙂 |
||
throw new Error( | ||
'Missing second argument. It must be a callback function.', | ||
); | ||
} | ||
if (typeof fn !== 'function') { | ||
throw new Error( | ||
`Invalid second argument, ${fn}. It must be a callback function.`, | ||
); | ||
} | ||
const spec = specFactory( | ||
description, | ||
fn, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,7 +45,7 @@ type StackTraceOptions = { | |
}; | ||
|
||
const PATH_NODE_MODULES = `${path.sep}node_modules${path.sep}`; | ||
const PATH_EXPECT_BUILD = `${path.sep}expect${path.sep}build${path.sep}`; | ||
const PATH_JEST_PACKAGES = `${path.sep}jest${path.sep}packages${path.sep}`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this will make actual errors in the jest code base harder to track down when developing. If that's ok, we should delete line 48 as well, as it's covered by this (ref https://github.com/facebook/jest/pull/5094/files#r157363023) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. Let me know if I have the all clear to remove 48 & 49. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If tests are still green, it'd be great to remove both. But 49 should cover what 48 does, so only 49 is needed |
||
|
||
// filter for noisy stack trace lines | ||
const JASMINE_IGNORE = /^\s+at(?:(?:.*?vendor\/|jasmine\-)|\s+jasmine\.buildExpectationResult)/; | ||
|
@@ -220,7 +220,7 @@ const formatPaths = ( | |
|
||
const getTopFrame = (lines: string[]) => { | ||
for (const line of lines) { | ||
if (line.includes(PATH_NODE_MODULES) || line.includes(PATH_EXPECT_BUILD)) { | ||
if (line.includes(PATH_NODE_MODULES) || line.includes(PATH_JEST_PACKAGES)) { | ||
continue; | ||
} | ||
|
||
|
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.
Wait, this is not supposed to be here
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.
@thymikee it's so the jest-circus tests can check the error for a missing or incorrect argument. If Flow is on for this file it won't run the test. I'm happy to go about this in another manner, if you'd rather.
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.
Just remove the
@flow
pragma from circus test. Let's keep this config as clean as possibleThere 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.
@thymikee
The lint config throws an error during yarn test. Can I remove this line? Or add the test file to the lint ignore? Or an exception to the rule?
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.
Nope. The problem with this and previous approach is that it's easy to forget we have this "exception", which may lead to falsy feel of confidence about our codebase. I've pushed a commit with a proper, in my opinion, change for this case 🙂
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.
Awesome! $FlowFixMe: is something I'll have to remember. Thanks!