From cbcf868c9e114e1a3be2357d3618aab0c15cf782 Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Sun, 13 Oct 2019 23:40:29 +0200 Subject: [PATCH] Refactor fileOps.getNewestFile Simplify getNewestFile by converting it to an async function, use the fs-extra promise based fs functions and remove the files argument as well as the callback. Refactors the tests as well to use the test/temp folder. --- client.js | 22 +++++---- lib/fileOps/fileOps.js | 58 +++++++++-------------- test/lib/fileOps/fileOps.js | 94 +++++++++++-------------------------- 3 files changed, 62 insertions(+), 112 deletions(-) diff --git a/client.js b/client.js index 27e8be92..ba93aacc 100644 --- a/client.js +++ b/client.js @@ -530,18 +530,22 @@ write-data=${ instance.path() }\r\n // Spawn factorio server //var serverprocess = child_process.exec(commandline); - fileOps.getNewestFile(instance.path("saves"), fs.readdirSync(instance.path("saves")),function(err, latestSave) { - if(err) { - console.error("ERROR!"); - console.error("Your savefile seems to be missing. This might because you created an instance without having factorio\ - installed and configured properly. Try installing factorio and adding your savefile to instances/[instancename]/saves/\n"); - throw err; - } + let latestSave = await fileOps.getNewestFile(instance.path("saves")); + if (latestSave === null) { + throw new Error( + "Your savefile seems to be missing. This might because you created an\n"+ + "instance without having factorio installed and configured properly.\n"+ + "Try installing factorio and adding your savefile to\n"+ + "instances/[instancename]/saves/" + ); + } + + { // implicit global serverprocess = child_process.spawn( './' + config.factorioDirectory + '/bin/x64/factorio', [ '-c', instance.path("config.ini"), - '--start-server', latestSave.file, + '--start-server', instance.path("saves", latestSave), '--rcon-port', args["rcon-port"] || Number(process.env.RCONPORT) || instanceconfig.clientPort, '--rcon-password', args["rcon-password"] || instanceconfig.clientPassword, '--server-settings', instance.path("server-settings.json"), @@ -632,7 +636,7 @@ write-data=${ instance.path() }\r\n // set some globals confirmedOrders = []; lastSignalCheck = Date.now(); - }); + } } async function startClient() { diff --git a/lib/fileOps/fileOps.js b/lib/fileOps/fileOps.js index 014f5577..1bcbe039 100644 --- a/lib/fileOps/fileOps.js +++ b/lib/fileOps/fileOps.js @@ -1,44 +1,28 @@ -const fs = require("fs"); +const fs = require("fs-extra"); const path = require("path"); -// gets newest file in a directory -// dir is directory -// files is array of filenames -// callback(err, filename string); -var getNewestFile = function(dir, files, callback) { - if (!callback) throw new Error("Missing callback"); - if (!files || (files && files.length === 0)) { - callback(new Error("No files")); - return; - } - // we are appending the file name to the directory, without the - // "/" we would get paths like /folderfile.txt instead of /folder/file.txt - if(dir[dir.length-1] != "/"){ - dir += "/"; - } - var newest = { file: files[0] }; - var checked = 0; - fs.stat(dir + newest.file, function(err, stats) { - if(err) { - callback(err); - } else { - newest.mtime = stats.mtime; - for (var i = 0; i < files.length; i++) { - var file = files[i]; - (function(file) { - fs.stat(file, function(err, stats) { - ++checked; - if (stats.mtime.getTime() > newest.mtime.getTime()) { - newest = { file : file, mtime : stats.mtime }; - } - if (checked == files.length) { - callback(undefined,newest); - } - }); - })(dir + file); +/** + * Returns the newest file in a directory + * + * @param {string} directory - The directory to look for the newest file in. + * @returns {string|null} + * Name of the file with the newest timestamp or null if the + * directory contains no files. + */ +async function getNewestFile(directory) { + let newestTime = new Date(0); + let newestFile = null; + for (let entry of await fs.readdir(directory, { withFileTypes: true })) { + if (entry.isFile()) { + let stat = await fs.stat(path.join(directory, entry.name)); + if (stat.mtime > newestTime) { + newestTime = stat.mtime; + newestFile = entry.name; } } - }); + } + + return newestFile; } // get names of all directories in directory diff --git a/test/lib/fileOps/fileOps.js b/test/lib/fileOps/fileOps.js index 7b9a66aa..74aadfee 100644 --- a/test/lib/fileOps/fileOps.js +++ b/test/lib/fileOps/fileOps.js @@ -1,40 +1,24 @@ -const assert = require('assert'); -const fs = require('fs'); +const assert = require('assert').strict; +const fs = require('fs-extra'); const path = require('path'); const fileOps = require('lib/fileOps'); -// create folders for testing -function setupTestingEnv(){ - if (!fs.existsSync(path.resolve(__dirname, "database"))){ - fs.mkdirSync(path.resolve(__dirname, "database")); - } - if (!fs.existsSync(path.resolve(__dirname, "test"))){ - fs.mkdirSync(path.resolve(__dirname, "test")); - } - if (!fs.existsSync(path.resolve(__dirname, "test/folder"))){ - fs.mkdirSync(path.resolve(__dirname, "test/folder")); - } - if (!fs.existsSync(path.resolve(__dirname, "test/another folder"))){ - fs.mkdirSync(path.resolve(__dirname, "test/another folder")); - } - if (!fs.existsSync(path.resolve(__dirname, "test/file.txt"))){ - fs.writeFileSync(path.resolve(__dirname, "test/file.txt"), "contents"); - } - if (!fs.existsSync(path.resolve(__dirname, "test/another file.txt"))){ - fs.writeFileSync(path.resolve(__dirname, "test/another file.txt"), "more contents"); - } - // we need this xfile to test a branch in fileOps.getNewestFile()s propagation - if (!fs.existsSync(path.resolve(__dirname, "test/xfile.txt"))){ - fs.writeFileSync(path.resolve(__dirname, "test/xfile.txt"), "contents"); - } -} -setupTestingEnv(); -setupTestingEnv(); // run twice to get branch coverage describe("fileOps.js", function(){ + let baseDir = path.join("test", "temp", "fileOps"); + async function setupTestingEnv() { + await fs.ensureDir(path.join(baseDir, "test", "folder")); + await fs.ensureDir(path.join(baseDir, "test", "another folder")); + + await fs.outputFile(path.join(baseDir, "test", "file.txt"), "contents"); + await fs.outputFile(path.join(baseDir, "test", "another file.txt"), "more contents"); + } + + before(setupTestingEnv); + describe("fileOps.getDirectoriesSync()", function(){ it("gets an array of directory names from a path", function(){ - let directories = fileOps.getDirectoriesSync(path.resolve(__dirname, "test")); + let directories = fileOps.getDirectoriesSync(path.join(baseDir, "test")); // because there are no arrays for some reason assert(typeof directories == "object"); @@ -43,7 +27,7 @@ describe("fileOps.js", function(){ assert(directories[0] == "another folder" && directories[1] == "folder"); }); it("does not return any filenames", function(){ - let directories = fileOps.getDirectoriesSync(path.resolve(__dirname, "test")); + let directories = fileOps.getDirectoriesSync(path.join(baseDir, "test")); // this is a function to return directories, not files. So yeah. assert(!directories.includes("file.txt") && !directories.includes("another file.txt")); @@ -56,7 +40,7 @@ describe("fileOps.js", function(){ }); describe("fileOps.getFileNamesSync(path)", function(){ it("gets names of files in directory", function(){ - let files = fileOps.getFileNamesSync(path.resolve(__dirname, "test")); + let files = fileOps.getFileNamesSync(path.join(baseDir, "test")); assert(files.length > 0); assert(files.includes("file.txt") && files.includes("another file.txt")); }); @@ -67,42 +51,20 @@ describe("fileOps.js", function(){ }); }); describe("fileOps.getNewestFile()", function(){ - it("gets the newest file in a directory async", function(done){ - fileOps.getNewestFile(path.resolve(__dirname, "test/"), fileOps.getFileNamesSync(path.resolve(__dirname, "test")), function(err, newest){ - assert(newest && typeof newest.file == "string"); - done(); - }); + it("returns a string in a directory with files", async function() { + let newest = await fileOps.getNewestFile(path.join(baseDir, "test")); + assert.equal(typeof newest, "string"); }); - it("throws if callback is missing", function(){ - assert.throws(function(){ - fileOps.getNewestFile("test", fileOps.getFileNamesSync(path.resolve(__dirname, "test"))); - }, Error); - }); - it("executes callback(err) if no files are provided", function(done){ - fileOps.getNewestFile(path.resolve(__dirname, "test"), [], function(err, newest){ - assert(err && typeof err == "object"); - done(); - }); - }); - it("executes callback(err) if launched without a directory", function(done){ - fileOps.getNewestFile("", fileOps.getFileNamesSync(path.resolve(__dirname, "test")), function(err, newest){ - assert(err); - done(); - }); - }); - it("executes callback(err) when you do stupid shit", function(done){ - fileOps.getNewestFile({thisWontWork: "but not crash either"}, {lolz:"yeah..."}, function(err, newest){ - assert(err); - // ENOENT: no such file or directory, stat 'C:\**\factorioClusterio\lib\test\undefined' - done(); - }); + it("returns null if directory is empty", async function() { + let newest = await fileOps.getNewestFile(path.join(baseDir, "test", "folder")); + assert.equal(newest, null); }); }); describe("fileOps.deleteFolderRecursiveSync(path)", function(){ it("deletes a folder and all files and folders in it", function(){ - let x = fileOps.deleteFolderRecursiveSync(path.resolve(__dirname, "test")); + let x = fileOps.deleteFolderRecursiveSync(path.join(baseDir, "test")); assert.throws(function(){ - fs.statSync(path.resolve(__dirname, "test")); + fs.statSync(path.join(baseDir, "test")); }, Error); assert(x); // give it some extra time because its sometimes very slow @@ -114,11 +76,11 @@ describe("fileOps.js", function(){ }); describe("fileOps.deleteFolderRecursive(path)", function(){ it("deletes a folder and all files and folders in it, just like sync version", async function(){ - setupTestingEnv(); - assert(fs.statSync(path.resolve(__dirname, "test"))); - let x = await fileOps.deleteFolderRecursive(path.resolve(__dirname, "test")); + await setupTestingEnv(); + assert(fs.statSync(path.join(baseDir, "test"))); + let x = await fileOps.deleteFolderRecursive(path.join(baseDir, "test")); assert.throws(function(){ - fs.statSync(path.resolve(__dirname, "test")); + fs.statSync(path.join(baseDir, "test")); }, Error); assert(x === undefined, "Promise version of this function does not have a return value upon success"); return true;