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

Implement node:test MockTracker #1855

Merged
merged 1 commit into from
Mar 20, 2024
Merged

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Mar 18, 2024

Requires the nodejs_compat compatibility flag to be enabled.

Does not yet support mock timers.

Does not implement the full node:test module, just the mocks

Refs: https://nodejs.org/docs/latest/api/test.html#class-mocktracker

Implementation was adapted directly from Node.js implementation. Only changes necessary to make it work in workers were made. Majority of new lines are the tests (which were also adapted directly from Node.js' tests). The API and implementation here are intended to match Node.js'.

While this could likely be polyfilled fairly easily, building it into the runtime allows us to take advantage of it for our own tests, much like we've started to do with node:assert

@jasnell jasnell requested review from a team as code owners March 18, 2024 16:56
@jasnell jasnell requested review from byule, hoodmane, fhanau and mrbbot March 18, 2024 16:56
@fhanau
Copy link
Collaborator

fhanau commented Mar 20, 2024

  • Does MockTimers need TS bindings?
  • MockTimers is Stability: 1 - Experimental and may have an unstable API – does this need to be guarded by e.g. an experimental flag or does nodejs_compat cover cases like this?
  • Tests look good, local mocks are auto restored after the test finishes wasn't adopted but I assume that's intentional.

LGTM otherwise, seems pretty helpful for developers to me.

@jasnell
Copy link
Member Author

jasnell commented Mar 20, 2024

I wasn't planning on implementing MockTimers for a while. I think just omitting those bits makes the most sense. I'll add todo comments.

Not porting that one test was intentional. The way it is structured it would need to be rewritten since wd-test has no concept of subtests/before/after etc so I wanted to look at it separately.

@jasnell jasnell force-pushed the jsnell/node-compat-mocktracker branch from 4919857 to ec775ff Compare March 20, 2024 14:13
Requires the `nodejs_compat` compatibility flag to be enabled.

Does not yet support mock timers.

Does not implement the full `node:test` module, just the mocks

Refs: https://nodejs.org/docs/latest/api/test.html#class-mocktracker
@jasnell jasnell force-pushed the jsnell/node-compat-mocktracker branch from ec775ff to 2091e3a Compare March 20, 2024 19:37
@jasnell jasnell merged commit fa43e8f into main Mar 20, 2024
9 of 10 checks passed
@fhanau fhanau deleted the jsnell/node-compat-mocktracker branch March 23, 2024 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants