Stabilize player-list integration tests and RCON fixtures (#1117)

* Wait for player list replication in integration tests

* Select available RCON ports before starting Factorio

* Document zero as automatic RCON port allocation

* Address review wording and test organization

* Restrict port collision hardening to integration fixtures

test/integration/clusterio.js

@@ -16,6 +16,7 @@ import {
 	requiresFactorio, hasFactorio,
 } from "./index.js";
 import { slowTest } from "../common.js";
+import { getTestRconPort } from "./rcon_port.js";
 import { ControllerEcho, HostEchoReceived } from "../file/test_plugin/messages.js";
 
 
@@ -338,6 +339,7 @@ describe("Integration of Clusterio", function() {
 			try {
 				hostProcess = await startAltHost();
 				await execCtl("instance create alt-start --id 97");
+				await execCtl(`instance config set alt-start factorio.rcon_port ${await getTestRconPort()}`);
 				await execCtl("instance assign alt-start 5");
 				await execCtl("instance config set alt-start instance.auto_start true");
 				const visibility = jsonArg({ lan: false, public: false });
@@ -705,6 +707,7 @@ describe("Integration of Clusterio", function() {
 			it("creates the instance", async function() {
 				this.timeout(6000);
 				await execCtl("instance create test --id 44");
+				await execCtl(`instance config set test factorio.rcon_port ${await getTestRconPort()}`);
 				let instances = await getInstances();
 				assert(instances.has(44), "instance was not created");
 				assert.equal(instances.get(44).status, "unassigned", "incorrect instance status");

test/integration/instance.js

@@ -11,6 +11,8 @@ import {
 	requiresFactorio, hasFactorio, getFactorioVersion, instancesDir,
 } from "./index.js";
 
+import { getTestRconPort } from "./rcon_port.js";
+
 const instId = 48;
 const instName = "Integration";
 const instSetConfig = `instance config set ${instName}`;
@@ -40,6 +42,46 @@ function getUser(name) {
 	return getControl().send(new lib.UserGetRequest(name));
 }
 
+// RCON completion does not acknowledge list replication. In particular, whitelist
+// updates pass through an asynchronous file watcher before being broadcast.
+async function waitForAssertion(check, timeoutMs = 5000) {
+	const deadline = performance.now() + timeoutMs;
+	for (;;) {
+		try {
+			await check();
+			return;
+		} catch (err) {
+			if (!(err instanceof assert.AssertionError) || performance.now() >= deadline) {
+				throw err;
+			}
+		}
+		await lib.wait(25);
+	}
+}
+
+async function waitForUserFlag(name, flag, expected, message) {
+	await waitForAssertion(async () => {
+		// The initial event also creates the user, so absence is a pending state.
+		const users = await getControl().send(new lib.UserListRequest());
+		const user = users.find(entry => entry.name.toLowerCase() === name.toLowerCase());
+		assert(user, `User ${name} has not been created`);
+		assert.equal(user[flag], expected, message);
+	});
+}
+
+async function waitForAltAdmin(expected, message) {
+	await waitForAssertion(async () => {
+		// Read Factorio's persisted list before checking /promote or /demote
+		// responses, so the checks cannot apply a change that has not replicated yet.
+		const json = await fs.readFile(path.join(instancesDir, instAltName, "server-adminlist.json"), "utf8");
+		let admins;
+		// A concurrent write can leave a partial document; retry that observation.
+		assert.doesNotThrow(() => { admins = JSON.parse(json); }, "Invalid admin list JSON");
+		assert(Array.isArray(admins), "Admin list is not an array");
+		assert.equal(admins.some(name => name.toLowerCase() === "adminplayer"), expected, message);
+	});
+}
+
 describe("Clusterio Instance", function() {
 	requiresFactorio(this);
 	before(async function() {
@@ -50,6 +92,7 @@ describe("Clusterio Instance", function() {
 
 		// Create a new instance
 		await execCtl(`instance create ${instName} --id ${instId}`);
+		await execCtl(`instance config set ${instName} factorio.rcon_port ${await getTestRconPort()}`);
 		await execCtl(`instance config set ${instName} factorio.enable_whitelist true`);
 		await execCtlProcess(`instance config set-prop ${instName} factorio.settings visibility "${visibility}"`);
 		await execCtl(`instance config set-prop ${instName} factorio.settings require_user_verification false`);
@@ -65,6 +108,7 @@ describe("Clusterio Instance", function() {
 
 		// Create a new alt instance
 		await execCtl(`instance create ${instAltName} --id ${instAltId}`);
+		await execCtl(`instance config set ${instAltName} factorio.rcon_port ${await getTestRconPort()}`);
 		await execCtl(`instance config set ${instAltName} factorio.enable_whitelist true`);
 		await execCtlProcess(`instance config set-prop ${instAltName} factorio.settings visibility "${visibility}"`);
 		await execCtl(`instance config set-prop ${instAltName} factorio.settings require_user_verification false`);
@@ -213,28 +257,28 @@ describe("Clusterio Instance", function() {
 					});
 				}
 				it("should respond to a player ban and unban event", async function() {
+					this.timeout(25000);
 					await execCtl(`${instSetConfig} factorio.sync_banlist bidirectional`);
 					await sendRcon(instId, "/ban BannedPlayer");
-					await lib.wait(15); // Propagation time, 10ms worked for me, so i made it 15ms
-					const userBanned = await getUser("BannedPlayer");
-					assert(userBanned.isBanned, "Player is not banned");
-					const isAltBanned = await sendRcon(instAltId, "/banlist get BannedPlayer");
-					assert.equal(isAltBanned, "BannedPlayer is banned.\n", "User is not banned on alt");
+					await waitForUserFlag("BannedPlayer", "isBanned", true, "Player is not banned");
+					await waitForAssertion(async () => {
+						const isAltBanned = await sendRcon(instAltId, "/banlist get BannedPlayer");
+						assert.equal(isAltBanned, "BannedPlayer is banned.\n", "User is not banned on alt");
+					});
 
 					await sendRcon(instId, "/unban BannedPlayer");
-					await lib.wait(15); // Propagation time, 10ms worked for me, so i made it 15ms
-					const userUnbanned = await getUser("BannedPlayer");
-					assert(!userUnbanned.isBanned, "Player is not unbanned");
-					const isAltUnbanned = await sendRcon(instAltId, "/banlist get BannedPlayer");
-					assert.equal(isAltUnbanned, "BannedPlayer is not banned.\n", "User is not unbanned on alt");
+					await waitForUserFlag("BannedPlayer", "isBanned", false, "Player is not unbanned");
+					await waitForAssertion(async () => {
+						const isAltUnbanned = await sendRcon(instAltId, "/banlist get BannedPlayer");
+						assert.equal(isAltUnbanned, "BannedPlayer is not banned.\n", "User is not unbanned on alt");
+					});
 				});
 				it("should respond to a player promote and demote event", async function() {
-					// Because there is no admin list command this test will be different to the other two
+					this.timeout(25000);
 					await execCtl(`${instSetConfig} factorio.sync_adminlist bidirectional`);
 					await sendRcon(instId, "/promote AdminPlayer");
-					await lib.wait(15); // Propagation time, 10ms worked for me, so i made it 15ms
-					const userPromote = await getUser("AdminPlayer");
-					assert(userPromote.isAdmin, "Player is not promoted");
+					await waitForUserFlag("AdminPlayer", "isAdmin", true, "Player is not promoted");
+					await waitForAltAdmin(true, "User is not promoted on alt");
 					const isAltPromoted = await sendRcon(instAltId, "/promote AdminPlayer");
 					assert.equal(isAltPromoted,
 						"AdminPlayer is already in the admin list and will be promoted upon joining the game.\n",
@@ -242,9 +286,8 @@ describe("Clusterio Instance", function() {
 					);
 
 					await sendRcon(instId, "/demote AdminPlayer");
-					await lib.wait(15); // Propagation time, 10ms worked for me, so i made it 15ms
-					const userDemote = await getUser("AdminPlayer");
-					assert(!userDemote.isAdmin, "Player is not demoted");
+					await waitForUserFlag("AdminPlayer", "isAdmin", false, "Player is not demoted");
+					await waitForAltAdmin(false, "User is not demoted on alt");
 					const isAltDemoted = await sendRcon(instAltId, "/demote AdminPlayer");
 					assert.equal(isAltDemoted,
 						"AdminPlayer is not in the admin list.\n",
@@ -252,26 +295,27 @@ describe("Clusterio Instance", function() {
 					);
 				});
 				it("should respond to a player whitelist add and remove event", async function() {
+					this.timeout(25000);
 					await execCtl(`${instSetConfig} factorio.sync_whitelist bidirectional`);
 					await sendRcon(instId, "/whitelist add WhitelistPlayer");
-					await lib.wait(15); // Propagation time, 10ms worked for me, so i made it 15ms
-					const userAdded = await getUser("WhitelistPlayer");
-					assert(userAdded.isWhitelisted, "Player is not whitelisted");
-					const isAltAdded = await sendRcon(instAltId, "/whitelist get WhitelistPlayer");
-					assert.equal(isAltAdded,
-						"WhitelistPlayer is whitelisted.\n",
-						"User is not whitelisted on alt"
-					);
+					await waitForUserFlag("WhitelistPlayer", "isWhitelisted", true, "Player is not whitelisted");
+					await waitForAssertion(async () => {
+						const isAltAdded = await sendRcon(instAltId, "/whitelist get WhitelistPlayer");
+						assert.equal(isAltAdded,
+							"WhitelistPlayer is whitelisted.\n",
+							"User is not whitelisted on alt"
+						);
+					});
 
 					await sendRcon(instId, "/whitelist remove WhitelistPlayer");
-					await lib.wait(15); // Propagation time, 10ms worked for me, so i made it 15ms
-					const userRemoved = await getUser("WhitelistPlayer");
-					assert(!userRemoved.isWhitelisted, "Player is not unwhitelisted");
-					const isAltRemoved = await sendRcon(instAltId, "/whitelist get WhitelistPlayer");
-					assert.equal(isAltRemoved,
-						"WhitelistPlayer is not whitelisted.\n",
-						"User is not unwhitelisted on alt"
-					);
+					await waitForUserFlag("WhitelistPlayer", "isWhitelisted", false, "Player is not unwhitelisted");
+					await waitForAssertion(async () => {
+						const isAltRemoved = await sendRcon(instAltId, "/whitelist get WhitelistPlayer");
+						assert.equal(isAltRemoved,
+							"WhitelistPlayer is not whitelisted.\n",
+							"User is not unwhitelisted on alt"
+						);
+					});
 				});
 			});
 		});
@@ -303,6 +347,7 @@ describe("start-all and stop-all commands", function() {
 			/"/g, process.platform === "win32" ? '""' : '\\"'
 		);
 		for (const name of [testInstName1, testInstName2, testInstName3]) {
+			await execCtl(`instance config set ${name} factorio.rcon_port ${await getTestRconPort()}`);
 			await execCtlProcess(`instance config set-prop ${name} factorio.settings visibility "${visibility}"`);
 			await execCtl(`instance config set-prop ${name} factorio.settings require_user_verification false`);
 		}

test/integration/rcon_port.js

@@ -0,0 +1,42 @@
+import net from "node:net";
+
+// Give test fixtures distinct ports from a dedicated range rather than relying
+// on the application's random RCON selection. Probe each candidate before use.
+let nextPort = 20000;
+
+export async function getTestRconPort() {
+	while (nextPort < 30000) {
+		const port = nextPort;
+		nextPort += 1;
+		const probe = net.createServer(socket => socket.destroy());
+		const listen = host => new Promise((resolve, reject) => {
+			probe.once("error", reject);
+			probe.listen({ host, port }, () => {
+				probe.removeListener("error", reject);
+				resolve();
+			});
+		});
+		try {
+			try {
+				await listen("::");
+			} catch (err) {
+				if (err.code !== "EAFNOSUPPORT" && err.code !== "EADDRNOTAVAIL") {
+					throw err;
+				}
+				await listen("0.0.0.0");
+			}
+			return port;
+		} catch (err) {
+			if (err.code !== "EADDRINUSE") {
+				throw err;
+			}
+		} finally {
+			if (probe.listening) {
+				await new Promise((resolve, reject) => {
+					probe.close(err => (err ? reject(err) : resolve()));
+				});
+			}
+		}
+	}
+	throw new Error("No available RCON port in the test fixture range 20000-29999");
+}

test/integration/rcon_port.test.js

@@ -0,0 +1,85 @@
+import assert from "node:assert/strict";
+import net from "node:net";
+import events from "node:events";
+
+import { getTestRconPort } from "./rcon_port.js";
+
+describe("test/integration/rcon_port.js", function() {
+	describe("getTestRconPort()", function() {
+		it("gives concurrent fixtures distinct ports and closes its probes", async function() {
+			const ports = await Promise.all(Array.from({ length: 16 }, () => getTestRconPort()));
+			assert.equal(new Set(ports).size, ports.length);
+			for (const port of ports) {
+				assert(port >= 20000 && port < 30000);
+				const probe = net.createServer();
+				try {
+					const listening = events.once(probe, "listening");
+					probe.listen(port, "0.0.0.0");
+					await listening;
+				} finally {
+					if (probe.listening) {
+						await new Promise(resolve => probe.close(resolve));
+					}
+				}
+			}
+		});
+
+		it("skips an occupied candidate", async function() {
+			const original = net.Server.prototype.listen;
+			let occupied;
+			net.Server.prototype.listen = function(options, callback) {
+				if (occupied === undefined) {
+					occupied = options.port;
+					queueMicrotask(() => this.emit("error", Object.assign(new Error("occupied"), {
+						code: "EADDRINUSE",
+					})));
+					return this;
+				}
+				return original.call(this, options, callback);
+			};
+			try {
+				assert.notEqual(await getTestRconPort(), occupied);
+			} finally {
+				net.Server.prototype.listen = original;
+			}
+		});
+
+		it("falls back to IPv4 when IPv6 is unavailable", async function() {
+			const original = net.Server.prototype.listen;
+			const hosts = [];
+			net.Server.prototype.listen = function(options, callback) {
+				hosts.push(options.host);
+				if (options.host === "::") {
+					queueMicrotask(() => this.emit("error", Object.assign(new Error("IPv6 disabled"), {
+						code: "EAFNOSUPPORT",
+					})));
+					return this;
+				}
+				return original.call(this, options, callback);
+			};
+			try {
+				await getTestRconPort();
+				assert.deepEqual(hosts, ["::", "0.0.0.0"]);
+			} finally {
+				net.Server.prototype.listen = original;
+			}
+		});
+
+		it("propagates permission errors", async function() {
+			const original = net.Server.prototype.listen;
+			const error = Object.assign(new Error("permission denied"), { code: "EACCES" });
+			let calls = 0;
+			net.Server.prototype.listen = function() {
+				calls += 1;
+				queueMicrotask(() => this.emit("error", error));
+				return this;
+			};
+			try {
+				await assert.rejects(getTestRconPort(), err => err === error);
+				assert.equal(calls, 1);
+			} finally {
+				net.Server.prototype.listen = original;
+			}
+		});
+	});
+});

test/integration/server.js

@@ -10,6 +10,7 @@ import { logger } from "@clusterio/lib";
 
 import { factorioDir, requiresFactorio } from "./index.js";
 import { slowTest } from "../common.js";
+import { getTestRconPort } from "./rcon_port.js";
 
 
 describe("Integration of host/src/server", function() {
@@ -71,6 +72,12 @@ describe("Integration of host/src/server", function() {
 			logFile.end();
 		});
 
+		beforeEach(async function() {
+			if (server.pid === null) {
+				server.rconPort = await getTestRconPort();
+			}
+		});
+
 		describe(".exampleSettings()", function() {
 			let settings;
 			it("returns an object", async function() {