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

Allowed functions in jest's describe #23102

Merged
merged 9 commits into from Mar 21, 2018
Merged

Allowed functions in jest's describe #23102

merged 9 commits into from Mar 21, 2018

Conversation

JoshuaKGoldberg
Copy link
Collaborator

As of 22.0.5, jest allows classes for describe blocks.

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

Josh Goldberg added 2 commits January 21, 2018 19:29
As of 22.0.5, jest allows classes for describe blocks.
@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Awaiting reviewer feedback labels Jan 22, 2018
@typescript-bot
Copy link
Contributor

typescript-bot commented Jan 22, 2018

@JoshuaKGoldberg Thank you for submitting this PR!

🔔 @NoHomey @jwbay @asvetliakov @alexjoverm @epicallan @ikatyang @wsmd @JamieMason @douglasduteil @AhnpGit - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@typescript-bot
Copy link
Contributor

typescript-bot commented Jan 22, 2018

@JoshuaKGoldberg The Travis CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks!

@@ -199,6 +199,10 @@ declare namespace jest {

type Lifecycle = (fn: ProvidesCallback, timeout?: number) => any;

interface IClassLike {
Copy link
Contributor

@wsmd wsmd Jan 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about calling this interface FunctionLike?

Since classes are technically "special functions" and the describe implementation obviously works functions as well, I think FunctionLike would be better here for the sake of clarity.

I'm thinking about the following scenarios:

const myInstance = new SomeClass();

describe(myInstance.constructor /* I don't know what you would do that but whatever 😅 */, () => {
  // ...
});
function doSomething() {
  // ...
}

describe(doSomething, () => {
  // ...
});

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM! Made the change.

I also added Function to the allowed type union because some projects don't provide a name string in their Function typings (since it doesn't exist in IE).

@typescript-bot typescript-bot added Where is Travis? Has Merge Conflict This PR can't be merged because it has a merge conflict. The author needs to update it. and removed The Travis CI build failed labels Feb 8, 2018
@typescript-bot
Copy link
Contributor

@JoshuaKGoldberg - It appears Travis did not correctly run on this PR! /cc @RyanCavanaugh to investigate and advise.

@JoshuaKGoldberg
Copy link
Collaborator Author

JoshuaKGoldberg commented Feb 8, 2018

I'll resolve the merge conflict ✔️

@typescript-bot typescript-bot added The Travis CI build failed and removed Where is Travis? Has Merge Conflict This PR can't be merged because it has a merge conflict. The author needs to update it. labels Feb 8, 2018
@JoshuaKGoldberg
Copy link
Collaborator Author

Ehh, I don't think my changes are causing the failures... @RyanCavanaugh

@aozgaa aozgaa closed this Mar 17, 2018
@aozgaa aozgaa reopened this Mar 17, 2018

declare var beforeAll: jest.Lifecycle;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you remove this ?

// Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped
// TypeScript Version: 2.3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please put this back, Travis failed because it couldn't find this line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spooky - must have been a bad merge...

@typescript-bot
Copy link
Contributor

🔔 @AhnpGit - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate?

@typescript-bot
Copy link
Contributor

A definition author has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Popular package This PR affects a popular package (as counted by NPM download counts).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants