Add error throwing to 'it'/'test' for incorrect arguments. #5558
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #5558 +/- ##
=========================================
+ Coverage 62.35% 63.15% +0.8%
=========================================
Files 216 216
Lines 7908 7912 +4
Branches 4 3 -1
=========================================
+ Hits 4931 4997 +66
+ Misses 2976 2914 -62
Partials 1 1
Continue to review full report at Codecov.
|
SimenB
previously requested changes
Feb 14, 2018
| `Invalid first argument, ${testName}. It must be a string.`, | ||
| ); | ||
| } | ||
| if (fn === undefined) { |
There was a problem hiding this comment.
It is legal to not pass second argument, I use it as test placeholders.
Might be a a good idea to have an explicit test.todo for that case, though
| `Invalid first argument, ${description}. It must be a string.`, | ||
| ); | ||
| } | ||
| if (fn === undefined) { |
There was a problem hiding this comment.
Ok, cool. So to clarify: add an it.todo and test.todo to ignore cases like
test.todo('justOneArgument')
but still throw the error for test('justOneArgument')?
There was a problem hiding this comment.
I think "yes", but it's too breaking. But I think we should do it for the next version.
(I'm all for adding test/it.todo now - though probably in a separate PR)
@cpojer thoughts?
There was a problem hiding this comment.
I'm not sure about test.todo, let's discuss that separately. I'm definitely on board with making test/it with one argument throw. I do not consider it a breaking change but rather a bug fix, so it's fine to go into a minor.
SimenB
reviewed
Feb 14, 2018
|
|
||
| const {summary, rest} = extractSummary(stderr); | ||
| expect(rest).toMatchSnapshot(); | ||
| // expect(rest).toMatchSnapshot(); |
There was a problem hiding this comment.
I think this PR is pretty good to go other than that 🙂
There was a problem hiding this comment.
Ok, great. Let me make those changes and I'll push it up.
There was a problem hiding this comment.
@SimenB , that snapshot was commented out because it listed the absolute path to the file on my computer, which won't match when the CI is run. Any ideas on how to hide the path?
There was a problem hiding this comment.
Your path is messed up - it's a relative path to an absolute one...
SimenB
reviewed
Feb 15, 2018
|
|
||
| const {summary, rest} = extractSummary(stderr); | ||
| expect(rest).toMatchSnapshot(); | ||
| const restWithoutStack = rest.slice(0, 343); |
There was a problem hiding this comment.
Heh, seems somewhat arbitrary.
We should probably clean the stack up better instead of adding this hack
There was a problem hiding this comment.
Should I use formatStackTrace from jest-message-utils when creating the error?
There was a problem hiding this comment.
No, it's called automatically.
As it's only an issue for the integration tests (as they used linked files instead of inside node_module), I think we should be good with just stripping that single line in https://github.com/facebook/jest/blob/d065e87a2322e1b4b70e7ba9de3c24aed7a2ea72/integration-tests/Utils.js#L162
There was a problem hiding this comment.
@SimenB I'm struggling with this one bit. cleanupStackTrace doesn't replace the stacktrace for the Env, but does for everything else.
thymikee
reviewed
Feb 15, 2018
|
|
||
| 431 | } | ||
| 432 | if (fn === undefined) { | ||
| > 433 | throw new Error( |
There was a problem hiding this comment.
Ugh, any way we can disable code frame for this one, @SimenB?
There was a problem hiding this comment.
It points to the it in real code, not sure why it shows the throw here..
There was a problem hiding this comment.
the frame is probably unable to find __tests__/only-constructs.test.js for whatever reason - I'd debug that
There was a problem hiding this comment.
Ok, I’ll look in it. Thanks!
There was a problem hiding this comment.
It was pulling in the wrong frame because In jest-message-util
for (const line of lines) {
if (
line.includes(PATH_NODE_MODULES) ||
line.includes(PATH_EXPECT_BUILD)
) {
continue;
}
Wouldn't catch if the the top frame was from jest packages, so I added a condition:
const PATH_JEST_PACKAGES = `${path.sep}jest${path.sep}packages${path.sep}`;
that way it ignores the jest code as the topframe.
This was referenced May 28, 2018
Merged
calebeby
pushed a commit
to Pigmice2733/scouting-frontend
that referenced
this pull request
May 30, 2018
This Pull Request updates dependency [jest](https://github.com/facebook/jest) from `v22.4.3` to `v23.0.1` <details> <summary>Release Notes</summary> ### [`v23.0.1`](https://github.com/facebook/jest/blob/master/CHANGELOG.md#​2301) [Compare Source](jestjs/jest@v23.0.0...3a3b4e3) ##### Chore & Maintenance * `[jest-jasemine2]` Add dependency on jest-each ([#​6308](`jestjs/jest#6308)) * `[jest-each]` Move jest-each into core Jest ([#​6278](`jestjs/jest#6278)) * `[examples]` Update typescript example to using ts-jest ([#​6260](`jestjs/jest#6260)) ##### Fixes * `[pretty-format]` Serialize inverse asymmetric matchers correctly ([#​6272](`jestjs/jest#6272)) --- ### [`v23.0.0`](https://github.com/facebook/jest/blob/master/CHANGELOG.md#​2300) [Compare Source](jestjs/jest@2745e3e...v23.0.0) ##### Features * `[expect]` Expose `getObjectSubset`, `iterableEquality`, and `subsetEquality` ([#​6210](`jestjs/jest#6210)) * `[jest-snapshot]` Add snapshot property matchers ([#​6210](`jestjs/jest#6210)) * `[jest-config]` Support jest-preset.js files within Node modules ([#​6185](`jestjs/jest#6185)) * `[jest-cli]` Add `--detectOpenHandles` flag which enables Jest to potentially track down handles keeping it open after tests are complete. ([#​6130](`jestjs/jest#6130)) * `[jest-jasmine2]` Add data driven testing based on `jest-each` ([#​6102](`jestjs/jest#6102)) * `[jest-matcher-utils]` Change "suggest to equal" message to be more advisory ([#​6103](`jestjs/jest#6103)) * `[jest-message-util]` Don't ignore messages with `vendor` anymore ([#​6117](`jestjs/jest#6117)) * `[jest-validate]` Get rid of `jest-config` dependency ([#​6067](`jestjs/jest#6067)) * `[jest-validate]` Adds option to inject `deprecationEntries` ([#​6067](`jestjs/jest#6067)) * `[jest-snapshot]` [**BREAKING**] Concatenate name of test, optional snapshot name and count ([#​6015](`jestjs/jest#6015)) * `[jest-runtime]` Allow for transform plugins to skip the definition process method if createTransformer method was defined. ([#​5999](`jestjs/jest#5999)) * `[expect]` Add stack trace for async errors ([#​6008](`jestjs/jest#6008)) * `[jest-jasmine2]` Add stack trace for timeouts ([#​6008](`jestjs/jest#6008)) * `[jest-jasmine2]` Add stack trace for thrown non-`Error`s ([#​6008](`jestjs/jest#6008)) * `[jest-runtime]` Prevent modules from marking themselves as their own parent ([#​5235](`jestjs/jest#5235)) * `[jest-mock]` Add support for auto-mocking generator functions ([#​5983](`jestjs/jest#5983)) * `[expect]` Add support for async matchers ([#​5919](`jestjs/jest#5919)) * `[expect]` Suggest toContainEqual ([#​5948](`jestjs/jest#5953)) * `[jest-config]` Export Jest's default options ([#​5948](`jestjs/jest#5948)) * `[jest-editor-support]` Move `coverage` to `ProjectWorkspace.collectCoverage` ([#​5929](`jestjs/jest#5929)) * `[jest-editor-support]` Add `coverage` option to runner ([#​5836](`jestjs/jest#5836)) * `[jest-haste-map]` Support extracting dynamic `import`s ([#​5883](`jestjs/jest#5883)) * `[expect]` Improve output format for mismatchedArgs in mock/spy calls. ([#​5846](`jestjs/jest#5846)) * `[jest-cli]` Add support for using `--coverage` in combination with watch mode, `--onlyChanged`, `--findRelatedTests` and more ([#​5601](`jestjs/jest#5601)) * `[jest-jasmine2]` [**BREAKING**] Adds error throwing and descriptive errors to `it`/ `test` for invalid arguments. `[jest-circus]` Adds error throwing and descriptive errors to `it`/ `test` for invalid arguments ([#​5558](`jestjs/jest#5558)) * `[jest-matcher-utils]` Add `isNot` option to `matcherHint` function ([#​5512](`jestjs/jest#5512)) * `[jest-config]` Add `<rootDir>` to runtime files not found error report ([#​5693](`jestjs/jest#5693)) * `[expect]` Make toThrow matcher pass only if Error object is returned from promises ([#​5670](`jestjs/jest#5670)) * `[expect]` Add isError to utils ([#​5670](`jestjs/jest#5670)) * `[expect]` Add inverse matchers (`expect.not.arrayContaining`, etc., [#​5517](`jestjs/jest#5517)) * `[expect]` `expect.extend` now also extends asymmetric matchers ([#​5503](`jestjs/jest#5503)) * `[jest-mock]` Update `spyOnProperty` to support spying on the prototype chain ([#​5753](`jestjs/jest#5753)) * `[jest-mock]` Add tracking of return values in the `mock` property ([#​5752](`jestjs/jest#5752)) * `[jest-mock]` Add tracking of thrown errors in the `mock` property ([#​5764](`jestjs/jest#5764)) * `[expect]`Add nthCalledWith spy matcher ([#​5605](`jestjs/jest#5605)) * `[jest-cli]` Add `isSerial` property that runners can expose to specify that they can not run in parallel ([#​5706](`jestjs/jest#5706)) * `[expect]` Add `.toBeCalledTimes` and `toHaveBeenNthCalledWith` aliases ([#​5826](`jestjs/jest#5826)) * `[jest-cli]` Interactive Snapshot Mode improvements ([#​5864](`jestjs/jest#5864)) * `[jest-editor-support]` Add `no-color` option to runner ([#​5909](`jestjs/jest#5909)) * `[jest-jasmine2]` Pretty-print non-Error object errors ([#​5980](`jestjs/jest#5980)) * `[jest-message-util]` Include column in stack frames ([#​5889](`jestjs/jest#5889)) * `[expect]` Introduce toStrictEqual ([#​6032](`jestjs/jest#6032)) * `[expect]` Add return matchers ([#​5879](`jestjs/jest#5879)) * `[jest-cli]` Improve snapshot summaries ([#​6181](`jestjs/jest#6181)) * `[expect]` Include custom mock names in error messages ([#​6199](`jestjs/jest#6199)) * `[jest-diff]` Support returning diff from oneline strings ([#​6221](`jestjs/jest#6221)) * `[expect]` Improve return matchers ([#​6172](`jestjs/jest#6172)) * `[jest-cli]` Overhaul watch plugin hooks names ([#​6249](`jestjs/jest#6249)) * `[jest-mock]` Include tracked call results in serialized mock ([#​6244](`jestjs/jest#6244)) ##### Fixes * `[jest-cli]` Fix stdin encoding to utf8 for watch plugins. ([#​6253](`jestjs/jest#6253)) * `[expect]` Better detection of DOM Nodes for equality ([#​6246](`jestjs/jest#6246)) * `[jest-cli]` Fix misleading action description for F key when in "only failed tests" mode. ([#​6167](`jestjs/jest#6167)) * `[jest-worker]` Stick calls to workers before processing them ([#​6073](`jestjs/jest#6073)) * `[babel-plugin-jest-hoist]` Allow using `console` global variable ([#​6075](`jestjs/jest#6075)) * `[jest-jasmine2]` Always remove node core message from assert stack traces ([#​6055](`jestjs/jest#6055)) * `[expect]` Add stack trace when `expect.assertions` and `expect.hasAssertions` causes test failures. ([#​5997](`jestjs/jest#5997)) * `[jest-runtime]` Throw a more useful error when trying to require modules after the test environment is torn down ([#​5888](`jestjs/jest#5888)) * `[jest-mock]` [**BREAKING**] Replace timestamps with `invocationCallOrder` ([#​5867](`jestjs/jest#5867)) * `[jest-jasmine2]` Install `sourcemap-support` into normal runtime to catch runtime errors ([#​5945](`jestjs/jest#5945)) * `[jest-jasmine2]` Added assertion error handling inside `afterAll hook` ([#​5884](`jestjs/jest#5884)) * `[jest-cli]` Remove the notifier actions in case of failure when not in watch mode. ([#​5861](`jestjs/jest#5861)) * `[jest-mock]` Extend .toHaveBeenCalled return message with outcome ([#​5951](`jestjs/jest#5951)) * `[jest-runner]` Assign `process.env.JEST_WORKER_ID="1"` when in runInBand mode ([#​5860](`jestjs/jest#5860)) * `[jest-cli]` Add descriptive error message when trying to use `globalSetup`/`globalTeardown` file that doesn't export a function. ([#​5835](`jestjs/jest#5835)) * `[expect]` Do not rely on `instanceof RegExp`, since it will not work for RegExps created inside of a different VM ([#​5729](`jestjs/jest#5729)) * `[jest-resolve]` Update node module resolution algorithm to correctly handle symlinked paths ([#​5085](`jestjs/jest#5085)) * `[jest-editor-support]` Update `Settings` to use spawn in shell option ([#​5658](`jestjs/jest#5658)) * `[jest-cli]` Improve the error message when 2 projects resolve to the same config ([#​5674](`jestjs/jest#5674)) * `[jest-runtime]` remove retainLines from coverage instrumentation ([#​5692](`jestjs/jest#5692)) * `[jest-cli]` Fix update snapshot issue when using watchAll ([#​5696](`jestjs/jest#5696)) * `[expect]` Fix rejects.not matcher ([#​5670](`jestjs/jest#5670)) * `[jest-runtime]` Prevent Babel warnings on large files ([#​5702](`jestjs/jest#5702)) * `[jest-mock]` Prevent `mockRejectedValue` from causing unhandled rejection ([#​5720](`jestjs/jest#5720)) * `[pretty-format]` Handle React fragments better ([#​5816](`jestjs/jest#5816)) * `[pretty-format]` Handle formatting of `React.forwardRef` and `Context` components ([#​6093](`jestjs/jest#6093)) * `[jest-cli]` Switch collectCoverageFrom back to a string ([#​5914](`jestjs/jest#5914)) * `[jest-regex-util]` Fix handling regex symbols in tests path on Windows ([#​5941](`jestjs/jest#5941)) * `[jest-util]` Fix handling of NaN/Infinity in mock timer delay ([#​5966](`jestjs/jest#5966)) * `[jest-resolve]` Generalise test for package main entries equivalent to ".". ([#​5968](`jestjs/jest#5968)) * `[jest-config]` Ensure that custom resolvers are used when resolving the configuration ([#​5976](`jestjs/jest#5976)) * `[website]` Fix website docs ([#​5853](`jestjs/jest#5853)) * `[expect]` Fix isEqual Set and Map to compare object values and keys regardless of order ([#​6150](`jestjs/jest#6150)) * `[pretty-format]` [**BREAKING**] Remove undefined props from React elements ([#​6162](`jestjs/jest#6162)) * `[jest-haste-map]` Properly resolve mocked node modules without package.json defined ([#​6232](`jestjs/jest#6232)) ##### Chore & Maintenance * `[jest-runner]` Move sourcemap installation from `jest-jasmine2` to `jest-runner` ([#​6176](`jestjs/jest#6176)) * `[jest-cli]` Use yargs's built-in `version` instead of rolling our own ([#​6215](`jestjs/jest#6215)) * `[docs]` Add explanation on how to mock methods not implemented in JSDOM * `[jest-jasmine2]` Simplify `Env.execute` and TreeProcessor to setup and clean resources for the top suite the same way as for all of the children suites ([#​5885](`jestjs/jest#5885)) * `[babel-jest]` [**BREAKING**] Always return object from transformer ([#​5991](`jestjs/jest#5991)) * `[*]` Run Prettier on compiled output ([#​5858](`jestjs/jest#3497)) * `[jest-cli]` Add fileChange hook for plugins ([#​5708](`jestjs/jest#5708)) * `[docs]` Add docs on using `jest.mock(...)` ([#​5648](`jestjs/jest#5648)) * `[docs]` Mention Jest Puppeteer Preset ([#​5722](`jestjs/jest#5722)) * `[docs]` Add jest-community section to website ([#​5675](`jestjs/jest#5675)) * `[docs]` Add versioned docs for v22.4 ([#​5733](`jestjs/jest#5733)) * `[docs]` Improve Snapshot Testing Guide ([#​5812](`jestjs/jest#5812)) * `[jest-runtime]` [**BREAKING**] Remove `jest.genMockFn` and `jest.genMockFunction` ([#​6173](`jestjs/jest#6173)) * `[jest-message-util]` Avoid adding unnecessary indent to blank lines in stack traces ([#​6211](`jestjs/jest#6211)) --- ### [`v22.4.4`](jestjs/jest@6851d8b...v22.4.4) [Compare Source](jestjs/jest@6851d8b...v22.4.4) --- </details> --- This PR has been generated by [Renovate Bot](https://renovatebot.com).
|
Did anyone realize that this broke the intent behind #1775? Even |
Why not implement it in a non-breaking way instead? #5493 doesn't require checking the second argument, only the first. |
pk-nb
added a commit
to pk-nb/jest
that referenced
this pull request
Jul 2, 2018
- Continues to validate arguments to avoid accidental skips from jestjs#5558
pk-nb
added a commit
to pk-nb/jest
that referenced
this pull request
Jul 2, 2018
- Continues to validate arguments to avoid accidental skips from jestjs#5558
pk-nb
added a commit
to pk-nb/jest
that referenced
this pull request
Jul 2, 2018
- Continues to validate arguments to avoid accidental skips from jestjs#5558
pk-nb
added a commit
to pk-nb/jest
that referenced
this pull request
Jul 12, 2018
- Continues to validate arguments to avoid accidental skips from jestjs#5558
hzoo
pushed a commit
to babel/babel
that referenced
this pull request
Jul 19, 2018
updated `jest` and `babel-jest` to `v23` fixed breaking change in `jest`: - https://github.com/facebook/jest/blob/master/CHANGELOG.md#2300 - jestjs/jest#5558 _edit:_ forgot to mention. test runner fix is based on: https://github.com/babel/babel/blob/master/packages/babel-parser/test/helpers/runFixtureTests.js#L11
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Addresses #5493. Previously, missing or invalid arguments in 'it' and 'test' would cause the tests to be skipped. This PR adds error throwing in both jest-jasmine2 and jest-circus, ensuring that when passing arguments:
Test plan
I linked the development build of the feature branch to another project and ran tests there to make sure it passed external tests, as well as the internal tests.
It works correctly with valid arguments.
It throws these magnificent errors with invalid arguments.
For the internal tests, jest-jasmine2, would throw an error for the correct use cases, because of its rule about nested tests. Go figure, testing tests is tricky.
I test jest-circus's 'it' with jest-jasmine2's 'it'. I needed to differentiate between the two methods, but since the jest-circus methods are module.exports I couldn't use imports' 'as' keyword to alias jest-circus's 'it', so i made an alias maker function, requiring in jest-circus's it and renaming it circusIt.
Also, had to add jest-circus's test file to the flowconfig ignore otherwise it wouldn't allow for invalid arguments. Flow, you are too good sometimes!
Note: the /integration-tests/tests/globals.test.js has two tests expecting an unimplemented test/it to return a status 0, which it can't anymore because of the nature of this feature. To get this up to speed I changed the names of these tests to start with "cannot test...", changed the expected status to 1, and since part of these tests requires the ``rest` snapshot, which lists the absolute path to the file starting with the root of my computer, I cleaned up the path in Utils, removing the non-universal parts of the path.