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

test: replace esmock with sinon #117

Merged
merged 2 commits into from
May 17, 2024
Merged

test: replace esmock with sinon #117

merged 2 commits into from
May 17, 2024

Conversation

fasttime
Copy link
Member

@fasttime fasttime commented May 17, 2024

Fix tests in Node.js 22.2.0 by using sinon to mock dependencies instead of esmock.

Fixes #116

@fasttime fasttime marked this pull request as ready for review May 17, 2024 18:43
error: logErrorStub
}
});
sinon.replaceGetter(log, "error", () => logErrorStub);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This replaces a property on the module namespace object?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, do you know of a better way to do that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, I just don't understand how it can work, shouldn't module namespace objects be frozen?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that Vitest does some kind of bundling or transpiling, which is also why the unit tests cannot be debugged.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it seems that when running through Vitest this isn't a real namespace object.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@mdjermanovic mdjermanovic merged commit 3987d0f into main May 17, 2024
11 checks passed
@mdjermanovic mdjermanovic deleted the test-replace-esmock branch May 17, 2024 20:48
@github-actions github-actions bot mentioned this pull request May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some tests are failing on latest Node v22.2.0
2 participants