Allowed named classes and functions as BlockNames #5154

JoshuaKGoldberg · 2017-12-21T16:27:17Z

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!)

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

JoshuaKGoldberg · 2017-12-21T16:49:50Z

packages/jest-jasmine2/src/jasmine/Suite.js

+  }
+
+  const stringified = descriptor.toString();
+  const typeDescriptorMatch = stringified.match(/class|function/);


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?

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

codecov-io · 2017-12-21T16:52:39Z

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 · 2017-12-22T09:08:11Z

Thanks for the PR!

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

cpojer · 2017-12-22T09:40:47Z

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

JoshuaKGoldberg · 2017-12-22T17:58:54Z

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 · 2017-12-22T18:07:57Z

What about Adder.name? Or Adder.displayName

JoshuaKGoldberg · 2017-12-22T18:11:12Z

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 · 2017-12-22T18:16:14Z

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

cpojer · 2017-12-22T21:08:02Z

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 · 2017-12-23T10:24:21Z

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 · 2017-12-24T00:42:35Z

CHANGELOG.md

@@ -8,6 +8,13 @@ None for now

 ### Chore & Maintenance

+## jest 22.0.5


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

# Conflicts: # CHANGELOG.md

JoshuaKGoldberg · 2017-12-24T18:52:48Z

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 · 2017-12-24T21:54:31Z

Thanks for making this Pr :)

SimenB · 2019-02-09T22:00:46Z

packages/jest-jasmine2/src/jasmine/Suite.js

+  const typeDescriptorMatch = stringified.match(/class|function/);
+  const indexOfNameSpace =
+    typeDescriptorMatch.index + typeDescriptorMatch[0].length;
+  const indexOfNameAfterSpace = stringified.search(/\(|\{/, indexOfNameSpace);


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

@SimenB ha, sure.

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

github-actions · 2021-05-12T01:30:20Z

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.

facebook-github-bot added the CLA Signed ✔️ label Dec 21, 2017

Allowed named classes and functions as BlockNames

2ec0c8b

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

JoshuaKGoldberg commented Dec 21, 2017

View reviewed changes

cpojer closed this Dec 22, 2017

cpojer reopened this Dec 22, 2017

Added integration test for describe with a class or function

79a11aa

SimenB reviewed Dec 23, 2017

View reviewed changes

Josh Goldberg added 2 commits December 23, 2017 19:15

Corrected globals.test.js.snap

b335f41

Mentioned changes in CHANGELOG.md

db2fc7d

SimenB reviewed Dec 24, 2017

View reviewed changes

SimenB approved these changes Dec 24, 2017

View reviewed changes

Josh Goldberg added 8 commits December 23, 2017 19:44

Master!

0bae63c

Merge branch 'master' into describe-function-and-class-names

67dcfe6

# Conflicts: # CHANGELOG.md

Added failure for non-string/class/function

8cd8505

I am become pretty

74ca575

Fixed failure message for globals.test.js.snap

c4b8343

Removed error test case for array-as-descriptor

e1f6c7f

Removed error test for array-as-descriptor

9c6b2c5

Added allowance for numbers as test descriptors

0744cf9

Prettier...

e547ef4

SimenB approved these changes Dec 24, 2017

View reviewed changes

cpojer merged commit 2cb57a0 into jestjs:master Dec 24, 2017

JoshuaKGoldberg deleted the describe-function-and-class-names branch December 24, 2017 21:57

yangshun mentioned this pull request Jan 11, 2018

Improve tests structure nusmodifications/nusmods#695

Open

renovate-tests mentioned this pull request Feb 6, 2018

Update dependency jest to v22 - autoclosed renovate-tests/changelog-md#3

Closed

renovate bot mentioned this pull request Feb 14, 2018

fix(deps): update dependency jest to ^22.0.0 manekinekko/devoxx-assistant#3

Open

ZhangYiJiang mentioned this pull request Feb 19, 2018

Add Jest support for using class and function as test name in describe() and test() flow-typed/flow-typed#1858

Merged

SimenB reviewed Feb 9, 2019

View reviewed changes

ulrichb mentioned this pull request Oct 16, 2019

[valid-describe] Support functions as 'name' argument jest-community/eslint-plugin-jest#431

Closed

github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allowed named classes and functions as BlockNames #5154

Allowed named classes and functions as BlockNames #5154

JoshuaKGoldberg commented Dec 21, 2017

JoshuaKGoldberg Dec 21, 2017

cpojer Dec 24, 2017

codecov-io commented Dec 21, 2017 •

edited

Loading

SimenB commented Dec 22, 2017

cpojer commented Dec 22, 2017

JoshuaKGoldberg commented Dec 22, 2017

SimenB commented Dec 22, 2017 •

edited

Loading

JoshuaKGoldberg commented Dec 22, 2017

SimenB commented Dec 22, 2017

cpojer commented Dec 22, 2017

SimenB left a comment

SimenB commented Dec 23, 2017

SimenB Dec 24, 2017

JoshuaKGoldberg commented Dec 24, 2017 •

edited

Loading

cpojer commented Dec 24, 2017

SimenB Feb 9, 2019

JoshuaKGoldberg Feb 9, 2019

github-actions bot commented May 12, 2021

		@@ -8,6 +8,13 @@ None for now

		### Chore & Maintenance

		## jest 22.0.5

Allowed named classes and functions as BlockNames #5154

Allowed named classes and functions as BlockNames #5154

Conversation

JoshuaKGoldberg commented Dec 21, 2017

JoshuaKGoldberg Dec 21, 2017

Choose a reason for hiding this comment

cpojer Dec 24, 2017

Choose a reason for hiding this comment

codecov-io commented Dec 21, 2017 • edited Loading

Codecov Report

SimenB commented Dec 22, 2017

cpojer commented Dec 22, 2017

JoshuaKGoldberg commented Dec 22, 2017

SimenB commented Dec 22, 2017 • edited Loading

JoshuaKGoldberg commented Dec 22, 2017

SimenB commented Dec 22, 2017

cpojer commented Dec 22, 2017

SimenB left a comment

Choose a reason for hiding this comment

SimenB commented Dec 23, 2017

SimenB Dec 24, 2017

Choose a reason for hiding this comment

JoshuaKGoldberg commented Dec 24, 2017 • edited Loading

cpojer commented Dec 24, 2017

SimenB Feb 9, 2019

Choose a reason for hiding this comment

JoshuaKGoldberg Feb 9, 2019

Choose a reason for hiding this comment

github-actions bot commented May 12, 2021

codecov-io commented Dec 21, 2017 •

edited

Loading

SimenB commented Dec 22, 2017 •

edited

Loading

JoshuaKGoldberg commented Dec 24, 2017 •

edited

Loading