Skip to content

Add non-mocked test for symlink #1308

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

Merged
merged 10 commits into from
Mar 22, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 1 addition & 1 deletion packages/hub/src/lib/download-file-to-cache-dir.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,6 @@ describe("downloadFileToCacheDir", () => {
// 2. should rename the incomplete to the blob expected name
expect(rename).toHaveBeenCalledWith(incomplete, expectedBlob);
// 3. should create symlink pointing to blob
expect(createSymlink).toHaveBeenCalledWith(expectedBlob, expectPointer);
expect(createSymlink).toHaveBeenCalledWith({ sourcePath: expectedBlob, finalPath: expectPointer });
});
});
4 changes: 2 additions & 2 deletions packages/hub/src/lib/download-file-to-cache-dir.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ export async function downloadFileToCacheDir(
// shortcut the download if needed
if (await exists(blobPath)) {
// create symlinks in snapshot folder to blob object
await createSymlink(blobPath, pointerPath);
await createSymlink({ sourcePath: blobPath, finalPath: pointerPath });
return pointerPath;
}

Expand All @@ -128,6 +128,6 @@ export async function downloadFileToCacheDir(
// rename .incomplete file to expect blob
await rename(incomplete, blobPath);
// create symlinks in snapshot folder to blob object
await createSymlink(blobPath, pointerPath);
await createSymlink({ sourcePath: blobPath, finalPath: pointerPath });
return pointerPath;
}
4 changes: 2 additions & 2 deletions packages/hub/src/utils/XetBlob.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ describe("XetBlob", () => {
}
return fetch(url, opts);
},
internalLogging: true,
// internalLogging: true,
});

const xetDownload = await blob.slice(0, 200_000).arrayBuffer();
Expand All @@ -124,7 +124,7 @@ describe("XetBlob", () => {
},
hash: "7b3b6d07673a88cf467e67c1f7edef1a8c268cbf66e9dd9b0366322d4ab56d9b",
size: 5_234_139_343,
internalLogging: true,
// internalLogging: true,
});

const xetDownload = await blob.slice(10_000_000, 10_100_000).arrayBuffer();
Expand Down
118 changes: 72 additions & 46 deletions packages/hub/src/utils/symlink.spec.ts
Original file line number Diff line number Diff line change
@@ -1,63 +1,89 @@
import { describe, test, expect, vi, beforeEach } from "vitest";
import * as fs from "node:fs/promises";
/* eslint-disable @typescript-eslint/consistent-type-imports */
/* eslint-disable @typescript-eslint/no-explicit-any */
import { describe, expect, it, vi } from "vitest";
import { createSymlink } from "./symlink";
import * as path from "node:path";
import type { FileHandle } from "node:fs/promises";

vi.mock("node:fs/promises", () => ({
rm: vi.fn(),
symlink: vi.fn(),
rename: vi.fn(),
copyFile: vi.fn(),
mkdir: vi.fn(),
mkdtemp: vi.fn(),
open: vi.fn(),
}));
import { readFileSync, writeFileSync } from "node:fs";
import { lstat, rm } from "node:fs/promises";
import { tmpdir } from "node:os";
import { join } from "node:path";

let failSymlink = false;
vi.mock("node:fs/promises", async (importOriginal) => ({
...(await importOriginal<typeof import("node:fs/promises")>()),
symlink: async (...args: any[]) => {
if (failSymlink) {
failSymlink = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure to get why we are changing the flag here

Copy link
Member Author

Choose a reason for hiding this comment

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

It's to reset the function once it's called. (so it doesn't impact other tests)

Could be done more elegantly

throw new Error("Symlink not supported");
}

vi.mock("node:os", () => ({
platform: vi.fn(),
// @ts-expect-error - ignore
return (await importOriginal<typeof import("node:fs/promises")>()).symlink(...args);
},
}));

describe("createSymlink", () => {
const src = "/path/to/src";
const dst = "/path/to/dst";
describe("utils/symlink", () => {
it("should create a symlink", async () => {
writeFileSync(join(tmpdir(), "test.txt"), "hello world");
await createSymlink({
sourcePath: join(tmpdir(), "test.txt"),
finalPath: join(tmpdir(), "test-symlink.txt"),
});

beforeEach(() => {
vi.resetAllMocks();
vi.mocked(fs.mkdtemp).mockImplementation(async (prefix) => `${prefix}/temp`);
vi.mocked(fs.open).mockResolvedValue({ close: vi.fn() } as unknown as FileHandle);
});
const stats = await lstat(join(tmpdir(), "test-symlink.txt"));
expect(stats.isSymbolicLink()).toBe(process.platform !== "win32");

test("should remove existing destination", async () => {
await createSymlink(dst, src);
expect(fs.rm).toHaveBeenCalledWith(dst);
// Test file content
const content = readFileSync(join(tmpdir(), "test-symlink.txt"), "utf8");
expect(content).toBe("hello world");

// Cleanup
await rm(join(tmpdir(), "test-symlink.txt"));
await rm(join(tmpdir(), "test.txt"));
});

describe("symlink not supported (Windows)", () => {
beforeEach(() => {
vi.mocked(fs.symlink).mockRejectedValue(new Error("Symlink not supported"));
it("should work when symlinking twice", async () => {
writeFileSync(join(tmpdir(), "test.txt"), "hello world");
writeFileSync(join(tmpdir(), "test2.txt"), "hello world2");
await createSymlink({
sourcePath: join(tmpdir(), "test.txt"),
finalPath: join(tmpdir(), "test-symlink.txt"),
});

test("should copyfile", async () => {
await createSymlink(dst, src);
expect(fs.copyFile).toHaveBeenCalledWith(path.resolve(dst), path.resolve(src));
await createSymlink({
sourcePath: join(tmpdir(), "test2.txt"),
finalPath: join(tmpdir(), "test-symlink.txt"),
});

test("should rename file if new_blob is true", async () => {
await createSymlink(dst, src, true);
expect(fs.rename).toHaveBeenCalledWith(path.resolve(dst), path.resolve(src));
});
const stats = await lstat(join(tmpdir(), "test-symlink.txt"));
expect(stats.isSymbolicLink()).toBe(process.platform !== "win32");

// Test file content
const content = readFileSync(join(tmpdir(), "test-symlink.txt"), "utf8");
expect(content).toBe("hello world2");

// Cleanup
await rm(join(tmpdir(), "test-symlink.txt"));
await rm(join(tmpdir(), "test.txt"));
await rm(join(tmpdir(), "test2.txt"));
});

describe("symlink supported", () => {
test("should symlink", async () => {
await createSymlink(dst, src);
expect(fs.symlink).toHaveBeenCalledWith(path.resolve(dst), path.resolve(src));
});
it("should work when symlink doesn't work (windows)", async () => {
writeFileSync(join(tmpdir(), "test.txt"), "hello world");

test("should symlink if new_blob is true", async () => {
await createSymlink(dst, src, true);
expect(fs.symlink).toHaveBeenCalledWith(path.resolve(dst), path.resolve(src));
failSymlink = true;
await createSymlink({
sourcePath: join(tmpdir(), "test.txt"),
finalPath: join(tmpdir(), "test-symlink.txt"),
});

const stats = await lstat(join(tmpdir(), "test-symlink.txt"));
expect(stats.isSymbolicLink()).toBe(false);

// Test file content
const content = readFileSync(join(tmpdir(), "test-symlink.txt"), "utf8");
expect(content).toBe("hello world");

// Cleanup
await rm(join(tmpdir(), "test-symlink.txt"));
await rm(join(tmpdir(), "test.txt"));
});
});
38 changes: 20 additions & 18 deletions packages/hub/src/utils/symlink.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,28 +36,30 @@ function expandUser(path: string): string {
* having the actual file in `dst`. If it is a new file (`new_blob=True`), we move it to `dst`. If it is not a new file
* (`new_blob=False`), we don't know if the blob file is already referenced elsewhere. To avoid breaking existing
* cache, the file is duplicated on the disk.
*
* In case symlinks are not supported, a warning message is displayed to the user once when loading `huggingface_hub`.
* The warning message can be disabled with the `HF_HUB_DISABLE_SYMLINKS_WARNING` environment variable.
*/
export async function createSymlink(dst: string, src: string, new_blob?: boolean): Promise<void> {
export async function createSymlink(params: {
/**
* The path to the symlink.
*/
finalPath: string;
/**
* The path the symlink should point to.
*/
sourcePath: string;
}): Promise<void> {
const abs_src = path.resolve(expandUser(params.sourcePath));
const abs_dst = path.resolve(expandUser(params.finalPath));

try {
await fs.rm(dst);
} catch (_e: unknown) {
/* empty */
await fs.rm(abs_dst);
} catch {
// ignore
}
const abs_src = path.resolve(expandUser(src));
const abs_dst = path.resolve(expandUser(dst));

try {
await fs.symlink(abs_dst, abs_src);
} catch (_e: unknown) {
if (new_blob) {
console.info(`Symlink not supported. Moving file from ${abs_src} to ${abs_dst}`);
await fs.rename(abs_dst, abs_src);
} else {
console.info(`Symlink not supported. Copying file from ${abs_src} to ${abs_dst}`);
await fs.copyFile(abs_dst, abs_src);
}
await fs.symlink(path.relative(path.dirname(abs_dst), abs_src), abs_dst);
} catch {
console.info(`Symlink not supported. Copying file from ${abs_src} to ${abs_dst}`);
await fs.copyFile(abs_src, abs_dst);
}
}
2 changes: 1 addition & 1 deletion packages/hub/vitest.config.mts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@ import { defineConfig } from "vitest/config";

export default defineConfig({
test: {
testTimeout: 30_000,
testTimeout: 60_000,
},
});