From 0fb9e793d746636b4becb79f68edc99d9b97a4b9 Mon Sep 17 00:00:00 2001 From: Arvids Godjuks Date: Sun, 18 Oct 2026 22:01:43 +0300 Subject: [PATCH] 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 | 3 + test/integration/instance.js | 111 ++++++++++++++++++++--------- test/integration/rcon_port.js | 42 +++++++++++ test/integration/rcon_port.test.js | 85 ++++++++++++++++++++++ test/integration/server.js | 7 ++ 5 files changed, 215 insertions(+), 33 deletions(-) create mode 100644 test/integration/rcon_port.js create mode 100644 test/integration/rcon_port.test.js diff --git a/test/integration/clusterio.js b/test/integration/clusterio.js index fa01eb8b..8508482f 100644 --- a/test/integration/clusterio.js +++ b/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"); diff --git a/test/integration/instance.js b/test/integration/instance.js index ff856717..25bdf4f0 100644 --- a/test/integration/instance.js +++ b/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`); } diff --git a/test/integration/rcon_port.js b/test/integration/rcon_port.js new file mode 100644 index 00000000..d15420c4 --- /dev/null +++ b/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"); +} diff --git a/test/integration/rcon_port.test.js b/test/integration/rcon_port.test.js new file mode 100644 index 00000000..2decd9de --- /dev/null +++ b/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; + } + }); + }); +}); diff --git a/test/integration/server.js b/test/integration/server.js index be1c70a6..e1f5b369 100644 --- a/test/integration/server.js +++ b/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() {