Add error throwing to 'it'/'test' for incorrect arguments.

[brianlmacdonald]

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:

• The first argument is a string.
• There is a second argument.
• That the second argument is a function.

Test plan

const {sum} = require('./sums.js')
//correct
it('adds', ()  => {
  expect(sum(2, 3)).toBe(5)
})
//incorrect
// first argument must be a string!
it(() => {
  expect(sum(2, 3,)).toBe(5)
})
//missing second argument!
it('adds')
//second argument must be a function!
it('adds', 'not a function')

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.

[codecov-io]

Codecov Report

Merging #5558 into master will increase coverage by 0.8%.
The diff coverage is 62.5%.

@@            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

Impacted Files	Coverage Δ
packages/jest-jasmine2/src/jasmine/Env.js	0% <0%> (ø)	⬆️
packages/jest-message-util/src/index.js	81.1% <100%> (ø)	⬆️
packages/jest-circus/src/index.js	70% <100%> (+70%)	⬆️
...kages/jest-circus/src/format_node_assert_errors.js	19.44% <0%> (+19.44%)	⬆️
packages/jest-circus/src/utils.js	19.76% <0%> (+19.76%)	⬆️
packages/jest-circus/src/event_handler.js	23.07% <0%> (+23.07%)	⬆️
packages/jest-circus/src/state.js	92.3% <0%> (+92.3%)	⬆️

---

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 196d9b8...f3f1213. Read the comment docs.

[SimenB]

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

[SimenB]

Same here

[brianlmacdonald]

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')?

[SimenB]

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?

[cpojer]

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]

fair enough 🙂

[SimenB]

why are these commented out?

[SimenB]

I think this PR is pretty good to go other than that 🙂

[brianlmacdonald]

Ok, great. Let me make those changes and I'll push it up.

[brianlmacdonald]

@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?

[SimenB]

Your path is messed up - it's a relative path to an absolute one...

invalid

[SimenB]

Heh, seems somewhat arbitrary.

We should probably clean the stack up better instead of adding this hack

[brianlmacdonald]

Should I use formatStackTrace from jest-message-utils when creating the error?

[SimenB]

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

[brianlmacdonald]

@SimenB I'm struggling with this one bit. cleanupStackTrace doesn't replace the stacktrace for the Env, but does for everything else.

[thymikee]

Ugh, any way we can disable code frame for this one, @SimenB?

[SimenB]

It points to the it in real code, not sure why it shows the throw here..

[SimenB]

the frame is probably unable to find __tests__/only-constructs.test.js for whatever reason - I'd debug that

[brianlmacdonald]

Ok, I’ll look in it. Thanks!

[brianlmacdonald]

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.

[shellscape]

Did anyone realize that this broke the intent behind #1775? Even test.skip now produces this error. It seems awfully needless that an intentionally skipped test (as there doesn't appear to be a test.todo equivalent) requires a noop function to pass validation.

[elyobo]

@evocateur yeah, it should really be called out as breaking in the changelog. Just updated it: 02f4166

Why not implement it in a non-breaking way instead? #5493 doesn't require checking the second argument, only the first.

[github-actions]

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.