Fix tests failing due broken Factorio detection (#839)

* Fix tests failing due to missmatched Factorio detection

When the configured factorio directory is full of junk then the tests
use the first directory it finds for one of its tests, while the server
code uses a more elaborate logic to figure this out.

Turns out this is complicating things more than needed. Simply trying
each entry in the directory and seeing if it works when treated as a
directory always work, regardsless of symlinks, junctions,
reparse-points or whatever else filesystem magic there is.

* Skip tests requiring Factorio if it's not present

Prevent confusing test failures by skipping all the tests that require a
working Factorio installation to sucteed, and print a warning at the end
of the test suite if tests may have been skipped due to this.

* Handle ENOTDIR when determining Factorio version

If the path to the factorio install contains a component that is not a
file then ENOTDIR may be returned on POSIX like systems.  Treat this the
same as ENOENT so that detection logic works when used on a file.

---------

Co-authored-by: Hornwitser <code@hornwitser.no>

packages/host/src/server.ts

@@ -24,7 +24,7 @@ async function getFactorioVersion(factorioDir: string) {
 	try {
 		changelog = await fs.readFile(changelogPath, "utf-8");
 	} catch (err: any) {
-		if (err.code === "ENOENT") {
+		if (err.code === "ENOENT" || err.code === "ENOTDIR") {
 			return null;
 		}
 		throw err;
@@ -85,17 +85,8 @@ async function listFactorioVersions(factorioDir: string) {
 	}
 
 	for (const name of await fs.readdir(factorioDir)) {
-		// We do not use withFileTypes or lstat because we want to follow symlinks
+		// Avoid any fancy directory detection in order to follow symlinks
 		const fullPath = path.join(factorioDir, name);
-		try {
-			const stats = await fs.stat(fullPath);
-			if (!stats.isDirectory()) {
-				continue;
-			}
-		} catch {
-			continue;
-		}
-
 		const version = await getFactorioVersion(fullPath);
 		if (version === null) {
 			continue;
@@ -204,17 +195,8 @@ async function findVersion(factorioDir: string, targetVersion: lib.TargetVersion
 	// Otherwise assume it is a directory of multiple versions
 	const versions = new Map<lib.FullVersion, string>();
 	for (const name of await fs.readdir(factorioDir)) {
-		// We do not use withFileTypes or lstat because we want to follow symlinks
+		// Avoid any fancy directory detection in order to follow symlinks
 		const fullPath = path.join(factorioDir, name);
-		try {
-			const stats = await fs.stat(fullPath);
-			if (!stats.isDirectory()) {
-				continue;
-			}
-		} catch {
-			continue;
-		}
-
 		const version = await getFactorioVersion(fullPath);
 		if (version === null) {
 			continue;

test/integration/clusterio.js

@@ -15,6 +15,7 @@ const {
 	TestControl, TestControlConnector, url, controlToken, slowTest,
 	execCtl, execCtlProcess, execController, execHost, sendRcon, getControl,
 	spawnNode, instancesDir, factorioDir, databaseDir, controllerConfigPath,
+	requiresFactorio, hasFactorio,
 } = require("./index");
 
 
@@ -276,6 +277,7 @@ describe("Integration of Clusterio", function() {
 		});
 		it("should download mods from controller", async function() {
 			slowTest(this);
+			requiresFactorio(this);
 			await runWithAltHost(async () => {
 				// Use the latest vesion in /dist instead of a hardcoded version
 				async function checkModDownloaded() {
@@ -291,6 +293,7 @@ describe("Integration of Clusterio", function() {
 		});
 		it("should auto start instances with auto_start enabled", async function() {
 			slowTest(this);
+			requiresFactorio(this);
 			this.timeout(30000); // Need an even longer timeout for this test
 
 			let hostProcess;
@@ -624,6 +627,7 @@ describe("Integration of Clusterio", function() {
 		describe("instance save create", function() {
 			it("creates a save", async function() {
 				slowTest(this);
+				requiresFactorio(this);
 				getControl().saveUpdates = [];
 				await execCtl("instance save create test");
 				await checkInstanceStatus(44, "stopped");
@@ -633,6 +637,7 @@ describe("Integration of Clusterio", function() {
 		describe("InstanceSaveDetailsUpdatesEvent", function() {
 			it("should have triggered for the created save", function() {
 				slowTest(this);
+				requiresFactorio(this);
 				assert.equal(getControl().saveUpdates.slice(-1)[0].updates[0].name, "world.zip");
 			});
 		});
@@ -640,6 +645,7 @@ describe("Integration of Clusterio", function() {
 		describe("instance save list", function() {
 			it("lists the created save", async function() {
 				slowTest(this);
+				requiresFactorio(this);
 				let result = await execCtlProcess("instance save list test");
 				assert(/world\.zip/.test(result.stdout), "world.zip not present in list save output");
 			});
@@ -648,6 +654,7 @@ describe("Integration of Clusterio", function() {
 		describe("instance export-data", function() {
 			it("exports the data", async function() {
 				slowTest(this);
+				requiresFactorio(this);
 				let exportPath = path.join("temp", "test", "static");
 				await fs.remove(exportPath);
 				await execCtl("instance export-data test");
@@ -669,12 +676,16 @@ describe("Integration of Clusterio", function() {
 
 		for (let cmd of ["start", "restart"]) {
 			describe(`instance ${cmd}`, function() {
+				requiresFactorio(this);
 				async function prepareToStart() {
 					if (cmd === "restart") {
 						await execCtl("instance start test");
 					}
 				}
 				after(async function() {
+					if (!hasFactorio()) {
+						return;
+					}
 					// It is expected that after these tests the instance is running, I dislike this dependency
 					// So this is here to make sure the other tests can continue any of these ones fail
 					const instances = await getInstances();
@@ -755,6 +766,7 @@ describe("Integration of Clusterio", function() {
 		}
 
 		describe("instance send-rcon", function() {
+			requiresFactorio(this);
 			this.afterAll(async function() {
 				// Prevents cascading failure where enable_script_commands is expected to be true
 				await execCtl("instance config set test factorio.enable_script_commands true");
@@ -803,6 +815,7 @@ describe("Integration of Clusterio", function() {
 		});
 
 		describe("instance config set-prop", function() {
+			requiresFactorio(this);
 			it("applies factorio settings while running", async function() {
 				slowTest(this);
 
@@ -958,6 +971,7 @@ describe("Integration of Clusterio", function() {
 			});
 			it("should send ban list commands to running instances", async function() {
 				slowTest(this);
+				requiresFactorio(this);
 				// Check that both names are not on the list
 				const preCommandStateLower = await sendRcon(44, "/banlist get test_rcon_ban");
 				const preCommandStateUpper = await sendRcon(44, "/banlist get test_RCON_ban");
@@ -985,6 +999,7 @@ describe("Integration of Clusterio", function() {
 			});
 			it("should send whitelist commands to running instances", async function() {
 				slowTest(this);
+				requiresFactorio(this);
 				// Check that both names are not on the list
 				const preCommandStateLower = await sendRcon(44, "/whitelist get test_rcon_whitelist");
 				const preCommandStateUpper = await sendRcon(44, "/whitelist get test_RCON_whitelist");
@@ -1019,6 +1034,7 @@ describe("Integration of Clusterio", function() {
 			it("should send admin list commands to running instances", async function() {
 				// Because there is no admin list command this test will be different to the other two above
 				slowTest(this);
+				requiresFactorio(this);
 				// Check that both names are not on the list
 				const preCommandStateLower = await sendRcon(44, "/demote test_rcon_admin");
 				const preCommandStateUpper = await sendRcon(44, "/demote test_RCON_admin");
@@ -1067,6 +1083,7 @@ describe("Integration of Clusterio", function() {
 		describe("instance extract-players", function() {
 			it("runs", async function() {
 				slowTest(this);
+				requiresFactorio(this);
 				await execCtl("instance extract-players test");
 			});
 		});
@@ -1074,6 +1091,7 @@ describe("Integration of Clusterio", function() {
 		describe("instance stop", function() {
 			it("stops the instance", async function() {
 				slowTest(this);
+				requiresFactorio(this);
 				await execCtl("instance stop test");
 				await checkInstanceStatus(44, "stopped");
 			});
@@ -1082,6 +1100,7 @@ describe("Integration of Clusterio", function() {
 		describe("instance load-scenario", function() {
 			it("starts the instance with the given settings", async function() {
 				slowTest(this);
+				requiresFactorio(this);
 				await execCtl("instance config set 44 factorio.enable_save_patching false");
 				await execCtl("instance config set 44 player_auth.load_plugin false");
 				await execCtl("instance config set 44 research_sync.load_plugin false");
@@ -1117,6 +1136,7 @@ describe("Integration of Clusterio", function() {
 		describe("instance kill", function() {
 			it("kills the instance", async function() {
 				slowTest(this);
+				requiresFactorio(this);
 				await execCtl("instance kill test");
 				await checkInstanceStatus(44, "stopped");
 			});
@@ -1345,6 +1365,7 @@ describe("Integration of Clusterio", function() {
 		describe("instanceUpdateEventHandler()", function() {
 			it("should have triggered for the previous instance status updates", function() {
 				slowTest(this);
+				requiresFactorio(this);
 				let statusesToCheck = new Set([
 					"unassigned", "stopped", "creating_save", "exporting_data",
 					"starting", "running", "stopping", "deleted", "unknown",

test/integration/export.js

@@ -5,11 +5,12 @@ const path = require("path");
 const { _exportLocale } = require("@clusterio/host/dist/node/src/export");
 const { FactorioServer } = require("@clusterio/host/dist/node/src/server");
 
-const { factorioDir } = require("./index");
+const { factorioDir, requiresFactorio } = require("./index");
 
 
 describe("Integration of lib/factorio/export", function() {
 	describe("exportLocale()", function() {
+		requiresFactorio(this);
 		it("returns a nested Map with base game locale information", async function() {
 			let writePath = path.join("temp", "test", "server");
 			let testServer = new FactorioServer(factorioDir, writePath, {});

test/integration/index.js

@@ -12,6 +12,7 @@ const lib = require("@clusterio/lib");
 const { LineSplitter, ConsoleTransport, logger } = lib;
 
 const { selectTargetCommand, initialize: initializeCtl } = require("@clusterio/ctl");
+const { _listFactorioVersions } = require("@clusterio/host/dist/node/src/server");
 
 // Make sure permissions from plugins are loaded
 require("../../plugins/global_chat/dist/node/index");
@@ -127,6 +128,25 @@ function slowTest(test) {
 	test.timeout(30000);
 }
 
+// Mark that this test or suite of tests requires a factorio install to run.
+function requiresFactorio(testOrSuite) {
+	if (testOrSuite.skip) {
+		if (!haveFactorioInstall) {
+			testOrSuite.skip();
+		}
+	} else {
+		before(function() {
+			if (!haveFactorioInstall) {
+				this.skip();
+			}
+		});
+	}
+}
+
+function hasFactorio() {
+	return haveFactorioInstall;
+}
+
 async function get(urlPath) {
 	let res = await phin({
 		method: "GET",
@@ -159,6 +179,7 @@ function getFactorioDir(hostConfig) {
 let controllerProcess;
 let hostProcess;
 let control;
+let haveFactorioInstall;
 
 const baseHostConfig = loadJSON("config-host.json");
 
@@ -337,6 +358,7 @@ before(async function() {
 
 	controllerProcess = await spawnNode("controller:", "../../packages/controller run", /Started controller/);
 
+	haveFactorioInstall = (await _listFactorioVersions(factorioDir)).versions.size > 0;
 	const relativeFactorioDir = path.isAbsolute(factorioDir) ? factorioDir : path.join("..", "..", factorioDir);
 	await execCtlProcess("host create-config --id 4 --name host --generate-token");
 	await execHost(`config set host.factorio_directory ${relativeFactorioDir}`);
@@ -382,6 +404,9 @@ after(async function() {
 	if (control) {
 		await control.connector.close();
 	}
+	if (haveFactorioInstall === false) {
+		console.warn("Many tests may have been skipped due to no factorio install being detected");
+	}
 });
 
 // Ensure the test processes are stopped.
@@ -398,6 +423,7 @@ module.exports = {
 	execController,
 	execCtlProcess,
 	slowTest,
+	requiresFactorio,
 	get,
 	exec,
 	execCtl,
@@ -413,6 +439,7 @@ module.exports = {
 	modsDir,
 	databaseDir,
 	factorioDir,
+	hasFactorio,
 	controllerConfigPath,
 	hostConfigPath,
 	controlConfigPath,

test/integration/instance.js

@@ -5,6 +5,7 @@ const lib = require("@clusterio/lib");
 const { testMatrix } = require("../common");
 const {
 	slowTest, exec, execCtl, execCtlProcess, sendRcon, getControl,
+	requiresFactorio, hasFactorio,
 } = require("./index");
 
 const instId = 48;
@@ -24,6 +25,7 @@ function getUser(name) {
 }
 
 describe("Clusterio Instance", function() {
+	requiresFactorio(this);
 	before(async function() {
 		this.timeout(20000);
 		const visibility = JSON.stringify({ lan: true, public: false }).replace(
@@ -56,6 +58,9 @@ describe("Clusterio Instance", function() {
 		await execCtl(`instance start ${instAltName}`);
 	});
 	after(async function() {
+		if (!hasFactorio()) {
+			return;
+		}
 		this.timeout(20000);
 		// Delete the instances
 		await execCtl(`instance delete ${instName}`);
@@ -186,6 +191,7 @@ describe("Clusterio Instance", function() {
 	}
 });
 describe("start-all and stop-all commands", function() {
+	requiresFactorio(this);
 	// Test instance IDs for start-all/stop-all tests
 	const testInstId1 = 50;
 	const testInstId2 = 51;

test/integration/server.js

@@ -9,10 +9,11 @@ const { InstanceManager } = require("@clusterio/controller");
 const { FactorioServer, _getFactorioVersion } = require("@clusterio/host/dist/node/src/server");
 const { logger } = lib;
 
-const { slowTest, factorioDir } = require("./index");
+const { slowTest, factorioDir, requiresFactorio } = require("./index");
 
 
 describe("Integration of host/src/server", function() {
+	requiresFactorio(this);
 	describe("getFactorioVersion()", function() {
 		it("should get a version from factorio's changelog.txt", async function() {
 			function checkVersion(version) {
@@ -25,14 +26,12 @@ describe("Integration of host/src/server", function() {
 			if (version !== null) {
 				checkVersion(version);
 			} else {
-				for (let entry of await fs.readdir(factorioDir, { withFileTypes: true })) {
-					if (!entry.isDirectory()) {
-						continue;
+				for (let name of await fs.readdir(factorioDir)) {
+					// Avoid any fancy directory detection in order to follow symlinks
+					version = await _getFactorioVersion(path.join(factorioDir, name));
+					if (version !== null) {
+						checkVersion(version);
 					}
-
-					checkVersion(
-						await _getFactorioVersion(path.join(factorioDir, entry.name))
-					);
 				}
 			}
 		});