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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return cleanup functions from beforeAll and beforeEach #10453

Closed
remcohaszing opened this issue Aug 26, 2020 · 5 comments
Closed

Return cleanup functions from beforeAll and beforeEach #10453

remcohaszing opened this issue Aug 26, 2020 · 5 comments

Comments

@remcohaszing
Copy link
Contributor

馃殌 Feature Proposal

It would be a nice addition if the beforeAll() and beforeEach() function could return cleanup functions. This is based on the cleanup function of React.useEffect().

Because one setup may depend on another, the cleanup functions should run in the reverse order of their definition after the afterAll() and afterEach() blocks respectively. I.e. the following test:

afterAll(() => {
  console.log(1);
});

beforeAll(() => {
  console.log(2);

  return () => {
    console.log(3);
  };
});

afterAll(() => {
  console.log(4);
});

beforeAll(() => {
  console.log(5);

  return () => {
    console.log(6);
  };
});

afterAll(() => {
  console.log(7);
});

it('', () => {
  console.log('test');
});

will log:

2
5
test
1
4
7
6
3

Motivation

Currently one needs to store the variables on a higher scope, so the cleanup can be done in a afterAll() or afterEach() block. This works fine, but it鈥檚 more work. One needs to declare the variables and possibly explicitly add type definitions. Also returning such cleanup functions keeps related setup and cleanup close together.

Example

Currently:

import { Sequelize } from 'sequelize-typescript';

import { User } from '../models';
import { setupDB } from '../utils';

let db: Sequelize;

beforeAll(async () => {
  db = await setupDB();
});

afterAll(async () =>
  await db.close();
});

it('should create a user', async () => {
  await User.create({ name: 'Me' });
});

With returned cleanup functions.

import { User } from '../models';
import { setupDB } from '../utils';

beforeAll(async () => {
  const db = await setupDB();

  return () => db.close();
});

it('should create a user', async () => {
  await User.create({ name: 'Me' });
});

Variables can still be assigned for use in other setup blocks or tests

import { Sequelize } from 'sequelize-typescript';

import { User } from '../models';
import { setupDB } from '../utils';

let db: Sequelize

beforeAll(async () => {
  db = await setupDB();

  return () => db.close();
});

afterEach(async () => {
  await db.truncate()
});

it('should create a user', async () => {
  await User.create({ name: 'Me' });
});

Pitch

This changes the behaviour of beforeAll() and beforeEach().

@jeysal
Copy link
Contributor

jeysal commented Sep 27, 2020

I agree that the hooks API could use a redesign (in fact I talked about this over a year ago). I'm not quite sure how that would happen - perhaps as additional functions under a different name.

I'm not sure we have another ticket open for this (#7823 is quite different), so we can leave this open for tracking I guess.

I would also like to mention that this is not the only way to improve hooks by using return values, so there would probably need to be some debate around it.
E.g. this is good for when you have before and after, but don't need anything in the module scope:

beforeEach(() => {
  const originalConsoleLog = console.log;
  console.log = jest.fn();
  return () => {
    console.log = originalConsoleLog;
  }
})

But the following is good for when you have before and need something in the module scope, but do not need after (e.g. with react-testing-library which does it for you):

const fixtureRef = beforeEach(() => {
  return render(<Component />);
})

test('t', () => {
  fixtureRef.current.getByText('hello world');
})

A second parameter if after is needed could also be a consideration:

const fixtureRef = aroundEach(() => {
  return render(<Component />);
}, (fixture) => {
  fixture.unmount();
})

test('t', () => {
  fixtureRef.current.getByText('hello world');
})

@remcohaszing
Copy link
Contributor Author

I have given the @jeysal鈥檚 some thought. I like this as well, but I don鈥檛 like the idea of having to use .current in every test to access the wanted value. This might even be a source of confusion for some users.

Of course one could still the following, even if the hook would return a fixture ref.

let fixture;

aroundEach(() => {
  fixture = render(<Component />);
  return fixture;
}, (ref /* Same as fixture */) => {
  fixture.unmount();
})

I do like the idea of using a new name for the hook, i.e. aroundEach or setupEach.

@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

github-actions bot commented May 5, 2022

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 May 5, 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