Allowed named classes and functions as BlockNames

[JoshuaKGoldberg]

Summary

describe can now take in classes and functions as the names. See #5152

Test plan

I'm curious about this actually - where would you like me to add tests? It wasn't very clear scanning through the code.

(first Jest PR; still getting used to Flow... expect failures the first few commits!)

[JoshuaKGoldberg]

If something silly, such as a number or { toString() { return "nope"; } is passed here, it'll crash. How would you like this function to deal with those error handling cases?

[cpojer]

Can you just make the function throw in that case and ask people to provide a string, class or function instead?

[codecov-io]

Codecov Report

Merging #5154 into master will decrease coverage by 0.1%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #5154      +/-   ##
==========================================
- Coverage   60.65%   60.54%   -0.11%     
==========================================
  Files         201      201              
  Lines        6695     6707      +12     
  Branches        3        3              
==========================================
  Hits         4061     4061              
- Misses       2633     2645      +12     
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-jasmine2/src/jasmine/Suite.js	0% <0%> (ø)	⬆️

---

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 23b7e98...e547ef4. Read the comment docs.

[SimenB]

Thanks for the PR!

I'm curios, is this needed? Why can't you do describe(Adder.toString(), () => {})?

[cpojer]

I agree with @SimenB. You can get all the benefits of easy refactoring by doing this manually without having to change Jest.

[JoshuaKGoldberg]

Adder.toString() does not actually work well - it fills up the terminal with the full contents of the class/function.

Using create-react-app's default setup, and adding a describe(Adder or describe(Adder.toString() to App.test.js, we get the lovely:

 PASS  src\App.test.js
  class App extends _react.Component {
  render() {
    return (
      _react2.default.createElement('div', { className: 'App', __source: { fileName: _jsxFileName, lineNumber: 8 }, __self: this },
        _react2.default.createElement('header', { className: 'App-header', __source: { fileName: _jsxFileName, lineNumber: 9 }, __self: this },
          _react2.default.createElement('img', { src: _logo2.default, className: 'App-logo', alt: 'logo', __source: { fileName: _jsxFileName, lineNumber: 10 }, __self: this }),
          _react2.default.createElement('h1', { className: 'App-title', __source: { fileName: _jsxFileName, lineNumber: 11 }, __self: this }, 'Welcome to React')),

        _react2.default.createElement('p', { className: 'App-intro', __source: { fileName: _jsxFileName, lineNumber: 13 }, __self: this }, 'To get started, edit ',
          _react2.default.createElement('code', { __source: { fileName: _jsxFileName, lineNumber: 14 }, __self: this }, 'src/App.js'), ' and save to reload.')));



  }}
    √ renders without crashing (17ms)

@cpojer could you please re-open the PR & issue?

[SimenB]

What about Adder.name? Or Adder.displayName

[JoshuaKGoldberg]

Undefined in IE. Two reasons why this is relevant:

• We're talking about letting Jest run in-browser, a common use case for which is old browser compat testing
• Some teams that are on TypeScript use modified versions of the standard lib.d.ts that exclude properties not known to work in all browsers, such as Function::name. That means they can't be used in tests either. Presumably Flow folks could have a similar situation.

[SimenB]

Hmm, OK. You can override describe in http://facebook.github.io/jest/docs/en/configuration.html#setuptestframeworkscriptfile-string, but maybe not as elegant

[cpojer]

Alright, I thought about this some more and have no reason to push back on it. Please add an integration test for this feature before we can merge it.

[SimenB]

Can you add a note to the changelog as well?

[SimenB]

This is failing on all CIs, something with the snapshot that was created is wrong (missing a number). It's also weird that you only have a single snapshot as you call toMatchSnapshot twice

[SimenB]

leave this out, it says "master" at the top 🙂

[JoshuaKGoldberg]

Added a thrown error if what's passed to describe isn't a string, class, or function. I tried adding a snapshot & test case (c4b8343) but CI required source lines including ..\circle-ci, and I couldn't find an example of existing code that has a similar test case type. Removed in e1f6c7f.

Also: I showed this PR off-hand to a friend and they mentioned they intentionally use numbers as their describe descriptors in some tests. Added numbers as an allowed typeof. I'm +0 to only doing the .name logic for function types, and otherwise just returning as before for others (to more closely mirror the existing behavior)... will change the PR if asked to. :)

[cpojer]

Thanks for making this Pr :)

[SimenB]

@JoshuaKGoldberg I'm looking at converting this to TS, and it's not happy about the second argument here. It's also not documented at MDN: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/search

Should I just remove it?

[JoshuaKGoldberg]

@SimenB ha, sure.

Looks like yet another "bug" fixed by TypeScript! 😍

[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.