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

Update odsp-driver package tests to build and run in ESM #23728

Merged
merged 4 commits into from
Feb 7, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions packages/drivers/odsp-driver/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,7 @@
"build:docs": "api-extractor run --local",
"build:esnext": "tsc --project ./tsconfig.json",
"build:genver": "gen-version",
"build:test": "npm run build:test:esm && npm run build:test:cjs",
"build:test:cjs": "fluid-tsc commonjs --project ./src/test/tsconfig.cjs.json",
"build:test": "npm run build:test:esm",
noencke marked this conversation as resolved.
Show resolved Hide resolved
"build:test:esm": "tsc --project ./src/test/tsconfig.json",
"check:are-the-types-wrong": "attw --pack .",
"check:biome": "biome check .",
Expand All @@ -86,8 +85,7 @@
"lint:fix": "fluid-build . --task eslint:fix --task format",
"test": "npm run test:mocha",
"test:coverage": "c8 npm test",
"test:mocha": "npm run test:mocha:cjs && echo \"ADO #7404 - ESM modules cannot be stubbed - npm run test:mocha:esm\"",
"test:mocha:cjs": "mocha --recursive \"dist/test/**/*.spec.*js\" --exit",
"test:mocha": "npm run test:mocha:esm",
"test:mocha:esm": "mocha --recursive \"lib/test/**/*.spec.*js\" --exit",
"test:mocha:verbose": "cross-env FLUID_TEST_VERBOSE=1 npm run test:mocha",
"tsc": "fluid-tsc commonjs --project ./tsconfig.cjs.json && copyfiles -f ../../../common/build/build-common/src/cjs/package.json ./dist",
Expand Down
6 changes: 5 additions & 1 deletion packages/drivers/odsp-driver/src/fetchSnapshot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import {
import { EpochTracker } from "./epochTracker.js";
import { getQueryString } from "./getQueryString.js";
import { getHeadersWithAuth } from "./getUrlAndHeadersWithAuth.js";
import { mockify } from "./mockify.js";
import { convertOdspSnapshotToSnapshotTreeAndBlobs } from "./odspSnapshotParser.js";
import { checkForKnownServerFarmType } from "./odspUrlHelper.js";
import {
Expand Down Expand Up @@ -693,7 +694,7 @@ function getTreeStatsCore(snapshotTree: ISnapshotTree, stats: ITreeStats): void
* @param epochTracker - epoch tracker used to add/validate epoch in the network call.
* @returns fetched snapshot.
*/
export async function downloadSnapshot(
async function downloadSnapshot(
odspResolvedUrl: IOdspResolvedUrl,
getAuthHeader: InstrumentedStorageTokenFetcher,
tokenFetchOptions: TokenFetchOptionsEx,
Expand Down Expand Up @@ -775,6 +776,9 @@ export async function downloadSnapshot(
};
}

const mockableDownloadSnapshot = mockify(downloadSnapshot);
export { mockableDownloadSnapshot as downloadSnapshot };
Copy link
Contributor

Choose a reason for hiding this comment

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

this pattern breaks IntelliSense for the function, and also makes it easy for any code using the function in this package to call either one (and isn't super clear which one should be called.

I think it would be better to avoid having two separate functions in scope here: if you want all callers to use this version, I think a decorator would be a better way to wrap it as that would keep the non-wrapped version out of scope and allows the IntelliSense/docs to function properly. It would also be more concise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, as I mentioned in this comment, I can just inline it instead, so there is no rename+export. I like that better anyway, it just ruined the diff for this PR.

When you say decorator do you mean an actual TS decorator? I was under the impression that you can only write those for class members, not free functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently, you are right, they don't work on free functions.

If you don't inline them, I think you should nave the version you won't want people using something like fooInner instead of naming the one you do want people to use mockableFoo. Also leave the doc comments on the actually exported one (and maybe link or inherit doc the inner one to it).

I do think inlining it would be better, though refactoring things do we don't need mocking in the first place would be best: adding testing only hooks to production code is always a bit sketchy as it implies the code itself isn't flexible enough to do what tests need which is generally a bad sign.

Copy link
Contributor Author

@noencke noencke Feb 3, 2025

Choose a reason for hiding this comment

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

You're right that renaming the inner helper would be better than renaming the outer exported function (because otherwise, code calling the function from within its module of declaration will accidentally use the wrong one). However, I just went ahead and in-lined the calls. The diff in this PR now looks worse, but the actual code looks better :)

I also took a look at refactoring so that e.g. fetch is injected rather than mocked. It's of course possible, but not trivial, and is beyond the scope of this PR IMO.


function isRedeemSharingLinkError(
odspResolvedUrl: IOdspResolvedUrl,
error: Partial<IOdspError>,
Expand Down
6 changes: 5 additions & 1 deletion packages/drivers/odsp-driver/src/getFileLink.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
} from "@fluidframework/telemetry-utils/internal";

import { getHeadersWithAuth } from "./getUrlAndHeadersWithAuth.js";
import { mockify } from "./mockify.js";
import {
fetchHelper,
getWithRetryForTokenRefresh,
Expand All @@ -42,7 +43,7 @@ const fileLinkCache = new Map<string, Promise<string>>();
* @param logger - used to log results of operation, including any error
* @returns Promise which resolves to file link url when successful; otherwise, undefined.
*/
export async function getFileLink(
async function getFileLink(
getToken: TokenFetcher<OdspResourceTokenFetchOptions>,
resolvedUrl: IOdspResolvedUrl,
logger: ITelemetryLoggerExt,
Expand Down Expand Up @@ -101,6 +102,9 @@ export async function getFileLink(
return fileLink;
}

const mockableGetFileLink = mockify(getFileLink);
export { mockableGetFileLink as getFileLink };
noencke marked this conversation as resolved.
Show resolved Hide resolved

/**
* Handles location redirection while fulfilling the getFileLink call. We don't want browser to handle
* the redirection as the browser will retry with same auth token which will not work as we need app
Expand Down
67 changes: 67 additions & 0 deletions packages/drivers/odsp-driver/src/mockify.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/*!
* Copyright (c) Microsoft Corporation and contributors. All rights reserved.
* Licensed under the MIT License.
*/

/**
* A special key used to store the original function in a {@link Mockable | mockable} function.
* @remarks Use {@link mockify | `mockify.key`} as a convenient way to access this key.
*/
export const mockifyMockKey = Symbol("`mockify` mock function key");

/**
* A function that can be mocked after being decorated by {@link mockify | mockify()}.
*/
export interface Mockable<T extends (...args: any[]) => unknown> {
(...args: Parameters<T>): ReturnType<T>;
[mockifyMockKey]: T;
}

/**
* Decorates a function to allow it to be mocked.
* @param fn - The function that will become mockable.
* @returns A function with a {@link mockifyMockKey | special property } that can be overwritten to mock the original function.
* By default, this property is set to the original function.
* If overwritten with a new function, the new function will be called instead of the original.
* @example
* ```typescript
* const original = () => console.log("original");
* const mockable = mockify(original);
* mockable(); // logs "original"
* mockable[mockify.key] = () => console.log("mocked");
* mockable(); // logs "mocked"
* mockable[mockify.key] = original;
* mockable(); // logs "original"
* ```
*
* This pattern is useful for mocking top-level exported functions in a module.
* For example,
* ```typescript
* export function fn() { /* ... * / }
* ```
* becomes
* ```typescript
* import { mockify } from "./mockify.js";
* export const fn = mockify(() => { /* ... * / });
* ```
* and can now be mocked by another module that imports it.
* ```typescript
* import * as sinon from "sinon";
* import { mockify } from "./mockify.js";
* import { fn } from "./module.js";
* sinon.stub(fn, mockify.key).callsFake(() => {
* // ... mock function implementation ...
* });
* // ...
* sinon.restore();
* ```
*/
export function mockify<T extends (...args: any[]) => unknown>(fn: T): Mockable<T> {
const mockable = (...args: Parameters<T>): ReturnType<T> => {
return mockable[mockifyMockKey](...args) as ReturnType<T>;
};
mockable[mockifyMockKey] = fn;
return mockable;
}

mockify.key = mockifyMockKey;
4 changes: 3 additions & 1 deletion packages/drivers/odsp-driver/src/odspUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ export async function fetchHelper(
const start = performanceNow();

// Node-fetch and dom have conflicting typing, force them to work by casting for now
return fetch(requestInfo, requestInit).then(
return fetchHelper.fetch(requestInfo, requestInit).then(
CraigMacomber marked this conversation as resolved.
Show resolved Hide resolved
async (fetchResponse) => {
const response = fetchResponse as unknown as Response;
// Let's assume we can retry.
Expand Down Expand Up @@ -221,6 +221,8 @@ export async function fetchHelper(
},
);
}
// This allows `fetch` to be mocked (e.g. with sinon `stub()`)
fetchHelper.fetch = fetch;

/**
* A utility function to fetch and parse as JSON with support for retries
Expand Down
6 changes: 3 additions & 3 deletions packages/drivers/odsp-driver/src/socketModule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,6 @@

import { io } from "socket.io-client";

// Import is required for side-effects.
// eslint-disable-next-line unicorn/prefer-export-from
export const SocketIOClientStatic = io;
import { mockify, type Mockable } from "./mockify.js";

export const SocketIOClientStatic: Mockable<typeof io> = mockify(io);
18 changes: 9 additions & 9 deletions packages/drivers/odsp-driver/src/test/fetchSnapshot.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ import { convertToCompactSnapshot } from "../compactSnapshotWriter.js";
import { HostStoragePolicyInternal } from "../contracts.js";
import { createOdspUrl } from "../createOdspUrl.js";
import { EpochTracker } from "../epochTracker.js";
import * as fetchSnapshotImport from "../fetchSnapshot.js";
import { ISnapshotRequestAndResponseOptions } from "../fetchSnapshot.js";
import { downloadSnapshot, ISnapshotRequestAndResponseOptions } from "../fetchSnapshot.js";
import { mockify } from "../mockify.js";
import { LocalPersistentCache, NonPersistentCache } from "../odspCache.js";
import { OdspDocumentStorageService } from "../odspDocumentStorageManager.js";
import { OdspDriverUrlResolver } from "../odspDriverUrlResolver.js";
Expand Down Expand Up @@ -139,7 +139,7 @@ describe("Tests1 for snapshot fetch", () => {
_response: Promise<ISnapshotRequestAndResponseOptions>,
callback: () => Promise<T>,
): Promise<T> {
const getDownloadSnapshotStub = stub(fetchSnapshotImport, "downloadSnapshot");
const getDownloadSnapshotStub = stub(downloadSnapshot, mockify.key);
getDownloadSnapshotStub.returns(_response);
try {
return await callback();
Expand Down Expand Up @@ -181,7 +181,7 @@ describe("Tests1 for snapshot fetch", () => {
_response: Promise<ISnapshotRequestAndResponseOptions>,
callback: () => Promise<T>,
): Promise<T> {
const getDownloadSnapshotStub = stub(fetchSnapshotImport, "downloadSnapshot");
const getDownloadSnapshotStub = stub(downloadSnapshot, mockify.key);
getDownloadSnapshotStub.returns(_response);
try {
return await callback();
Expand Down Expand Up @@ -222,7 +222,7 @@ describe("Tests1 for snapshot fetch", () => {
_response: Promise<ISnapshotRequestAndResponseOptions>,
callback: () => Promise<T>,
): Promise<T> {
const getDownloadSnapshotStub = stub(fetchSnapshotImport, "downloadSnapshot");
const getDownloadSnapshotStub = stub(downloadSnapshot, mockify.key);
getDownloadSnapshotStub.returns(_response);
try {
return await callback();
Expand Down Expand Up @@ -270,7 +270,7 @@ describe("Tests1 for snapshot fetch", () => {
_response: Promise<ISnapshotRequestAndResponseOptions>,
callback: () => Promise<T>,
): Promise<T> {
const getDownloadSnapshotStub = stub(fetchSnapshotImport, "downloadSnapshot");
const getDownloadSnapshotStub = stub(downloadSnapshot, mockify.key);
getDownloadSnapshotStub.returns(_response);
try {
return await callback();
Expand Down Expand Up @@ -336,7 +336,7 @@ describe("Tests1 for snapshot fetch", () => {
_response: Promise<ISnapshotRequestAndResponseOptions>,
callback: () => Promise<T>,
): Promise<T> {
const getDownloadSnapshotStub = stub(fetchSnapshotImport, "downloadSnapshot");
const getDownloadSnapshotStub = stub(downloadSnapshot, mockify.key);
getDownloadSnapshotStub.returns(_response);
try {
return await callback();
Expand Down Expand Up @@ -401,7 +401,7 @@ describe("Tests1 for snapshot fetch", () => {
_response: Promise<ISnapshotRequestAndResponseOptions>,
callback: () => Promise<T>,
): Promise<T> {
const getDownloadSnapshotStub = stub(fetchSnapshotImport, "downloadSnapshot");
const getDownloadSnapshotStub = stub(downloadSnapshot, mockify.key);
getDownloadSnapshotStub.returns(_response);
try {
return await callback();
Expand Down Expand Up @@ -455,7 +455,7 @@ describe("Tests1 for snapshot fetch", () => {
_response: Promise<ISnapshotRequestAndResponseOptions>,
callback: () => Promise<T>,
): Promise<T> {
const getDownloadSnapshotStub = stub(fetchSnapshotImport, "downloadSnapshot");
const getDownloadSnapshotStub = stub(downloadSnapshot, mockify.key);
getDownloadSnapshotStub.returns(_response);
try {
return await callback();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,12 @@ import { stub, type SinonStub } from "sinon";
import { Socket } from "socket.io-client";

import { createOdspUrl } from "../createOdspUrl.js";
import { mockify } from "../mockify.js";
import { OdspDocumentServiceFactory } from "../odspDocumentServiceFactory.js";
import { OdspDriverUrlResolver } from "../odspDriverUrlResolver.js";
import { getJoinSessionCacheKey } from "../odspUtils.js";
import * as socketModule from "../socketModule.js";
import * as joinSession from "../vroom.js";
import { SocketIOClientStatic } from "../socketModule.js";
import { fetchJoinSession } from "../vroom.js";

// eslint-disable-next-line import/no-internal-modules
import { ClientSocketMock } from "./socketTests/socketMock.js";
Expand Down Expand Up @@ -56,14 +57,14 @@ describe("expose joinSessionInfo Tests", () => {
);

function addJoinSessionStub(): SinonStub {
const joinSessionStub = stub(joinSession, "fetchJoinSession").callsFake(
const joinSessionStub = stub(fetchJoinSession, mockify.key).callsFake(
async () => joinSessionResponse,
);
return joinSessionStub;
}

async function mockSocket<T>(_response: Socket, callback: () => Promise<T>): Promise<T> {
const getSocketCreationStub = stub(socketModule, "SocketIOClientStatic");
const getSocketCreationStub = stub(SocketIOClientStatic, mockify.key);
getSocketCreationStub.returns(_response);
try {
return await callback();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,14 @@ import { SinonFakeTimers, SinonStub, stub, useFakeTimers } from "sinon";

import { OdspFluidDataStoreLocator } from "../contractsPublic.js";
import { createOdspUrl } from "../createOdspUrl.js";
import { mockify } from "../mockify.js";
import { LocalPersistentCache } from "../odspCache.js";
import * as odspDocumentDeltaConnection from "../odspDocumentDeltaConnection.js";
import { OdspDocumentDeltaConnection } from "../odspDocumentDeltaConnection.js";
import { OdspDocumentService } from "../odspDocumentService.js";
import { OdspDocumentServiceFactory } from "../odspDocumentServiceFactory.js";
import { OdspDriverUrlResolver } from "../odspDriverUrlResolver.js";
import * as joinSession from "../vroom.js";
import { fetchJoinSession } from "../vroom.js";

describe("joinSessions Tests", () => {
let clock: SinonFakeTimers;
Expand Down Expand Up @@ -65,7 +66,7 @@ describe("joinSessions Tests", () => {

function addJoinSessionStub(time: number): SinonStub {
joinSessionResponse.refreshSessionDurationSeconds = time;
const joinSessionStub = stub(joinSession, "fetchJoinSession").callsFake(
const joinSessionStub = stub(fetchJoinSession, mockify.key).callsFake(
async () => joinSessionResponse,
);
return joinSessionStub;
Expand Down
13 changes: 11 additions & 2 deletions packages/drivers/odsp-driver/src/test/localOdspDriver.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

import { strict as assert } from "node:assert";
import fs from "node:fs";
import { dirname } from "node:path";
import { fileURLToPath } from "node:url";

import { IClient, SummaryType } from "@fluidframework/driver-definitions";
import {
Expand Down Expand Up @@ -50,7 +52,7 @@ describe("Local Odsp driver", () => {
};

const localSnapshot = fs.readFileSync(
`${__dirname}/../../src/test/localSnapshots/localSnapshot1.json`,
`${getDirname()}/../../src/test/localSnapshots/localSnapshot1.json`,
{ encoding: "utf8" },
);

Expand Down Expand Up @@ -152,7 +154,7 @@ describe("Local Odsp driver", () => {

it("Delta storage service returns trailing ops", async () => {
const snapshotWithTrailingOps = fs.readFileSync(
`${__dirname}/../../src/test/localSnapshots/localSnapshot2.json`,
`${getDirname()}/../../src/test/localSnapshots/localSnapshot2.json`,
{ encoding: "utf8" },
);
const service = new LocalOdspDocumentService(
Expand Down Expand Up @@ -290,3 +292,10 @@ describe("Local Odsp driver", () => {
});
});
});

/**
* Retrieves the directory in which this module resides (equivalent to `__dirname` in CJS)
*/
function getDirname(): string {
return dirname(fileURLToPath(import.meta.url));
}
9 changes: 5 additions & 4 deletions packages/drivers/odsp-driver/src/test/mockFetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,11 @@

import assert from "node:assert";

import * as fetchModule from "node-fetch";
import { Headers } from "node-fetch";
import { stub } from "sinon";

import { fetchHelper } from "../odspUtils.js";

/**
* Mock response returned by {@link createResponse}.
*/
Expand Down Expand Up @@ -49,14 +50,14 @@ export async function mockFetchMultiple<T>(
responses: (() => Promise<object>)[],
type: FetchCallType = "single",
): Promise<T> {
const fetchStub = stub(fetchModule, "default");
const fetchStub = stub(fetchHelper, "fetch");
fetchStub.callsFake(async () => {
if (type === "external") {
fetchStub.restore();
}
const cb = responses.shift();
assert(cb !== undefined, "the end");
return cb() as Promise<fetchModule.Response>;
return cb() as Promise<Response>;
});
try {
return await callback();
Expand Down Expand Up @@ -89,7 +90,7 @@ export async function mockFetchError<T>(
response: Error,
type: FetchCallType = "single",
): Promise<T> {
const fetchStub = stub(fetchModule, "default");
const fetchStub = stub(fetchHelper, "fetch");
fetchStub.callsFake(async () => {
if (type === "external") {
fetchStub.restore();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ import { stub } from "sinon";
import { SharingLinkHeader } from "../contractsPublic.js";
import { createOdspCreateContainerRequest } from "../createOdspCreateContainerRequest.js";
import { createOdspUrl } from "../createOdspUrl.js";
import * as fileLinkImport from "../getFileLink.js";
import { getFileLink } from "../getFileLink.js";
import { mockify } from "../mockify.js";
import { OdspDriverUrlResolverForShareLink } from "../odspDriverUrlResolverForShareLink.js";
import {
getLocatorFromOdspUrl,
Expand Down Expand Up @@ -80,7 +81,7 @@ describe("Tests for OdspDriverUrlResolverForShareLink resolver", () => {
response: Promise<string>,
callback: () => Promise<T>,
): Promise<T> {
const getFileLinkStub = stub(fileLinkImport, "getFileLink");
const getFileLinkStub = stub(getFileLink, mockify.key);
getFileLinkStub.returns(response);
try {
return await callback();
Expand Down
Loading
Loading