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: Update for v27 #55013

Merged
merged 2 commits into from
Aug 10, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 10 additions & 19 deletions types/jest/index.d.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Type definitions for Jest 26.0
// Type definitions for Jest 27.0
// Project: https://jestjs.io/
// Definitions by: Asana (https://asana.com)
// Ivo Stratev <https://github.com/NoHomey>
Expand Down Expand Up @@ -28,6 +28,7 @@
// Pawel Fajfer <https://github.com/pawfa>
// Regev Brody <https://github.com/regevbr>
// Alexandre Germain <https://github.com/gerkindev>
// Adam Jones <https://github.com/domdomegg>
// Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped
// Minimum TypeScript Version: 3.8

Expand Down Expand Up @@ -73,10 +74,6 @@ type ExtractEachCallbackArgs<T extends ReadonlyArray<any>> = {
];

declare namespace jest {
/**
* Provides a way to add Jasmine-compatible matchers into your Jest context.
*/
function addMatchers(matchers: jasmine.CustomMatcherFactories): typeof jest;
/**
* Disables automatic mocking in the module loader.
*/
Expand Down Expand Up @@ -194,11 +191,6 @@ declare namespace jest {
*/
// tslint:disable-next-line: no-unnecessary-generics
function requireMock<TModule extends {} = any>(moduleName: string): TModule;
/**
* Resets the module registry - the cache of all required modules. This is
* useful to isolate modules where local state might conflict between tests.
*/
function resetModuleRegistry(): typeof jest;
/**
* Resets the module registry - the cache of all required modules. This is
* useful to isolate modules where local state might conflict between tests.
Expand All @@ -216,14 +208,18 @@ declare namespace jest {
function retryTimes(numRetries: number): typeof jest;
/**
* Exhausts tasks queued by setImmediate().
* > Note: This function is only available when using modern fake timers
* > implementation
*/
function runAllImmediates(): typeof jest;
/**
* Exhausts the micro-task queue (usually interfaced in node via process.nextTick).
*/
function runAllTicks(): typeof jest;
/**
* Exhausts the macro-task queue (i.e., all tasks queued by setTimeout() and setInterval()).
* Exhausts both the macro-task queue (i.e., all tasks queued by setTimeout(),
* setInterval(), and setImmediate()) and the micro-task queue (usually interfaced
* in node via process.nextTick).
*/
function runAllTimers(): typeof jest;
/**
Expand All @@ -233,11 +229,6 @@ declare namespace jest {
* those new tasks will not be executed by this call.
*/
function runOnlyPendingTimers(): typeof jest;
/**
* (renamed to `advanceTimersByTime` in Jest 21.3.0+) Executes only the macro
* task queue (i.e. all tasks queued by setTimeout() or setInterval() and setImmediate()).
*/
function runTimersToTime(msToRun: number): typeof jest;
/**
* Advances all timers by msToRun milliseconds. All pending "macro-tasks" that have been
* queued via setTimeout() or setInterval(), and would be executed within this timeframe
Expand Down Expand Up @@ -340,9 +331,10 @@ declare namespace jest {
fail(error?: string | { message: string }): any;
}

type ProvidesCallback = (cb: DoneCallback) => any;
type ProvidesCallback = ((cb: DoneCallback) => void | undefined) | (() => Promise<unknown>);
Copy link
Contributor

@G-Rath G-Rath Aug 8, 2021

Choose a reason for hiding this comment

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

While this is correct for test suite & case functions, it's technically not for hooks because you can return any value so long as you don't have the done callback, as it's meant to make it easier compatibility with short-handed arrow functions; currently this type will require folks to return a promise even if they're not doing anything async.

I think we should make a new HookCallback to cover this (I'm impartial to if we create a new Lifecycle-like type or make Lifecycle take a generic of ProvidesCallback | HookCallback)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review! 🙌

currently this type will require folks to return a promise even if they're not doing anything async

The ProvidesCallback type should allow them to return void or undefined or a promise if they don't use done (the tests verify this I believe). If they use done, they must return void or undefined. We could make this more explicit perhaps by changing this line to:

type ProvidesCallback = ((cb: DoneCallback) => void | undefined) | (() => Promise<unknown> | void | undefined);

make it easier compatibility with short-handed arrow functions

I'm not sure of exactly what you mean here. This change will disallow code like:

const getMagicNumber = () => 1234;
jest.beforeAll(() => getMagicNumber());

because we would then be returning a number. This is not void, undefined or Promise.

This is intentional for Jest 27 to match @SimenB's change: "Disallow return values other than a Promise from hooks and tests"

If you think we should allow hooks to return any we could easily implement that with something like:

type ProvidesHookCallback = (cb: DoneCallback) => any;
// or just so it's clear that it's a subtype (but wouldn't change the actual typing)
type ProvidesHookCallback = (cb: DoneCallback) => any | ProvidesCallback;

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

...but I believe this will create a discrepancy between DefinitelyTyped and Jest which is undesirable. Happy to take the consensus of active reviewers though.

Copy link
Contributor

@G-Rath G-Rath Aug 9, 2021

Choose a reason for hiding this comment

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

What I'm meaning is that it's still valid to return anything from a hook so long as you don't have done - I've confirmed this with this on latest jest:

describe('a test', () => {
  beforeEach(() => 1);

  it('passes', () => {
    expect(true).toBe(true);
  });
});

We're even doing it in eslint-plugin-jest:

// no-deprecated-functions.ts
/** @internal */
export const _clearCachedJestVersion = () => (cachedJestVersion = null);

// __tests_/no-deprecated-functions.ts
describe('the rule', () => {
  beforeEach(() => _clearCachedJestVersion());
  ...
});

The problem I'm foreseeing is that people will be using this pattern without issue on Jest 27 right now, only for them to update their types and suddenly get it flagged as an error despite it working at runtime - and unless they scour the changelog and find the entry mentioning this breaking type-only change, I expect they'll open issues and PRs here saying the types are wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, happy to - I only made this change to match the changelog and internal jest types.

As long as nobody else objects I'll change this, as per the end of my previous comment (adding ProvidesHookCallback).

Copy link
Contributor

@G-Rath G-Rath Aug 9, 2021

Choose a reason for hiding this comment

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

as per the end of my previous comment (adding ProvidesHookCallback).

I believe the change you described there won't be correct: Lifecycle is currently used to type both hooks (which do allow returning anything unless you're using done) & tests (which don't allow returning anything ever).

The current change you've made looks correct for tests, so I think we just need to make a new type (or really duplicate the types as they were before your change) for hooks - which is why I suggested maybe using a generic might be tidier since it would save having two Lifecycle types.

Nope I'm wrong - it's ProvidesCallback that is being used for both hooks and tests currently 😅

In saying that, the ProvidesHookCallback should not allow returning if the callback function is present, as that will cause an error in v27.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about when using jest-jasmine2 as the test runner? In such cases, returning a Promise together with the done callback is completely ok and safe. I had to revert the version back exactly because of this.

We should rely on the runner reporting runtime errors instead of the type system, especially in cases where the runtime is configurable (like for jest). When using jest-circus, the combination of a done callback and a returned promise will raise a descriptive error:

Test functions cannot both take a 'done' callback and return something. Either use a 'done' callback, or return a promise.
Suggested change
type ProvidesCallback = ((cb: DoneCallback) => void | undefined) | (() => Promise<unknown>);
type ProvidesCallback = (cb: DoneCallback) => Promise<void | undefined> | void | undefined;

Copy link
Contributor Author

@domdomegg domdomegg Aug 24, 2021

Choose a reason for hiding this comment

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

Hi @enisdenjo, can you give an example of a test in jest-jasmine2 that requires both a done callback and returning a promise?

As per the Jest changelog and blog, breaking changes in Jest 27 include making TypeScript types stricter to prevent returning a Promise and taking a done callback - no matter the runtime environment.

It might be worth raising an issue on Jest directly rather than the types if you disagree with the change to disallow using both a done callback and returning a promise. If you think here in the types we've misinterpreted what Jest has changed, then happy to discuss - but otherwise we're debating something that is out of scope for DefinitelyTyped.

Copy link
Contributor

@enisdenjo enisdenjo Aug 24, 2021

Choose a reason for hiding this comment

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

Hey @domdomegg,

can you give an example of a test in jest-jasmine2 that requires both a done callback and returning a promise?

I use done+promise all around, here are a few examples:

Do note that all above examples run on Jest v27 with the test runner set to jest-jasmine2.

This PR makes all of them fail with TS, even though its perfectly type legal and safe to do so with jest-jasmine2.

As per the Jest changelog and blog, breaking changes in Jest 27 include making TypeScript types stricter to prevent returning a Promise and taking a done callback - no matter the runtime environment.

I think you're wrong here, I guess you're talking about this entry in the changelog:

[jest-circus] [BREAKING] Fail tests when multiple done() calls are made (jestjs/jest#10624)

The change is exclusive to jest-circus, not whole jest (as stated in the scope on the beginning of the changelog entry). jest-circus is a test runner and jestjs/jest#10624 fails tests during runtime.

See #55013 (comment).

It might be worth raising an issue on Jest directly rather than the types if you disagree with the change to disallow using both a done callback and returning a promise.

Personally, I am against this change, it just complicates stuff unnecessarily. You want async tests to leverage the await and still use a done in a callback to indicate completion where tests have mixes of promises and events (as my examples show above).

Furthermore, there is already an issue raised for jest-circus: jestjs/jest#10529. Yeah, sure, you can just do jestjs/jest#10529 (comment); but, test suites are meant to be convenient and easy to use.

Copy link
Contributor

@enisdenjo enisdenjo Aug 24, 2021

Choose a reason for hiding this comment

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

My bad, I referenced the wrong changelog entry before. You were probably talking about:

[jest-globals] [BREAKING] Disallow mixing a done callback and returning a Promise from hooks and tests (jestjs/jest#10512)

which does indeed belong to jest-globals. 🤔

Copy link
Contributor

@enisdenjo enisdenjo Aug 24, 2021

Choose a reason for hiding this comment

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

@domdomegg even though the v27 changelog claims done+promise is disallowed globally, it seems not to be. The done+promise error is only present when using the jest-circus test runner and the assertion can be found here (inside jest-circus) only:

https://github.com/facebook/jest/blob/7c6cea2696e6f08553189a5e077126d610751576/packages/jest-circus/src/utils.ts#L212-L220

type ProvidesHookCallback = (() => any) | ProvidesCallback;

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

interface FunctionLike {
readonly name: string;
Expand Down Expand Up @@ -1371,7 +1363,6 @@ declare namespace jasmine {
function createSpyObj<T>(baseName: string, methodNames: any[]): T;
function pp(value: any): string;
function addCustomEqualityTester(equalityTester: CustomEqualityTester): void;
function addMatchers(matchers: CustomMatcherFactories): void;
function stringMatching(value: string | RegExp): Any;

interface Clock {
Expand Down
90 changes: 84 additions & 6 deletions types/jest/jest-tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,20 +15,36 @@ describe('', () => {
/* Lifecycle events */

beforeAll(() => {});
beforeAll(() => null);
beforeAll(() => true);
beforeAll((done: jest.DoneCallback) => {});
beforeAll((done: jest.DoneCallback) => done.fail(), 9001);
// $ExpectError
beforeAll((done: jest.DoneCallback) => Promise.resolve());

beforeEach(() => {});
beforeEach(() => null);
beforeEach(() => true);
beforeEach((done: jest.DoneCallback) => {});
beforeEach((done: jest.DoneCallback) => done.fail(), 9001);
// $ExpectError
beforeEach((done: jest.DoneCallback) => Promise.resolve());

afterAll(() => {});
afterAll(() => null);
afterAll(() => true);
afterAll((done: jest.DoneCallback) => {});
afterAll((done: jest.DoneCallback) => done.fail(), 9001);
// $ExpectError
afterAll((done: jest.DoneCallback) => Promise.resolve());

afterEach(() => {});
afterEach(() => null, 9001);
afterEach(() => true, 9001);
afterEach((done: jest.DoneCallback) => {});
afterEach((done: jest.DoneCallback) => done.fail(), 9001);
// $ExpectError
afterEach((done: jest.DoneCallback) => Promise.resolve());

/* describe */

Expand Down Expand Up @@ -246,10 +262,7 @@ describe('', () => {

const customMatcherFactories: jasmine.CustomMatcherFactories = {};

jest.addMatchers(customMatcherFactories)
.addMatchers({})
.addMatchers(customMatcherFactories)
.autoMockOff()
jest.autoMockOff()
.autoMockOn()
.clearAllMocks()
.clearAllTimers()
Expand All @@ -268,15 +281,13 @@ jest.addMatchers(customMatcherFactories)
.mock('moduleName', jest.fn())
.mock('moduleName', jest.fn(), {})
.mock('moduleName', jest.fn(), { virtual: true })
.resetModuleRegistry()
.resetModules()
.isolateModules(() => {})
.retryTimes(3)
.runAllImmediates()
.runAllTicks()
.runAllTimers()
.runOnlyPendingTimers()
.runTimersToTime(9001)
.advanceTimersByTime(9001)
.setMock('moduleName', {})
.setMock<{}>('moduleName', {})
Expand Down Expand Up @@ -1494,3 +1505,70 @@ test.only.each`

expect('').toHaveProperty('path.to.thing');
expect('').toHaveProperty('path.to.thing', {});

/* Test function can return a promise */

test(`returns a Promise<boolean>`, () => {
return Promise.resolve(true);
});

test(`returns a Promise<{ isAnObject: boolean }>`, () => {
return Promise.resolve({ isAnObject: true });
});

test(`returns a Promise<any>`, () => {
return Promise.resolve('any' as any);
});

/* Test function can take and call the done callback function */

test(`uses done`, (done) => {
done();
});

/* Test function can do nothing */

test(`does nothing`, () => {
// noop
});

/* Test function should not return non-promise */

// $ExpectError
test(`returns a boolean`, () => {
return true;
});

// $ExpectError
test(`returns a number`, () => {
return 3;
});

// $ExpectError
test(`returns an object`, () => {
return {
isAnObject: true
};
});

/* Test function should not return promise and takes done callback function */

// $ExpectError
test(`returns a Promise<boolean> and takes done`, (done) => {
return Promise.resolve(true);
});

// $ExpectError
test(`returns a Promise<{ isAnObject: boolean }> and takes done`, (done) => {
return Promise.resolve({ isAnObject: true });
});

// $ExpectError
test(`returns a Promise<any> and takes done`, (done) => {
return Promise.resolve('any' as any);
});

// $ExpectError
test(`async function takes done`, async (done) => {
done();
});