[Bug]: jest-circus doesn't respect beforeAll / beforeEach / afterEach / afterAll order

[satanTime]

Version

27.5.1

Steps to reproduce

https://app.circleci.com/pipelines/github/ike18t/ng-mocks/4752/workflows/52fd9352-393d-4011-ad15-c3eaebbacb27/jobs/183399

const triggers: string[] = [];

describe('mock-instance-reset', () => {
  describe('test', () => {
    beforeAll(() => triggers.push('beforeAll:1'));
    afterAll(() => triggers.push('afterAll:1'));

    beforeAll(() => triggers.push('beforeAll:2'));
    afterAll(() => triggers.push('afterAll:2'));

    beforeEach(() => triggers.push('beforeEach:1'));
    beforeEach(() => triggers.push('beforeEach:2'));

    afterEach(() => triggers.push('afterEach:1'));
    afterEach(() => triggers.push('afterEach:2'));

    it('triggers test #1', () => {
      expect(1).toEqual(1);
    });

    it('triggers test #2', () => {
      expect(2).toEqual(2);
    });

    describe('nested', () => {
      beforeAll(() => triggers.push('beforeAll:3'));
      afterAll(() => triggers.push('afterAll:3'));

      beforeEach(() => triggers.push('beforeEach:3'));
      afterEach(() => triggers.push('afterEach:3'));

      it('triggers test #3', () => {
        expect(3).toEqual(3);
      });
    });
  });

  it('has expected order', () => {
    // first before is called the first
    // first after is called the last
    expect(triggers).toEqual([
      'beforeAll:1',
      'beforeAll:2',

      'beforeEach:1',
      'beforeEach:2',
      'afterEach:2',
      'afterEach:1',

      'beforeEach:1',
      'beforeEach:2',
      'afterEach:2',
      'afterEach:1',

      'beforeAll:3',
      'beforeEach:1',
      'beforeEach:2',
      'beforeEach:3',
      'afterEach:3',
      'afterEach:2',
      'afterEach:1',
      'afterAll:3',

      'afterAll:2',
      'afterAll:1',
    ]);
  });
});

Expected behavior

Based on documentation, https://jestjs.io/docs/setup-teardown, before hooks are FIFO, after hooks are LIFO.
it works like that in jest-jasmine2, whereas jest-circus doesn't respect the order.

Actual behavior

jest-circus calls after hooks as FIFO.

        "beforeAll:1",
        "beforeAll:2",
        "beforeEach:1",
        "beforeEach:2",
    -   "afterEach:2", // expected before #1
        "afterEach:1",
    +   "afterEach:2", // called after #1
        "beforeEach:1",
        "beforeEach:2",
    +   "afterEach:1",
        "afterEach:2",
    -   "afterEach:1",
        "beforeAll:3",
        "beforeEach:1",
        "beforeEach:2",
        "beforeEach:3",
        "afterEach:3",
    +   "afterEach:1",
        "afterEach:2",
    -   "afterEach:1",
        "afterAll:3",
    -   "afterAll:2",
        "afterAll:1",
    +   "afterAll:2",

Additional context

No response

Environment

System:
    OS: macOS 12.3.1
    CPU: (16) x64 Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
  Binaries:
    Node: 16.13.2 - /opt/local/bin/node
    Yarn: 1.22.17 - /opt/local/bin/yarn
    npm: 8.7.0 - /Volumes/MGS/Projects/ng-mocks/node_modules/.bin/npm
  npmPackages:
    jest: 27.5.1 => 27.5.1

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

[satanTime]

Up

[justjake]

I just encountered this bug while trying to migrate test helpers to Jest.

[justjake]

Here's my work-around:

import {
	beforeAll,
	afterAll as fifoAfterAll,
	beforeEach,
	afterEach as fifoAfterEach,
} from "@jest/globals"

export * from "@jest/globals"

/**
 * Jest w/ Circus incorrectly runs afterEach hooks in FIFO order.
 * We need to do some shenanigans to run them in LIFO order.
 * https://github.com/facebook/jest/issues/12678
 */
function createLastInFirstOutHook(
	hookName: string,
	fifoBeforeHook: Global.HookBase,
	fifoAfterHook: Global.HookBase
) {
	const stack: HookFn[] = []

	return function lifoJestHook(fn: () => Promise<void> | void) {
		fifoBeforeHook(() => {
			stack.push(fn)
		})

		fifoAfterHook(function lifoHookInvocation(this: TestContext) {
			const fn = stack.pop()
			if (!fn) {
				throw new Error(`${hookName} hook called too many times`)
			}
			return (fn as PromiseReturningTestFn).call(this)
		} as DoneTakingTestFn)
	}
}

/**
 * Run the given function after each test.
 *
 * `afterEach` hooks defined with this function will be run in the reverse order
 * of definition, with the last one to be defined running first (LIFO order).
 */
export const afterEach = createLastInFirstOutHook(
	"afterEach",
	beforeEach,
	fifoAfterEach
)

/**
 * Run the given function after all tests in the `describe` block or suite.
 *
 * `afterAll` hooks defined with this function will be run in the reverse order
 * of definition, with the last one to be defined running first (LIFO order).
 */
export const afterAll = createLastInFirstOutHook(
	"afterAll",
	beforeAll,
	fifoAfterAll
)

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

[satanTime]

up

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

[satanTime]

Up

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

[satanTime]

Up

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

[satanTime]

Up

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

[satanTime]

up

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

[satanTime]

Up

[AndrewSouthpaw]

FWIW it looks like they updated the docs to note the difference in test runners: https://jestjs.io/docs/setup-teardown#order-of-execution

[satanTime]

Hi @AndrewSouthpaw,

I would still vote for fixing a bug instead of documenting it.

There are many testing libraries around, which use hooks under the hood, and the expectation is that the order of their execution will be respected, as it is in all other testing frameworks. Not sure why jest decided to invent own way and to cause issues.

Let's take a look at a simple example: we want to backup a global variable for a test, and restore it afterwards.
To avoid copy-pasting of tons of lines of code, we decided to write a helper function:

const backupTest = (newValue: typeof globalVar): void => {
  let backup: typeof globalVar;

  beforeEach(() => {
    backup = globalVar;
    globalVar = newValue;
  });

  afterEach(() => {
    globalVar = backup;
  });
};

The idea is simple, our test should look like that:

describe('globalVar', () => {
  backupTest(1);

  it('equals 1', () => {
    expect(globalVar).toEqual(1);
  });
});

so instead of having 10 lines of code how to backup globalVar for each test, we have just 1.

Now, let's imagine that globalVar isn't primitive, but a complex object and backupTest does only 1 modification, whereas we need an additional modification, for example, another group of stub members.

In all testing libraries it will look like that:

describe('globalVar', () => {
  backupTest(1); // one group of stub members.
  backupTest(2); // another group of stub members.

  // tests
});

and it works well, after the test, globalVar will have its initial value before the suite, because the first executed afterEach belongs to 2 and the second executed afterEach belongs to 1.

However, with the new changes in jest, globalVar is going to be 1, because of the broken order, because first jest executes afterEach for 1, and then afterEach for 2, which restores a wrong value.

let globalVar = 0;

const backupTest = (newValue: typeof globalVar): void => {
  let backup: typeof globalVar;

  beforeEach(() => {
    backup = globalVar;
    globalVar = newValue;
  });

  afterEach(() => {
    globalVar = backup;
  });
};

describe('backup', () => {
  it('equals 0 before all', () => {
    expect(globalVar).toEqual(0);
  });

  describe('globalVar', () => {
    backupTest(1); // setting globalVar to 1 and restoring it to 0 afterwards
    backupTest(2); // setting globalVar to 2 and restoring it to 1 afterwards

    it('equals 2 before each', () => {
      expect(globalVar).toEqual(2);
    });

    describe('each', () => {
      backupTest(3); // setting globalVar to 3 and restoring it to 2 afterwards
      backupTest(4); // setting globalVar to 4 and restoring it to 3 afterwards

      it('equals 4 after each', () => {
        expect(globalVar).toEqual(4);
      });
    });

    it('resets to 2 after each', () => {
      expect(globalVar).toEqual(2);
    });
  });

  it('resets to 0 after all', () => {
    expect(globalVar).toEqual(0);
  });
});

which fails on jest as

    expect(received).toEqual(expected) // deep equality

    Expected: 0
    Received: 1

      42 |
      43 |   it('resets to 0 after all', () => {
    > 44 |     expect(globalVar).toEqual(0);
         |                       ^
      45 |   });
      46 | });
      47 |

[AndrewSouthpaw]

Oh I agree its confusing, and I think your code sample demonstrates how it's an easy issue to stub your foot on. I would've assumed afterEach was LIFO. I just wanted to point out that it is now documented, addressing this part of the OP:

Based on documentation, https://jestjs.io/docs/setup-teardown, before hooks are FIFO, after hooks are LIFO.

Thanks for keeping this issue open! Hopefully it gains traction / attention at some point.

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

[justjake]

Bump

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

[satanTime]

Up

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

[satanTime]

Up

[frosas]

This looks like a duplicate of #11456

[satanTime]

Hi @frosas, agree! closing this one.

[github-actions]

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.