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

Jest suppresses errors in describe callback when only running specific tests #8334

Closed
G-Rath opened this issue Apr 15, 2019 · 10 comments
Closed

Comments

@G-Rath
Copy link
Contributor

G-Rath commented Apr 15, 2019

🐛 Bug Report

If you run a specific test (such as using --testNamePattern - there could be other ways, but this is how I found the bug), jest will eat any errors thrown outside of that test, skipping the test and sometimes complaining about a describe callback not returning a value.

I did do a very minor dive into this a while ago, before I determined the cause (as when I first started jest I was getting these strange "a describe callback must not return a value", despite never returning a value) - somewhere internally something returns null instead of undefined, which jest doesn't know it's doing and hence complains to the developer.

To Reproduce

  1. Write a test file that errors outside of an test/it block, that is inside at least one describe block
  2. Run the test specifically using a parameter like --testNamePatten

Expected behavior

Jest tells me that the error happened, in the same manner it does when I run the describe block.

Jest also shouldn't complain about a describe callback shouldn't have a return value (which it does even if I'm running the describe block - let me know if you'd like me to make a separate issue for this)

Link to repl or repo (highly encouraged)

Here is an example repo.

Do npm run showcase to see the bug in action.

Heres what it outputs on my side:

PS C:\Users\G-Rath\workspace\projects-personal\js-bugs>  jest "--testNamePattern=^test$" --runTestsByPath test/test.spec.js
  console.log node_modules/jest-jasmine2/build/jasmine/Env.js:520
      ● Test suite failed to run

        A "describe" callback must not return a value.
        Returning a value from "describe" will fail the test in a future version of Jest.

        > 1 | describe('test', () => {
            |                        ^
          2 |   throw new Error();
          3 | 
          4 |   it('showcases the bug', () => {

          at addSpecsToSuite (node_modules/jest-jasmine2/build/jasmine/Env.js:522:17)
          at Object.<anonymous> (test/test.spec.js:1:24)


Test Suites: 1 skipped, 0 of 1 total
Tests:       1 skipped, 1 total
Snapshots:   0 total
Time:        2.03s
Ran all test suites within paths "test/test.spec.js".

Run npx envinfo --preset jest

Paste the results here:

  System:
    OS: Windows 10
    CPU: (8) x64 Intel(R) Core(TM) i7-6700HQ CPU @ 2.60GHz
  Binaries:
    Node: 10.15.3 - C:\nodejs\node.EXE
    Yarn: 1.13.0 - ~\AppData\Roaming\npm\yarn.CMD
    npm: 6.9.0 - ~\AppData\Roaming\npm\npm.CMD
@jeysal
Copy link
Contributor

jeysal commented Apr 15, 2019

Confirmed. The describe warning was a code path I didn't consider when implementing that feature, I'll make a fix for it.
The error being swallowed is unrelated, appears to be because the test case being searched is never created due to the error and then the whole suite is marked as skipped. Not very nice I guess but perhaps not a top priority fix since it's jest-jasmine2 - jest-circus behaves properly.

@G-Rath
Copy link
Contributor Author

G-Rath commented Apr 15, 2019

Confirmed. The describe warning was a code path I didn't consider when implementing that feature, I'll make a fix for it.

Maybe you could somehow utilise that?

This is the only way I've gotten this behavior (i.e outside of explicitly returning), so maybe you could replace null with a canary (such as a magic value, or boolean property/variable) to output a warning on the lines of "Hey we've not returned properly - this usually happens when there was an error before creating test cases".

Not very nice I guess but perhaps not a top priority fix since it's jest-jasmine2 - jest-circus behaves properly.

Fair enough - is there anything else I should do to make this more actionable, such as opening an issue for jest-jasmine2?

@jeysal
Copy link
Contributor

jeysal commented Apr 15, 2019

The describe warning thing is a small fix, we can keep this issue open for the suppressed I guess

@jeysal
Copy link
Contributor

jeysal commented Apr 15, 2019

#8335 for the describe warning part

@jeysal jeysal changed the title Jest suppresses errors & complains about describe callback returning a value when only running specific tests Jest suppresses errors in describe callback when only running specific tests Apr 15, 2019
@jeysal
Copy link
Contributor

jeysal commented Apr 15, 2019

Fair enough - is there anything else I should do to make this more actionable, such as opening an issue for jest-jasmine2?

jest-circus will supersede jest-jasmine2 as the default test framework implementation soon, so that will solve it for most people anyway.
It might not be trivial to fix the suppression in jest-jasmine2 so possibly not worth it for something that's obsolete soon.
You can opt in to jest-circus now if you want to try it out BTW :)

@G-Rath
Copy link
Contributor Author

G-Rath commented Apr 15, 2019

Can confirm not an issue using jest-circus:

  ● Test suite failed to run

    Error

      3 | 
      4 | describe('test', () => {
    > 5 |   throw new Error('');
        |         ^
      6 | 
      7 |   it('geolocation', () => {
      8 |     expect(1).toHaveBeenCalledTimes(1);

      at Object.<anonymous>.describe (test/src/leafs/location/BrowserLocationLeaf.spec.ts:5:9)
      at Object.<anonymous> (test/src/leafs/location/BrowserLocationLeaf.spec.ts:4:1)

  ● Test suite failed to run

    Cannot spyOn on a primitive value; undefined given

      3 | 
      4 | describe('test', () => {
    > 5 |   const spy = jest.spyOn(window.navigator.geolocation, 'getCurrentPosition');
        |                    ^
      6 | 
      7 |   it('geolocation', () => {
      8 |     expect(spy).toHaveBeenCalledTimes(1);

      at ModuleMockerClass.spyOn (node_modules/jest-mock/build/index.js:832:13)
      at Object.<anonymous>.describe (test/src/leafs/location/BrowserLocationLeaf.spec.ts:5:20)
      at Object.<anonymous> (test/src/leafs/location/BrowserLocationLeaf.spec.ts:4:1)

Is there any documentation on the differences between jest-circus & jest-jasmine2? I've heard that jasmine2 has its issues, but am really curious on what exactly the differences are/will-be between the two (if any?).

I'm sure this'll probably be provided in a blog post when it's finally released so feel free to tell me to shush, but I figure might as well ask while I've got your eyes on this issue, in case there is a nice md somewhere 😜

@jeysal
Copy link
Contributor

jeysal commented Apr 16, 2019

circus is designed to be mostly compatible with jasmine2 except for maybe disallowing a few bad patterns. Jest does not make significant breaking changes. It's mostly about cleanup / more maintainable code and cool stuff like extensibility

jeysal added a commit that referenced this issue Apr 16, 2019
…ibe throws (#8335)

<!-- Thanks for submitting a pull request! Please provide enough information so that others can review your pull request. The two fields below are mandatory. -->

<!-- Please remember to update CHANGELOG.md in the root of the project if you have not done so. -->

## Summary

Fixes the describe warning part of #8334 (intentionally not marking this as closing the whole issue)

<!-- Explain the **motivation** for making this change. What existing problem does the pull request solve? -->

## Test plan

Added e2e test case that fails with master build (the stdout `toBe('')` check).
<!-- Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes UI. -->
@github-actions
Copy link

This issue is stale because it has been open for 1 year with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the Stale label Feb 25, 2022
@github-actions
Copy link

This issue was closed because it has been stalled for 7 days with no activity. Please open a new issue if the issue is still relevant, linking to this one.

@github-actions
Copy link

This issue 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 Apr 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants