tidy up implementation, fix tests, don't accept filehandle as logging…

….file because then it's impossible for us to now whether we should close it
trufflesuite · Mar 8, 2023 · e535671 · e535671
1 parent f8381cc
commit e535671
Show file tree

Hide file tree

Showing 8 changed files with 32 additions and 86 deletions.
diff --git a/src/chains/ethereum/console.log/tests/index.test.ts b/src/chains/ethereum/console.log/tests/index.test.ts
@@ -21,7 +21,7 @@ import {
   CONTRACT_NAME
 } from "./helpers";
 
-describe.skip("@ganache/console.log", () => {
+describe("@ganache/console.log", () => {
   const logger = {
     log: () => {}
   };

diff --git a/src/chains/ethereum/ethereum/src/provider.ts b/src/chains/ethereum/ethereum/src/provider.ts
@@ -432,12 +432,10 @@ export class EthereumProvider
     this.#executor.stop();
     await this.#blockchain.stop();
 
-    // only call close on the logger if it's an instance of AsyncronousLogger
+    // only need to do this if it's an `AsyncronousLogger`
     if ("getCompletionHandle" in this.#options.logging.logger) {
-      // todo: maybe need to stop the logger from accepting new logs. This should work as is, because we wait for
-      // any logs created before we call getCompletionHandle().
       await this.#options.logging.logger.getCompletionHandle();
-      closeSync(+this.#options.logging.file);
+      closeSync(this.#options.logging.file);
     }
 
     this.#executor.end();

diff --git a/src/chains/ethereum/options/src/logging-options.ts b/src/chains/ethereum/options/src/logging-options.ts
@@ -75,11 +75,11 @@ export type LoggingConfig = {
 
     /**
      * If you set this option, Ganache will write logs to a file located at the
-     * specified path. You can provide a path, or numerical file descriptor.
+     * specified path.
      */
     readonly file: {
       type: number;
-      rawType: number | PathLike;
+      rawType: PathLike;
     };
   };
 };
@@ -108,18 +108,15 @@ export const LoggingOptions: Definitions<LoggingConfig> = {
     cliType: "boolean"
   },
   file: {
-    normalize: (raw: number | PathLike): number => {
+    normalize: (raw: PathLike): number => {
       let descriptor: number;
-      if (typeof raw === "number") {
-        descriptor = raw as number;
-      } else {
-        try {
-          descriptor = openSync(raw as PathLike, "a");
-        } catch (err) {
-          throw new Error(
-            `Failed to open log file ${raw}. Please check if the file path is valid and if the process has write permissions to the directory.`
-          );
-        }
+
+      try {
+        descriptor = openSync(raw, "a");
+      } catch (err) {
+        throw new Error(
+          `Failed to open log file ${raw}. Please check if the file path is valid and if the process has write permissions to the directory.`
+        );
       }
       return descriptor;
     },

diff --git a/src/chains/ethereum/options/tests/logging-options.test.ts b/src/chains/ethereum/options/tests/logging-options.test.ts
@@ -66,9 +66,7 @@ describe("EthereumOptionsConfig", () => {
           });
           try {
             assert(typeof options.logging.file === "number");
-            assert.doesNotThrow(() =>
-              closeSync(options.logging.file as number)
-            );
+            assert.doesNotThrow(() => closeSync(options.logging.file));
           } finally {
             await unlink(validFilePath);
           }
@@ -80,9 +78,7 @@ describe("EthereumOptionsConfig", () => {
           });
           try {
             assert(typeof options.logging.file === "number");
-            assert.doesNotThrow(() =>
-              closeSync(options.logging.file as number)
-            );
+            assert.doesNotThrow(() => closeSync(options.logging.file));
           } finally {
             await unlink(validFilePath);
           }
@@ -94,27 +90,7 @@ describe("EthereumOptionsConfig", () => {
           });
           try {
             assert(typeof options.logging.file === "number");
-            assert.doesNotThrow(() =>
-              closeSync(options.logging.file as number)
-            );
-          } finally {
-            await unlink(validFilePath);
-          }
-        });
-
-        it("uses an existing descriptor if passed in", async () => {
-          const fd = openSync(validFilePath, "a");
-
-          const options = EthereumOptionsConfig.normalize({
-            logging: { file: fd }
-          });
-
-          try {
-            assert.strictEqual(options.logging.file, fd);
-            assert(typeof options.logging.file === "number");
-            assert.doesNotThrow(() =>
-              closeSync(options.logging.file as number)
-            );
+            assert.doesNotThrow(() => closeSync(options.logging.file));
           } finally {
             await unlink(validFilePath);
           }
@@ -140,13 +116,12 @@ describe("EthereumOptionsConfig", () => {
               calls.push([message, ...params]);
             }
           };
-          const descriptor = openSync(validFilePath, "a");
 
           try {
             const options = EthereumOptionsConfig.normalize({
               logging: {
                 logger,
-                file: descriptor
+                file: validFilePath
               }
             });
 

diff --git a/src/chains/filecoin/options/src/logging-options.ts b/src/chains/filecoin/options/src/logging-options.ts
@@ -1,4 +1,3 @@
-import { normalize } from "./helpers";
 import { Definitions } from "@ganache/options";
 import { openSync, PathLike } from "fs";
 import { Logger, createLogger } from "@ganache/utils";
@@ -26,24 +25,24 @@ export type LoggingConfig = {
 
     /**
      * If you set this option, Ganache will write logs to a file located at the
-     * specified path. You can provide a path, or numerical file descriptor.
+     * specified path.
      */
     readonly file: {
       type: number;
-      rawType: number | PathLike;
+      rawType: PathLike;
     };
   };
 };
 
 export const LoggingOptions: Definitions<LoggingConfig> = {
   file: {
-    normalize: (raw: number | PathLike): number => {
+    normalize: (raw: PathLike): number => {
       let descriptor: number;
       if (typeof raw === "number") {
         descriptor = raw as number;
       } else {
         try {
-          descriptor = openSync(raw as PathLike, "a");
+          descriptor = openSync(raw, "a");
         } catch (err) {
           throw new Error(
             `Failed to open log file ${raw}. Please check if the file path is valid and if the process has write permissions to the directory.`

diff --git a/src/chains/filecoin/options/tests/logging-options.test.ts b/src/chains/filecoin/options/tests/logging-options.test.ts
@@ -40,9 +40,7 @@ describe("FilecoinOptionsConfig", () => {
           });
           try {
             assert(typeof options.logging.file === "number");
-            assert.doesNotThrow(() =>
-              closeSync(options.logging.file as number)
-            );
+            assert.doesNotThrow(() => closeSync(options.logging.file));
           } finally {
             await unlink(validFilePath);
           }
@@ -54,9 +52,7 @@ describe("FilecoinOptionsConfig", () => {
           });
           try {
             assert(typeof options.logging.file === "number");
-            assert.doesNotThrow(() =>
-              closeSync(options.logging.file as number)
-            );
+            assert.doesNotThrow(() => closeSync(options.logging.file));
           } finally {
             await unlink(validFilePath);
           }
@@ -68,27 +64,7 @@ describe("FilecoinOptionsConfig", () => {
           });
           try {
             assert(typeof options.logging.file === "number");
-            assert.doesNotThrow(() =>
-              closeSync(options.logging.file as number)
-            );
-          } finally {
-            await unlink(validFilePath);
-          }
-        });
-
-        it("uses an existing descriptor if passed in", async () => {
-          const fd = openSync(validFilePath, "a");
-
-          const options = FilecoinOptionsConfig.normalize({
-            logging: { file: fd }
-          });
-
-          try {
-            assert.strictEqual(options.logging.file, fd);
-            assert(typeof options.logging.file === "number");
-            assert.doesNotThrow(() =>
-              closeSync(options.logging.file as number)
-            );
+            assert.doesNotThrow(() => closeSync(options.logging.file));
           } finally {
             await unlink(validFilePath);
           }
@@ -114,13 +90,12 @@ describe("FilecoinOptionsConfig", () => {
               calls.push([message, ...params]);
             }
           };
-          const descriptor = openSync(validFilePath, "a");
 
           try {
             const options = FilecoinOptionsConfig.normalize({
               logging: {
                 logger,
-                file: descriptor
+                file: validFilePath
               }
             });
 

diff --git a/src/packages/options/src/create.ts b/src/packages/options/src/create.ts
@@ -51,7 +51,7 @@ function fill(defaults: any, options: any, target: any, namespace: any) {
       const propDefinition = def[key];
       let value = namespaceOptions[key];
       if (value !== undefined) {
-        const normalized = propDefinition.normalize(value, namespaceOptions);
+        const normalized = propDefinition.normalize(value, config);
         if (normalized !== undefined) {
           checkForConflicts(
             key,
@@ -66,7 +66,7 @@ function fill(defaults: any, options: any, target: any, namespace: any) {
         const legacyName = propDefinition.legacyName || key;
         value = options[legacyName];
         if (value !== undefined) {
-          const normalized = propDefinition.normalize(value, options);
+          const normalized = propDefinition.normalize(value, config);
           if (normalized !== undefined) {
             checkForConflicts(
               key,
@@ -90,7 +90,7 @@ function fill(defaults: any, options: any, target: any, namespace: any) {
       const legacyName = propDefinition.legacyName || key;
       const value = options[legacyName];
       if (value !== undefined) {
-        const normalized = propDefinition.normalize(value, options);
+        const normalized = propDefinition.normalize(value, config);
         if (normalized !== undefined) {
           checkForConflicts(
             key,

diff --git a/src/packages/utils/src/things/logger.ts b/src/packages/utils/src/things/logger.ts
@@ -26,10 +26,12 @@ export function createLogger(config: {
   } else {
     if (typeof config.file !== "number") {
       throw new Error(
-        "`config.file` was not correctly noramlized to a file descriptor. This should not happen."
+        `'config.file' was not correctly noramlized to a file descriptor. This should not happen. ${
+          config.file
+        }: ${typeof config.file}`
       );
     }
-    const descriptor = config.file as number;
+    const descriptor = config.file;
 
     const diskLogFormatter = (message: any) => {
       const linePrefix = `${new Date().toISOString()} `;