From 108fb8eb3641e9f4050f34e426f98a0662377f07 Mon Sep 17 00:00:00 2001 From: Sosuke Suzuki Date: Thu, 28 Aug 2025 10:20:06 +0900 Subject: [PATCH 1/2] Add tests for returned promises by `node:fs/promises` --- .../fs/fs-promises-returns-promise.test.ts | 188 ++++++++++++++++++ 1 file changed, 188 insertions(+) create mode 100644 test/js/node/fs/fs-promises-returns-promise.test.ts diff --git a/test/js/node/fs/fs-promises-returns-promise.test.ts b/test/js/node/fs/fs-promises-returns-promise.test.ts new file mode 100644 index 000000000000..a377ac53ed95 --- /dev/null +++ b/test/js/node/fs/fs-promises-returns-promise.test.ts @@ -0,0 +1,188 @@ +import { describe, expect, it } from "bun:test"; +import { tempDirWithFiles } from "harness"; +import * as promises from "node:fs/promises"; +import path from "node:path"; + +describe("fs.promises functions return proper Promise instances", () => { + it("should return Promise instances for asyncWrap functions", async () => { + const dir = tempDirWithFiles("fs-promises-test", { + "test.txt": "hello world", + "another.txt": "test content", + }); + + const testFile = path.join(dir, "test.txt"); + const nonExistentFile = path.join(dir, "nonexistent.txt"); + const tempFile = path.join(dir, "temp.txt"); + + // Test functions that use asyncWrap and should return proper Promise instances + const tests = [ + // File operations + { name: "readFile", fn: () => promises.readFile(testFile) }, + { name: "writeFile", fn: () => promises.writeFile(tempFile, "new content") }, + { name: "appendFile", fn: () => promises.appendFile(testFile, " appended") }, + { name: "access", fn: () => promises.access(testFile) }, + { name: "stat", fn: () => promises.stat(testFile) }, + { name: "lstat", fn: () => promises.lstat(testFile) }, + + // Directory operations + { name: "readdir", fn: () => promises.readdir(dir) }, + { name: "mkdir", fn: () => promises.mkdir(path.join(dir, "new-dir")) }, + + // File manipulation + { name: "copyFile", fn: () => promises.copyFile(testFile, path.join(dir, "copy.txt")) }, + { name: "rename", fn: () => promises.rename(path.join(dir, "copy.txt"), path.join(dir, "renamed.txt")) }, + { name: "truncate", fn: () => promises.truncate(testFile, 5) }, + { name: "unlink", fn: () => promises.unlink(path.join(dir, "renamed.txt")) }, + + // Operations that may fail but should still return Promise + { name: "rm", fn: () => promises.rm(nonExistentFile), shouldFail: true }, + { name: "rmdir", fn: () => promises.rmdir(nonExistentFile), shouldFail: true }, + { name: "readlink", fn: () => promises.readlink(testFile), shouldFail: true }, // Not a symlink + ]; + + for (const test of tests) { + try { + const result = test.fn(); + + // Check that the result is a Promise instance + expect(result instanceof Promise).toBe(true); + expect(typeof result.then).toBe("function"); + expect(typeof result.catch).toBe("function"); + expect(typeof result.finally).toBe("function"); + expect(result.constructor.name).toBe("Promise"); + + // Actually await the promise to ensure it works + if (test.shouldFail) { + await expect(result).rejects.toThrow(); + } else { + await result; + } + } catch (error) { + throw new Error(`Test for ${test.name} failed: ${error}`); + } + } + }); + + it("should return Promise for open() which creates FileHandle", async () => { + const dir = tempDirWithFiles("fs-promises-open-test", { + "test.txt": "hello world", + }); + + const testFile = path.join(dir, "test.txt"); + const openResult = promises.open(testFile, "r"); + + expect(openResult instanceof Promise).toBe(true); + expect(typeof openResult.then).toBe("function"); + expect(openResult.constructor.name).toBe("Promise"); + + // Test that it resolves to a FileHandle + const fileHandle = await openResult; + expect(typeof fileHandle.close).toBe("function"); + expect(typeof fileHandle.read).toBe("function"); + expect(typeof fileHandle.write).toBe("function"); + + await fileHandle.close(); + }); + + it("should return Promise for exists() function", async () => { + const dir = tempDirWithFiles("fs-promises-exists-test", { + "exists.txt": "I exist", + }); + + const existingFile = path.join(dir, "exists.txt"); + const nonExistentFile = path.join(dir, "does-not-exist.txt"); + + // Test with existing file + const existsResult1 = promises.exists(existingFile); + expect(existsResult1 instanceof Promise).toBe(true); + expect(existsResult1.constructor.name).toBe("Promise"); + expect(await existsResult1).toBe(true); + + // Test with non-existent file + const existsResult2 = promises.exists(nonExistentFile); + expect(existsResult2 instanceof Promise).toBe(true); + expect(existsResult2.constructor.name).toBe("Promise"); + expect(await existsResult2).toBe(false); + }); + + it("should return Promise for functions with file descriptor operations", async () => { + const dir = tempDirWithFiles("fs-promises-fd-test", { + "test.txt": "hello world for fd test", + }); + + const testFile = path.join(dir, "test.txt"); + const fileHandle = await promises.open(testFile, "r+"); + + try { + // Test fd-based operations that use asyncWrap + const fdTests = [ + { name: "fstat", fn: () => promises.fstat(fileHandle.fd) }, + { name: "fchmod", fn: () => promises.fchmod(fileHandle.fd, 0o644) }, + { name: "fchown", fn: () => promises.fchown(fileHandle.fd, process.getuid?.() ?? 0, process.getgid?.() ?? 0) }, + { name: "fsync", fn: () => promises.fsync(fileHandle.fd) }, + { name: "fdatasync", fn: () => promises.fdatasync(fileHandle.fd) }, + { name: "ftruncate", fn: () => promises.ftruncate(fileHandle.fd, 5) }, + ]; + + for (const test of fdTests) { + try { + const result = test.fn(); + expect(result instanceof Promise).toBe(true); + expect(result.constructor.name).toBe("Promise"); + await result; + } catch (error) { + // Some operations might fail due to permissions, but should still return Promise + console.warn(`${test.name} failed (expected on some systems): ${error}`); + } + } + } finally { + await fileHandle.close(); + } + }); + + it("regression test: fs.promises.rm should return Promise instance, not InternalPromise", async () => { + // This is the specific bug that was fixed - rm was returning InternalPromise instead of Promise + const nonExistentFile = path.join(__dirname, "definitely-does-not-exist-" + Math.random()); + + const rmResult = promises.rm(nonExistentFile); + + // The main assertion - this was returning false before the fix + expect(rmResult instanceof Promise).toBe(true); + expect(rmResult.constructor.name).toBe("Promise"); + expect(typeof rmResult.then).toBe("function"); + expect(typeof rmResult.catch).toBe("function"); + expect(typeof rmResult.finally).toBe("function"); + + // Should reject with ENOENT error + await expect(rmResult).rejects.toThrow(); + }); + + it("should have correct function names without exposing internal implementation", () => { + // Ensure function names don't leak internal implementation details + expect(promises.rm.name).toBe("rm"); + expect(promises.readFile.name).toBe("readFile"); + expect(promises.writeFile.name).toBe("writeFile"); + expect(promises.appendFile.name).toBe("appendFile"); + expect(promises.exists.name).toBe("exists"); + expect(promises.open.name).toBe("open"); + expect(promises.access.name).toBe("access"); + expect(promises.stat.name).toBe("stat"); + expect(promises.readdir.name).toBe("readdir"); + expect(promises.mkdir.name).toBe("mkdir"); + expect(promises.copyFile.name).toBe("copyFile"); + expect(promises.rename.name).toBe("rename"); + expect(promises.unlink.name).toBe("unlink"); + expect(promises.truncate.name).toBe("truncate"); + expect(promises.chmod.name).toBe("chmod"); + expect(promises.chown.name).toBe("chown"); + + // Ensure no function names start with "defaultAsync" which would expose internal implementation + const functionNames = Object.getOwnPropertyNames(promises) + .filter(name => typeof promises[name] === "function") + .map(name => promises[name].name); + + for (const name of functionNames) { + expect(name).not.toMatch(/^defaultAsync/); + } + }); +}); \ No newline at end of file From cbb186328da75a9d1891b258dc0c8a802ddc4fc5 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Thu, 28 Aug 2025 02:16:45 +0000 Subject: [PATCH 2/2] [autofix.ci] apply automated fixes --- .../fs/fs-promises-returns-promise.test.ts | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/test/js/node/fs/fs-promises-returns-promise.test.ts b/test/js/node/fs/fs-promises-returns-promise.test.ts index a377ac53ed95..80b313067f6a 100644 --- a/test/js/node/fs/fs-promises-returns-promise.test.ts +++ b/test/js/node/fs/fs-promises-returns-promise.test.ts @@ -23,17 +23,17 @@ describe("fs.promises functions return proper Promise instances", () => { { name: "access", fn: () => promises.access(testFile) }, { name: "stat", fn: () => promises.stat(testFile) }, { name: "lstat", fn: () => promises.lstat(testFile) }, - + // Directory operations { name: "readdir", fn: () => promises.readdir(dir) }, { name: "mkdir", fn: () => promises.mkdir(path.join(dir, "new-dir")) }, - + // File manipulation { name: "copyFile", fn: () => promises.copyFile(testFile, path.join(dir, "copy.txt")) }, { name: "rename", fn: () => promises.rename(path.join(dir, "copy.txt"), path.join(dir, "renamed.txt")) }, { name: "truncate", fn: () => promises.truncate(testFile, 5) }, { name: "unlink", fn: () => promises.unlink(path.join(dir, "renamed.txt")) }, - + // Operations that may fail but should still return Promise { name: "rm", fn: () => promises.rm(nonExistentFile), shouldFail: true }, { name: "rmdir", fn: () => promises.rmdir(nonExistentFile), shouldFail: true }, @@ -43,14 +43,14 @@ describe("fs.promises functions return proper Promise instances", () => { for (const test of tests) { try { const result = test.fn(); - + // Check that the result is a Promise instance expect(result instanceof Promise).toBe(true); expect(typeof result.then).toBe("function"); expect(typeof result.catch).toBe("function"); expect(typeof result.finally).toBe("function"); expect(result.constructor.name).toBe("Promise"); - + // Actually await the promise to ensure it works if (test.shouldFail) { await expect(result).rejects.toThrow(); @@ -143,16 +143,16 @@ describe("fs.promises functions return proper Promise instances", () => { it("regression test: fs.promises.rm should return Promise instance, not InternalPromise", async () => { // This is the specific bug that was fixed - rm was returning InternalPromise instead of Promise const nonExistentFile = path.join(__dirname, "definitely-does-not-exist-" + Math.random()); - + const rmResult = promises.rm(nonExistentFile); - + // The main assertion - this was returning false before the fix expect(rmResult instanceof Promise).toBe(true); expect(rmResult.constructor.name).toBe("Promise"); expect(typeof rmResult.then).toBe("function"); expect(typeof rmResult.catch).toBe("function"); expect(typeof rmResult.finally).toBe("function"); - + // Should reject with ENOENT error await expect(rmResult).rejects.toThrow(); }); @@ -175,14 +175,14 @@ describe("fs.promises functions return proper Promise instances", () => { expect(promises.truncate.name).toBe("truncate"); expect(promises.chmod.name).toBe("chmod"); expect(promises.chown.name).toBe("chown"); - + // Ensure no function names start with "defaultAsync" which would expose internal implementation const functionNames = Object.getOwnPropertyNames(promises) .filter(name => typeof promises[name] === "function") .map(name => promises[name].name); - + for (const name of functionNames) { expect(name).not.toMatch(/^defaultAsync/); } }); -}); \ No newline at end of file +});