test_runner: error on mocking an already mocked date

[RedYetiDev]

Fixes #55849

Previously, the properties from the global Date, including the not-writable isMock and [kMock] values, would be copied if the global Date was already mocked. This became an issue because the mock setup would attempt overwrite these properties.

Now, ERR_INVALID_STATE is thrown, as Date is already being mocked!.

Example

import { test } from 'node:test'

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

Before

✖ <anonymous> (0.536701ms)
  TypeError: Cannot redefine property: Symbol(MockTimers)
      at defineProperties (<anonymous>)
      at #createDate (node:internal/test_runner/mock/mock_timers:396:5)
      at Object.Date (node:internal/test_runner/mock/mock_timers:635:45)
      at node:internal/test_runner/mock/mock_timers:659:74
      at Array.forEach (<anonymous>)
      at #toggleEnableTimers (node:internal/test_runner/mock/mock_timers:659:5)
      at MockTimers.enable (node:internal/test_runner/mock/mock_timers:740:29)
      at TestContext.<anonymous> (file:///repro.js:5:39)
      at Test.runInAsyncScope (node:async_hooks:211:14)
      at Test.run (node:internal/test_runner/test:934:25)

After

✖ <anonymous> (0.573319ms)
  Error [ERR_INVALID_STATE]: Invalid state: Date is already being mocked!
      at #createDate (node:internal/test_runner/mock/mock_timers:332:13)
      at Object.Date (node:internal/test_runner/mock/mock_timers:638:45)
      at node:internal/test_runner/mock/mock_timers:662:74
      at Array.forEach (<anonymous>)
      at #toggleEnableTimers (node:internal/test_runner/mock/mock_timers:662:5)
      at MockTimers.enable (node:internal/test_runner/mock/mock_timers:743:29)
      at TestContext.<anonymous> (file:///repro.js:5:39)
      at Test.runInAsyncScope (node:async_hooks:211:14)
      at Test.run (node:internal/test_runner/test:934:25)
      at Test.start (node:internal/test_runner/test:833:17) {
    code: 'ERR_INVALID_STATE'
  }

[nodejs-github-bot]

Review requested:

• @nodejs/test_runner

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.41%. Comparing base (b52a49b) to head (52a34ef).
Report is 9 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #55858   +/-   ##
=======================================
  Coverage   88.41%   88.41%           
=======================================
  Files         654      654           
  Lines      187824   187827    +3     
  Branches    36134    36137    +3     
=======================================
+ Hits       166060   166064    +4     
+ Misses      15005    15003    -2     
- Partials     6759     6760    +1     

Files with missing lines	Coverage Δ
lib/internal/test_runner/mock/mock_timers.js	98.74% <100.00%> (+<0.01%)	⬆️

... and 33 files with indirect coverage changes

[JakobJingleheimer]

Is throwing necessary/helpful? If it's already mocked, why not just silently abort? (don't re-mock, but don't throw)

[RedYetiDev]

IMO throwing is helpful because in this case, we aren't the ones mocking.

If the user attempts to perform any other mocking utilities (such as restoring the mock), it won't work.

[JakobJingleheimer]

IMO throwing is helpful because in this case, we aren't the ones mocking.

If the user attempts to perform any other mocking utilities (such as restoring the mock), it won't work.

Ahh, good then :)

[cjihrig]

LGTM. I do think we should follow this up with a change to remove the screaming error messages.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/63576/