Timers mock panic with sub-test

[axetroy]

Version

v22.10.0

Platform

No response

Subsystem

No response

What steps will reproduce the bug?

1. Create a test file a.test.mjs

import test from 'node:test'

test.beforeEach((t) => {
    t.mock.timers.enable({ apis: ['Date'] })
    t.mock.timers.tick(1731488301220) // 2024-11-13 16:58:21.220
})

test('test 1', async (t) => {
    await t.test('sub test 1', async (t) => {
        //
    })
})

2. Run the following command to test

node --test --experimental-default-type=module ./a.test.mjs

How often does it reproduce? Is there a required condition?

Every time

What is the expected behavior? Why is that the expected behavior?

There should be not panic

What do you see instead?

▶ test 1
  ✖ sub test 1 (1.0925ms)
    TypeError [Error]: Cannot redefine property: Symbol(MockTimers)
        at defineProperties (<anonymous>)
        at #createDate (node:internal/test_runner/mock/mock_timers:381:5)
        at Object.Date (node:internal/test_runner/mock/mock_timers:614:45)
        at node:internal/test_runner/mock/mock_timers:635:74
        at Array.forEach (<anonymous>)
        at #toggleEnableTimers (node:internal/test_runner/mock/mock_timers:635:5)
        at MockTimers.enable (node:internal/test_runner/mock/mock_timers:720:29)
        at TestContext.<anonymous> (file:///path/to/a.test.mjs:4:19)
        at TestHook.runInAsyncScope (node:async_hooks:211:14)
        at TestHook.run (node:internal/test_runner/test:934:25)

Additional information

No response

[avivkeller]

Even simpler reproduction:

import { test, beforeEach } from 'node:test'

beforeEach((t) => t.mock.timers.enable());

test(() => test());

$ node repro.js

It's failing here:

node/lib/internal/test_runner/mock/mock_timers.js

Lines 396 to 413 in be5a500

	ObjectDefineProperties(MockDate, {
	__proto__: null,
	[kMock]: {
	__proto__: null,
	enumerable: false,
	configurable: false,
	writable: false,
	value: this,
	},
	isMock: {
	__proto__: null,
	enumerable: true,
	configurable: false,
	writable: false,
	value: true,
	},
	});

[deokjinkim]

With above example, #createDate is called 2 times. So when I change configurable of [kMock] to true, above example is working. But I'm not sure that this approach is right.

node/lib/internal/test_runner/mock/mock_timers.js

Line 401 in be5a500

configurable: false,

[avivkeller]

I don't think we should set it to true.

IMO The appropriate response should be an error that the mock is already enabled, as is done if you try to enable it twice.

[avivkeller]

A better way to look at this is:

import { test } from 'node:test'

test((outer) => {
    outer.mock.timers.enable()
    test((inner) => inner.mock.timers.enable())
});

When outer changes the Date object, it sets these properties.

Then, when inner wants to change the Date object, it copies those properties, before trying to set them:

node/lib/internal/test_runner/mock/mock_timers.js

Lines 380 to 383 in be5a500

	ObjectDefineProperties(
	MockDate,
	dateProps,
	);

IMO there are two paths that can be taken here:
A) Throw an error that the object has already been mocked
B) Exclude those properties, and overwrite the mock.

I'm inclined for (A).

[avivkeller]

Opened #55858