Add non-mocked test for symlink (#1308)

Alternative to #1307

Fix #1306 

I was just too confused by the src/dst params, nodej's official docs
could be clearer too.

Now the params are explictly named and there is a real test to check the
filesystem contents.

By the way, it's a `copy` operation not a `rename` (because it was
already the case, the `new_blob` param was never passed)

Also took the opportunty to use relative symlinks cc @Wauplin for better
windows support

cc @jeffmaury @axel7083

Author: coyotte508

packages/hub/src/lib/download-file-to-cache-dir.spec.ts

@@ -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 });
 	});
 });

packages/hub/src/lib/download-file-to-cache-dir.ts

@@ -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;
 	}
 
@@ -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;
 }

packages/hub/src/utils/XetBlob.spec.ts

@@ -98,7 +98,7 @@ describe("XetBlob", () => {
 					}
 					return fetch(url, opts);
 				},
-				internalLogging: true,
+				// internalLogging: true,
 			});
 
 			const xetDownload = await blob.slice(0, 200_000).arrayBuffer();
@@ -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();

packages/hub/src/utils/symlink.spec.ts

@@ -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;
+			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"));
 	});
 });

packages/hub/src/utils/symlink.ts

@@ -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);
 	}
 }

packages/hub/vitest.config.mts

@@ -2,6 +2,6 @@ import { defineConfig } from "vitest/config";
 
 export default defineConfig({
 	test: {
-		testTimeout: 30_000,
+		testTimeout: 60_000,
 	},
 });