Update odsp-driver package tests to run in esm instead of cjs #23728
+134
−40
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
area: driver
There was a problem hiding this comment.
Copilot reviewed 5 out of 15 changed files in this pull request and generated no comments.
Files not reviewed (10)
- packages/drivers/odsp-driver/package.json: Language not supported
- packages/drivers/odsp-driver/src/test/joinSessionCacheTests.spec.ts: Evaluated as low risk
- packages/drivers/odsp-driver/src/test/fetchSnapshot.spec.ts: Evaluated as low risk
- packages/drivers/odsp-driver/src/test/socketTests/socketTests.spec.ts: Evaluated as low risk
- packages/drivers/odsp-driver/src/test/joinSessionPeriodicCall.spec.ts: Evaluated as low risk
- packages/drivers/odsp-driver/src/test/odspDriverUrlResolverForShareLink.spec.ts: Evaluated as low risk
- packages/drivers/odsp-driver/src/test/socketTests/deltaConnectionUpdateTests.spec.ts: Evaluated as low risk
- packages/drivers/odsp-driver/src/odspUtils.ts: Evaluated as low risk
- packages/drivers/odsp-driver/src/test/mockFetch.ts: Evaluated as low risk
- packages/drivers/odsp-driver/src/vroom.ts: Evaluated as low risk
Comments suppressed due to low confidence (1)
packages/drivers/odsp-driver/src/fetchSnapshot.ts:697
- Ensure that the changes to use mockify for downloadSnapshot are covered by tests.
export const downloadSnapshot = mockify(
noencke
force-pushed
the
odsp-driver-esm-tests
branch
from
603cbfe to
d952b82
Compare
noencke
commented
Feb 1, 2025
| @@ -101,6 +102,9 @@ export async function getFileLink( | |||
| return fileLink; | |||
| } | |||
|
|
|||
| const mockableGetFileLink = mockify(getFileLink); | |||
| export { mockableGetFileLink as getFileLink }; | |||
There was a problem hiding this comment.
You can also inline mockify when defining the original function, but for this PR it makes the diff really unreadable.
noencke
force-pushed
the
odsp-driver-esm-tests
branch
from
d952b82 to
15d8a18
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This required updating a few places where functions were being mocked by the sinon library.
The sinon
stub()function mocks a given function on a given object. For top-level functions, this meant that the exporting module object's reference to the function was being replaced with the stub. This is no longer permitted in ES6 since module objects are readonly.To solve this, there is now a helper function
mockify()which can decorate a function with a property that allows it to be easily mocked by sinon. All the problematic functions and their mock sites have been updated to use this helper. One function,fetchHelperwas already a wrapper aroundfetch(which was being mocked) - so it has been updated to use the same pattern but in a slightly different way.This PR updates the package test scripts to build and run in ESM rather than CJS.
It also had to replace two usages of the CJS-supported
__dirnamewith the equivalent code based onimport.meta.urlin order to build in ESM.