Address review on the stubs and controller tests

- The controller tests run against a real Controller, which is side effect
  free while not started, with real users through its UserManager. The only
  fakes left are two spies which record broadcasts and permission pushes on
  their way through. Since any user update also applies auto assignment, the
  leave event is observed directly rather than through a role change.
- raise_event fills in the name and tick of the event the way factorio does,
  rather than wrapping the payload, and game.players finds players by index
  as well as by name through a metatable.
- names is generic over anything with a name property, so it lives on the
  suite; the save and load helper no longer calls on_load, the test calls the
  handler itself as it does for the other handlers.
- Sent messages and events are compared whole with deep_eq, which pins the
  event shape, the tick, and that the unassign key is absent rather than
  empty.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

exp_roles/test/controller.test.js

@@ -1,6 +1,7 @@
 "use strict";
 const t = require("tap");
 const lib = require("@clusterio/lib");
+const { Controller } = require("@clusterio/controller");
 const { ControllerPlugin } = require("../dist/node/controller");
 const messages = require("../dist/node/messages");
 const { seedRoles } = require("../dist/node/seed");
@@ -8,191 +9,186 @@ const { seedRoles } = require("../dist/node/seed");
 // Importing this defines the exp_scenario permissions the seed grants
 require("@expcluster/scenario/dist/node/permissions");
 
+// The controller validates message classes against the link registry
+lib.Link.register(messages.RoleUpdatedEvent);
+lib.Link.register(messages.AssignmentUpdatedEvent);
+lib.Link.register(messages.RoleListRequest);
+lib.Link.register(messages.RoleMetaUpdateRequest);
+lib.Link.register(messages.SeedRolesRequest);
+lib.Link.register(messages.AssignmentListRequest);
+lib.Link.register(messages.AssignmentUpdateRequest);
+
 const logger = { child: () => logger, info: () => {}, warn: () => {}, error: () => {}, verbose: () => {} };
 
-class FakeUser {
+/** Build a plugin around a real controller, which is side effect free while not started. */
-	constructor(name, roleIds = []) {
+async function startPlugin(t2, { roles = [] } = {}) {
-		this.name = name;
+	const controllerConfig = new lib.ControllerConfig("controller", {
-		this.roleIds = new Set(roleIds);
-		this.playerStats = { onlineTimeMs: 0 };
-		this.updatedAtMs = 0;
-		this.isDeleted = false;
-	}
-
-	set(field, value) {
-		this[field] = value;
-		this.updatedAtMs += 1;
-	}
-}
-
-/** Build a plugin around fake controller internals, started on first use by each test. */
-async function startPlugin(t2, { roles = [], users = [], config = {} } = {}) {
-	const state = { broadcasts: [], permissionUpdates: [] };
-	const configValues = {
 		"controller.database_directory": t2.testdir(),
 		"controller.default_role_id": lib.Role.DefaultPlayerRoleId,
-		...config,
+	});
+	const controller = new Controller(logger, [], controllerConfig);
+	for (const role of roles) {
+		controller.roles.set(role);
+	}
+
+	// Spies which record and then defer to the real behaviour
+	const state = { broadcasts: [], permissionUpdates: [] };
+	const broadcast = controller.subscriptions.broadcast.bind(controller.subscriptions);
+	controller.subscriptions.broadcast = event => {
+		state.broadcasts.push(event);
+		broadcast(event);
 	};
-
+	const permissionsUpdated = controller.userPermissionsUpdated.bind(controller);
-	const roleStore = new lib.SubscribableDatastore(
+	controller.userPermissionsUpdated = user => {
-		...await new lib.MemoryDatastoreProvider(new Map(roles.map(role => [role.id, role]))).bootstrap()
+		state.permissionUpdates.push(user.name);
-	);
+		permissionsUpdated(user);
-	const userStore = new Map(users.map(user => [user.name, user]));
-
-	const controller = {
-		config: { get: key => configValues[key] },
-		roles: roleStore,
-		users: {
-			records: { on: () => {}, values: () => userStore.values() },
-			getByNameMutable: name => userStore.get(name),
-			valuesMutable: () => userStore.values(),
-			getOrCreateUser: name => {
-				if (!userStore.has(name)) {
-					userStore.set(name, new FakeUser(name));
-				}
-				return userStore.get(name);
-			},
-		},
-		subscriptions: { handle: () => {}, broadcast: event => state.broadcasts.push(event) },
-		handle: () => {},
-		userPermissionsUpdated: user => state.permissionUpdates.push(user.name),
 	};
 
 	const plugin = new ControllerPlugin({ name: "exp_roles" }, controller, undefined, logger);
 	await plugin.init();
-	return { plugin, controller, state, users: userStore };
+	return { plugin, controller, state };
 }
 
 const role = (id, name, permissions = []) => new lib.Role(id, name, "", new Set(permissions));
 
 t.test("class ControllerPlugin", t2 => {
-	t2.test("init and sweepRoleMeta manage the role properties", async t2 => {
+	t2.test("init and sweepRoleMeta manage the role properties", async t3 => {
-		const { plugin } = await startPlugin(t2, { roles: [role(0, "Cluster Admin"), role(5, "Moderator")] });
+		const { plugin } = await startPlugin(t3, { roles: [role(0, "Cluster Admin"), role(5, "Moderator")] });
 
-		t2.strictSame(plugin.roleMeta.get(0).order, 1, "the first role is ordered first");
+		t3.strictSame(plugin.roleMeta.get(0).order, 1, "the first role is ordered first");
-		t2.strictSame(plugin.roleMeta.get(5).order, 2, "the next role is ordered after it");
+		t3.strictSame(plugin.roleMeta.get(5).order, 2, "the next role is ordered after it");
 
 		plugin.roleMeta.set(new messages.RoleMetaRecord(99, 3));
 		plugin.sweepRoleMeta();
-		t2.notOk(plugin.roleMeta.get(99), "properties without a role are removed");
+		t3.notOk(plugin.roleMeta.get(99), "properties without a role are removed");
-		t2.ok(plugin.roleMeta.get(5), "properties with a role are kept");
+		t3.ok(plugin.roleMeta.get(5), "properties with a role are kept");
 	});
 
-	t2.test("buildRoleRecord combines the role with its properties", async t2 => {
+	t2.test("buildRoleRecord combines the role with its properties", async t3 => {
-		const { plugin } = await startPlugin(t2, {
+		const { plugin, controller } = await startPlugin(t3, {
 			roles: [role(1, "Player"), role(5, "Moderator", ["exp_scenario.command.kill"])],
 		});
 		plugin.roleMeta.set(new messages.RoleMetaRecord(5, 2, 1, "Mod"));
 
-		const record = plugin.buildRoleRecord(plugin.controller.roles.get(5));
+		const record = plugin.buildRoleRecord(controller.roles.get(5));
-		t2.strictSame(record.name, "Moderator", "the name comes from the role");
+		t3.strictSame(record.name, "Moderator", "the name comes from the role");
-		t2.strictSame(record.permissions, ["exp_scenario.command.kill"], "the permissions come from the role");
+		t3.strictSame(record.permissions, ["exp_scenario.command.kill"], "the permissions come from the role");
-		t2.strictSame(record.meta.shortHand, "Mod", "the properties come from the datastore");
+		t3.strictSame(record.meta.shortHand, "Mod", "the properties come from the datastore");
-		t2.notOk(record.isDefault, "not the default role");
+		t3.notOk(record.isDefault, "not the default role");
-		t2.ok(plugin.buildRoleRecord(plugin.controller.roles.get(1)).isDefault, "the default role is marked");
+		t3.ok(plugin.buildRoleRecord(controller.roles.get(1)).isDefault, "the default role is marked");
 	});
 
-	t2.test("rolesUpdated and roleMetaUpdated broadcast to subscribers", async t2 => {
+	t2.test("rolesUpdated and roleMetaUpdated broadcast to subscribers", async t3 => {
-		const { plugin, controller, state } = await startPlugin(t2, { roles: [role(1, "Player")] });
+		const { plugin, controller, state } = await startPlugin(t3, { roles: [role(1, "Player")] });
 
 		state.broadcasts.length = 0;
 		controller.roles.set(role(5, "Moderator"));
-		const updated = state.broadcasts.flatMap(event => event.updates.map(update => update.name));
+		const updated = state.broadcasts
-		t2.ok(updated.includes("Moderator"), "a new role is broadcast");
+			.filter(event => event instanceof messages.RoleUpdatedEvent)
+			.flatMap(event => event.updates.map(update => update.name));
+		t3.ok(updated.includes("Moderator"), "a new role is broadcast");
 
 		state.broadcasts.length = 0;
 		plugin.roleMeta.set(new messages.RoleMetaRecord(5, 2, 0, "Mod"));
-		t2.ok(state.broadcasts.some(
+		t3.ok(state.broadcasts.some(
 			event => event instanceof messages.RoleUpdatedEvent && event.updates[0].meta.shortHand === "Mod"
 		), "a property change is broadcast as its role");
 
 		state.broadcasts.length = 0;
 		await plugin.onControllerConfigFieldChanged("controller.default_role_id");
-		t2.ok(state.broadcasts.some(event => event instanceof messages.RoleUpdatedEvent), "a default role change rebroadcasts");
+		t3.ok(
+			state.broadcasts.some(event => event instanceof messages.RoleUpdatedEvent),
+			"a default role change rebroadcasts",
+		);
 	});
 
-	t2.test("handleRoleSubscription replays only newer records", async t2 => {
+	t2.test("handleRoleSubscription replays only newer records", async t3 => {
-		const { plugin, controller } = await startPlugin(t2, { roles: [role(1, "Player")] });
+		const { plugin, controller } = await startPlugin(t3, { roles: [role(1, "Player")] });
 		controller.roles.set(role(5, "Moderator"));
 		const updatedAtMs = plugin.buildRoleRecord(controller.roles.get(5)).updatedAtMs;
 
 		const all = await plugin.handleRoleSubscription({ lastRequestTimeMs: 0 });
-		t2.strictSame(all.updates.length, 2, "everything is replayed from the start");
+		t3.strictSame(all.updates.length, 2, "everything is replayed from the start");
 		const none = await plugin.handleRoleSubscription({ lastRequestTimeMs: updatedAtMs });
-		t2.strictSame(none, null, "nothing is replayed when up to date");
+		t3.strictSame(none, null, "nothing is replayed when up to date");
 	});
 
-	t2.test("listAssignmentRecords mirrors users without the default role", async t2 => {
+	t2.test("listAssignmentRecords mirrors users without the default role", async t3 => {
-		const alice = new FakeUser("alice", [lib.Role.DefaultPlayerRoleId, 5]);
+		const { plugin, controller } = await startPlugin(t3, { roles: [role(1, "Player"), role(5, "Moderator")] });
-		alice.updatedAtMs = 10;
+		controller.users.createUser("alice").set("roleIds", new Set([lib.Role.DefaultPlayerRoleId, 5]));
-		const bob = new FakeUser("bob", [lib.Role.DefaultPlayerRoleId]);
+		controller.users.createUser("bob");
-		const { plugin } = await startPlugin(t2, { roles: [role(1, "Player"), role(5, "Moderator")], users: [alice, bob] });
 
 		const records = plugin.listAssignmentRecords();
-		t2.strictSame(records.length, 1, "users with only the default role are left out");
+		t3.strictSame(records.length, 1, "users with only the default role are left out");
-		t2.strictSame(records[0].name, "alice");
+		t3.strictSame(records[0].name, "alice");
-		t2.strictSame([...records[0].roleIds], [5], "the default role is not listed");
+		t3.strictSame([...records[0].roleIds], [5], "the default role is not listed");
 	});
 
-	t2.test("handleAssignmentUpdateRequest validates the user and roles", async t2 => {
+	t2.test("handleAssignmentUpdateRequest validates the user and roles", async t3 => {
-		const alice = new FakeUser("alice", [5]);
+		const { plugin, controller, state } = await startPlugin(t3, {
-		const { plugin, state } = await startPlugin(t2, {
+			roles: [role(1, "Player"), role(5, "Moderator"), role(6, "Regular")],
-			roles: [role(1, "Player"), role(5, "Moderator"), role(6, "Regular")], users: [alice],
 		});
+		controller.users.createUser("alice").set("roleIds", new Set([5]));
 
-		await t2.rejects(
+		await t3.rejects(
 			plugin.handleAssignmentUpdateRequest(new messages.AssignmentUpdateRequest("nobody", [], [])),
 			{ message: /does not exist/ }, "an unknown user is refused",
 		);
-		await t2.rejects(
+		await t3.rejects(
 			plugin.handleAssignmentUpdateRequest(new messages.AssignmentUpdateRequest("alice", [99], [])),
 			{ message: /does not exist/ }, "an unknown role is refused",
 		);
 
 		await plugin.handleAssignmentUpdateRequest(new messages.AssignmentUpdateRequest("alice", [6], [5]));
-		t2.strictSame([...alice.roleIds], [6], "roles are added and removed");
+		t3.strictSame([...controller.users.getByName("alice").roleIds], [6], "roles are added and removed");
-		t2.ok(state.permissionUpdates.includes("alice"), "permission changes are pushed to the user");
+		t3.ok(state.permissionUpdates.includes("alice"), "permission changes are pushed to the user");
 	});
 
-	t2.test("applyAutoAssign and onPlayerEvent grant roles from online time", async t2 => {
+	t2.test("applyAutoAssign and onPlayerEvent grant roles from online time", async t3 => {
-		const newcomer = new FakeUser("newcomer");
+		const { plugin, controller } = await startPlugin(t3, {
-		newcomer.playerStats.onlineTimeMs = 3600000;
-		const veteran = new FakeUser("veteran");
-		veteran.playerStats.onlineTimeMs = 7200000;
-		const jailed = new FakeUser("jailed", [7]);
-		jailed.playerStats.onlineTimeMs = 7200000;
-
-		const { plugin } = await startPlugin(t2, {
 			roles: [role(1, "Player"), role(6, "Regular"), role(7, "Jail")],
-			users: [newcomer, veteran, jailed],
 		});
+		const onlineTime = ms => new Map([[1, lib.PlayerStats.fromJSON({ online_time_ms: ms })]]);
+		controller.users.createUser("newcomer").set("instanceStats", onlineTime(3600000));
+		controller.users.createUser("veteran").set("instanceStats", onlineTime(7200000));
+		const jailed = controller.users.createUser("jailed");
+		jailed.set("roleIds", new Set([7]));
+		jailed.set("instanceStats", onlineTime(7200000));
+		const roleIds = name => controller.users.getByName(name).roleIds;
+
 		// The blocking role is marked first, since setting properties applies auto assignment
 		plugin.roleMeta.set(new messages.RoleMetaRecord(7, 3, 1, "", "", null, null, true));
 		plugin.roleMeta.set(new messages.RoleMetaRecord(6, 2, 0, "", "", null, 7200000));
-		t2.ok(veteran.roleIds.has(6), "enough online time grants the role");
+		t3.ok(roleIds("veteran").has(6), "enough online time grants the role");
-		t2.notOk(newcomer.roleIds.has(6), "not enough online time does not");
+		t3.notOk(roleIds("newcomer").has(6), "not enough online time does not");
-		t2.notOk(jailed.roleIds.has(6), "a blocking role prevents the grant");
+		t3.notOk(roleIds("jailed").has(6), "a blocking role prevents the grant");
 
-		veteran.roleIds.delete(6);
+		// Any user update also applies auto assignment, so the leave path is
+		// observed directly rather than through a role change
+		const applied = [];
+		plugin.applyAutoAssign = names => applied.push(names);
 		await plugin.onPlayerEvent(undefined, { type: "leave", name: "veteran" });
-		t2.ok(veteran.roleIds.has(6), "granted again when the player leaves");
+		t3.strictSame(applied, [["veteran"]], "a leave applies auto assignment for that user");
+		await plugin.onPlayerEvent(undefined, { type: "join", name: "veteran" });
+		t3.strictSame(applied.length, 1, "other player events do not");
 	});
 
-	t2.test("handleSeedRolesRequest creates the roles and reuses them by name", async t2 => {
+	t2.test("handleSeedRolesRequest creates the roles and reuses them by name", async t3 => {
-		const { plugin, controller } = await startPlugin(t2, {
+		const { plugin, controller } = await startPlugin(t3, {
 			roles: [role(0, "Cluster Admin", ["core.admin"]), role(1, "Player")],
 		});
 
 		await plugin.handleSeedRolesRequest();
-		t2.strictSame(controller.roles.size, seedRoles.length, "every seed role exists");
+		t3.strictSame(controller.roles.size, seedRoles.length, "every seed role exists");
 
 		const moderator = [...controller.roles.values()].find(other => other.name === "Moderator");
-		t2.ok(moderator.permissions.has("exp_scenario.command.jail"), "parent permissions are flattened in");
+		t3.ok(moderator.permissions.has("exp_scenario.command.jail"), "parent permissions are flattened in");
-		t2.ok(plugin.roleMeta.get(moderator.id), "the role properties are created");
+		t3.ok(plugin.roleMeta.get(moderator.id), "the role properties are created");
-		t2.strictSame(plugin.roleMeta.get(moderator.id).shortHand, "Mod", "the properties match the seed");
+		t3.strictSame(plugin.roleMeta.get(moderator.id).shortHand, "Mod", "the properties match the seed");
 
 		await plugin.handleSeedRolesRequest();
-		t2.strictSame(controller.roles.size, seedRoles.length, "seeding again reuses the roles");
+		t3.strictSame(controller.roles.size, seedRoles.length, "seeding again reuses the roles");
 	});
+
 	t2.end();
 });

exp_roles/test/lua/assignment.lua

@@ -1,6 +1,6 @@
 --- Tests for the assignment methods of module/control.lua
 local Suite = ... --- The suite this file adds its tests to, see test/lua/framework.lua
-local test, check, eq = Suite.test, Suite.check, Suite.eq
+local test, check, eq, deep_eq = Suite.test, Suite.check, Suite.eq, Suite.deep_eq
 
 --- alice is a moderator and bob a regular, with changes sent to the controller
 local function setup(env)
@@ -20,13 +20,20 @@ end
 test("role:assign applies locally and is sent to the controller", function(env)
     local players = setup(env)
     env.R("Moderator"):assign(players.bob, { by_player_name = "alice" })
-    check(#env.sent == 1 and env.sent[1].channel == "exp_roles:assignment_update", "the assignment is sent")
+    deep_eq(env.sent, {
-    check(env.sent[1].data.name == "bob", "the payload names the player")
+        { channel = "exp_roles:assignment_update", data = { name = "bob", assign = { 5 } } },
-    eq(env.sent[1].data.assign, { 5 }, "the payload names the role")
+    }, "the assignment is sent to the controller")
     check(env.R("Moderator"):has_player(players.bob), "the role applies before confirmation")
-    check(#env.events == 1, "one event is raised")
+    deep_eq(env.events, {
-    eq(env.events[1].data.assigned, { 5 }, "the event carries the assigned role id")
+        {
-    check(env.events[1].data.by_player_index == 1, "the event carries who made the change")
+            name = env.Roles.events.on_player_roles_changed,
+            tick = 1,
+            player_index = 2,
+            by_player_index = 1,
+            assigned = { 5 },
+            unassigned = {},
+        },
+    }, "the event carries the change")
     check(#env.printed == 1 and env.printed[1][1] == "exp-roles.game-message-assign", "the change is announced")
     eq(env.sounds, { "bob:utility/achievement_unlocked" }, "the assign sound plays")
 
@@ -54,11 +61,20 @@ end)
 test("role:unassign removes a synced role", function(env)
     local players = setup(env)
     env.R("Regular"):unassign(players.bob, { by_player_name = "alice", silent = true })
-    check(#env.sent == 1, "the unassignment is sent")
+    deep_eq(env.sent, {
-    eq(env.sent[1].data.unassign, { 6 }, "the payload names the role")
+        { channel = "exp_roles:assignment_update", data = { name = "bob", unassign = { 6 } } },
+    }, "the unassignment is sent to the controller")
     check(not env.R("Regular"):has_player(players.bob), "the role is removed locally")
-    check(#env.events == 1, "one event is raised")
+    deep_eq(env.events, {
-    eq(env.events[1].data.unassigned, { 6 }, "the event carries the unassigned role id")
+        {
+            name = env.Roles.events.on_player_roles_changed,
+            tick = 1,
+            player_index = 2,
+            by_player_index = 1,
+            assigned = {},
+            unassigned = { 6 },
+        },
+    }, "the event carries the change")
     check(#env.printed == 0, "silent suppresses the announcement")
     eq(env.sounds, { "bob:utility/game_lost" }, "the unassign sound plays")
 end)
@@ -72,8 +88,16 @@ test("reject_assignment rolls the assignment back", function(env)
     env.Roles.reject_assignment{ name = "alice", role_ids = { 7 } }
     check(not env.R("Jail"):has_player(players.alice), "the rejected role is rolled back")
     check(#env.sent == 0, "the rollback is not sent to the controller")
-    check(#env.events == 1, "the rollback raises the event")
+    deep_eq(env.events, {
-    eq(env.events[1].data.unassigned, { 7 }, "the event carries the refused role id")
+        {
+            name = env.Roles.events.on_player_roles_changed,
+            tick = 1,
+            player_index = 1,
+            by_player_index = 0,
+            assigned = {},
+            unassigned = { 7 },
+        },
+    }, "the rollback raises the event")
 
     local sd = env.Roles._script_data()
     check(sd.local_players.alice == nil and sd.pending.alice == nil, "the rollback clears the local state")
@@ -100,20 +124,20 @@ end)
 test("role:assign of a higher priority role suppresses the rest", function(env)
     local players = setup(env)
     env.R("Jail"):assign(players.alice, { by_player_name = "<server>", silent = true })
-    eq(env.names(env.Roles.get_player_roles(players.alice)), { "Jail" }, "only the jail role applies")
+    eq(Suite.names(env.Roles.get_player_roles(players.alice)), { "Jail" }, "only the jail role applies")
     check(env.R("Moderator"):has_player(players.alice), "a suppressed role is still held")
     check(not env.Roles.player_has_permission(players.alice, "exp_scenario.command.kill"), "permissions are lost")
     check(not env.Roles.player_has_permission(players.alice, "exp_scenario.gui.readme"), "the default role is lost")
     check(not env.Roles.player_outranks(players.alice, players.bob), "a jailed player no longer outranks")
-    eq(env.events[1].data.assigned, { 7 }, "the event lists the jail role")
+    eq(env.events[1].assigned, { 7 }, "the event lists the jail role")
-    check(#env.events[1].data.unassigned == 0, "the suppressed roles are not listed as unassigned")
+    check(#env.events[1].unassigned == 0, "the suppressed roles are not listed as unassigned")
 
     env.reset_log()
     env.R("Jail"):unassign(players.alice, { by_player_name = "<server>", silent = true })
-    eq(Suite.sorted(env.names(env.Roles.get_player_roles(players.alice))), { "Moderator", "Player" },
+    eq(Suite.sorted(Suite.names(env.Roles.get_player_roles(players.alice))), { "Moderator", "Player" },
         "unjail restores the roles")
-    check(#env.events[1].data.assigned == 0, "the restored roles are not listed as assigned")
+    check(#env.events[1].assigned == 0, "the restored roles are not listed as assigned")
-    eq(env.events[1].data.unassigned, { 7 }, "the unjail event lists the jail role")
+    eq(env.events[1].unassigned, { 7 }, "the unjail event lists the jail role")
 end)
 
 test("role:assign ignores the server", function(env)

exp_roles/test/lua/env.lua

@@ -55,15 +55,6 @@ local function make_env()
         return (assert(env.Roles.get_role_by_name(name), "No role named " .. name))
     end
 
-    --- Names of an array of roles
-    function env.names(roles)
-        local rtn = {}
-        for index, role in ipairs(roles) do
-            rtn[index] = role.name
-        end
-        return rtn
-    end
-
     --- An assignment record as the controller would send it
     function env.assignment(name, role_ids)
         return { name = name, role_ids = role_ids }
@@ -78,13 +69,6 @@ local function make_env()
         }
     end
 
-    --- Save and load the map, as far as the roles module can tell
-    local stub_save_load = env.save_load
-    function env.save_load()
-        stub_save_load()
-        env.Roles.on_load()
-    end
-
     return env
 end
 

exp_roles/test/lua/holders.lua

@@ -38,9 +38,9 @@ end)
 test("role:get_players is limited to this map and can be filtered", function(env)
     setup(env)
     local mod = env.R("Moderator")
-    eq(Suite.sorted(env.names(mod:get_players())), { "alice", "dave" }, "players are limited to this map")
+    eq(Suite.sorted(Suite.names(mod:get_players())), { "alice", "dave" }, "players are limited to this map")
-    eq(env.names(mod:get_players(true)), { "alice" }, "filtered to connected players")
+    eq(Suite.names(mod:get_players(true)), { "alice" }, "filtered to connected players")
-    eq(env.names(mod:get_players(false)), { "dave" }, "filtered to offline players")
+    eq(Suite.names(mod:get_players(false)), { "dave" }, "filtered to offline players")
 end)
 
 test("role:print reaches online holders", function(env)

exp_roles/test/lua/lookup.lua

@@ -22,14 +22,14 @@ end)
 
 test("get_ordered_roles returns the most privileged first", function(env)
     env.initialise{}
-    eq(env.names(env.Roles.get_ordered_roles()), { "Cluster Admin", "Moderator", "Regular", "Jail", "Player" },
+    eq(Suite.names(env.Roles.get_ordered_roles()), { "Cluster Admin", "Moderator", "Regular", "Jail", "Player" },
         "roles follow their order")
 end)
 
 test("sort_roles orders most privileged first", function(env)
     env.initialise{}
     local sorted = env.Roles.sort_roles{ env.R("Player"), env.R("Cluster Admin"), env.R("Regular") }
-    eq(env.names(sorted), { "Cluster Admin", "Regular", "Player" }, "the list is sorted in place")
+    eq(Suite.names(sorted), { "Cluster Admin", "Regular", "Player" }, "the list is sorted in place")
 end)
 
 test("get_default_role follows is_default", function(env)
@@ -39,9 +39,9 @@ end)
 
 test("get_higher_roles and get_lower_roles include the role and exclude the default", function(env)
     env.initialise{}
-    eq(Suite.sorted(env.names(env.Roles.get_higher_roles(env.R("Regular")))),
+    eq(Suite.sorted(Suite.names(env.Roles.get_higher_roles(env.R("Regular")))),
         { "Cluster Admin", "Moderator", "Regular" }, "higher roles")
-    eq(Suite.sorted(env.names(env.Roles.get_lower_roles(env.R("Regular")))),
+    eq(Suite.sorted(Suite.names(env.Roles.get_lower_roles(env.R("Regular")))),
         { "Jail", "Regular" }, "lower roles")
 end)
 

exp_roles/test/lua/players.lua

@@ -18,16 +18,16 @@ end
 
 test("get_player_roles includes the default role", function(env)
     local players = setup(env)
-    eq(Suite.sorted(env.names(env.Roles.get_player_roles(players.alice))), { "Moderator", "Player" },
+    eq(Suite.sorted(Suite.names(env.Roles.get_player_roles(players.alice))), { "Moderator", "Player" },
         "held roles and the default role")
-    eq(env.names(env.Roles.get_player_roles(players.carol)), { "Player" },
+    eq(Suite.names(env.Roles.get_player_roles(players.carol)), { "Player" },
         "a player with no roles has the default role")
 end)
 
 test("get_player_roles treats nil and index 0 as the server", function(env)
     setup(env)
-    eq(env.names(env.Roles.get_player_roles(nil)), { "<server>" }, "nil is the server")
+    eq(Suite.names(env.Roles.get_player_roles(nil)), { "<server>" }, "nil is the server")
-    eq(env.names(env.Roles.get_player_roles(env.server)), { "<server>" }, "index 0 is the server")
+    eq(Suite.names(env.Roles.get_player_roles(env.server)), { "<server>" }, "index 0 is the server")
     check(env.Roles.player_has_permission(nil, "anything.at.all"), "the server has every permission")
     check(not env.R("Player"):has_player(nil), "the server does not have the default role")
 end)

exp_roles/test/lua/sync.lua

@@ -24,7 +24,7 @@ test("initialise applies triggers and raises empty events", function(env)
     setup(env)
     deep_eq(env.admin_state, { alice = true, carol = false }, "triggers run for connected players")
     check(#env.events == 2, "the event is raised once per connected player")
-    check(#env.events[1].data.assigned == 0 and #env.events[1].data.unassigned == 0, "the events are empty")
+    check(#env.events[1].assigned == 0 and #env.events[1].unassigned == 0, "the events are empty")
     check(#env.printed == 0 and #env.sent == 0 and #env.sounds == 0, "initialise is silent and sends nothing")
 end)
 
@@ -49,16 +49,23 @@ test("receive_assignment_updates applies controller changes", function(env)
     env.reset_log()
     env.Roles.receive_assignment_updates{ env.assignment("carol", { 5 }) }
     check(env.R("Moderator"):has_player(players.carol), "the assignment applies")
-    check(#env.events == 1, "the event is raised")
+    deep_eq(env.events, {
-    eq(env.events[1].data.assigned, { 5 }, "the event carries the role id")
+        {
-    check(env.events[1].data.by_player_index == 0, "controller changes have no by player")
+            name = env.Roles.events.on_player_roles_changed,
+            tick = 1,
+            player_index = 3,
+            by_player_index = 0,
+            assigned = { 5 },
+            unassigned = {},
+        },
+    }, "the event carries the change with no by player")
     check(#env.printed == 1 and env.printed[1][4] == "<server>", "the change is announced by the server")
     check(env.admin_state.carol == true, "triggers follow the assignment")
 
     env.reset_log()
     env.Roles.receive_assignment_updates{ { name = "carol", role_ids = {}, is_deleted = true } }
     check(not env.R("Moderator"):has_player(players.carol), "a removal applies")
-    eq(env.events[1].data.unassigned, { 5 }, "the removal raises the event")
+    eq(env.events[1].unassigned, { 5 }, "the removal raises the event")
     check(env.admin_state.carol == false, "triggers follow the removal")
 end)
 
@@ -77,7 +84,7 @@ test("receive_role_updates applies to the holders", function(env)
     }
     check(env.R("Regular"):has_permission("exp_scenario.command.jail"), "the permission change applies")
     check(#env.events == 2, "a role change raises for every connected player")
-    check(#env.events[1].data.assigned == 0, "role change events are empty")
+    check(#env.events[1].assigned == 0, "role change events are empty")
 
     env.Roles.receive_role_updates{
         { id = 6, name = "Regular", permissions = {}, meta = { id = 0, order = 3 }, is_deleted = true },
@@ -92,7 +99,7 @@ test("receive_role_updates moves the default role", function(env)
         { id = 8, name = "Guest", permissions = {}, meta = { id = 0, order = 7 }, is_default = true },
     }
     check(env.Roles.get_default_role() == env.R("Guest"), "the new default role is known")
-    eq(env.names(env.Roles.get_player_roles(players.carol)), { "Guest" }, "players pick up the new default")
+    eq(Suite.names(env.Roles.get_player_roles(players.carol)), { "Guest" }, "players pick up the new default")
 end)
 
 test("set_emit_events gates what is sent", function(env)
@@ -108,6 +115,7 @@ test("on_load restores the state after a save and load", function(env)
     local players = setup(env)
     env.R("Jail"):assign(players.carol, { silent = true, local_only = true })
     env.save_load()
+    env.Roles.on_load()
 
     check(env.R("Moderator"):is_higher_than(env.R("Regular")), "role methods survive through the registered metatable")
     check(env.R("Moderator"):has_player(players.alice), "synced roles survive")

test/lua/framework.lua

@@ -101,6 +101,18 @@ function Framework.new_suite(make_env)
         Suite.check(deep_equal(a, b), name, ("%s ~= %s"):format(repr(a, 1), repr(b, 1)))
     end
 
+    --- Names of an array of values with a name property, such as roles,
+    --- players, forces, or events
+    --- @param values { name: string }[]
+    --- @return string[]
+    function Suite.names(values)
+        local rtn = {}
+        for index, value in ipairs(values) do
+            rtn[index] = value.name
+        end
+        return rtn
+    end
+
     --- Sorted copy of an array of strings
     function Suite.sorted(list)
         local rtn = {}

test/lua/stubs.lua

@@ -65,7 +65,10 @@ function Stubs.new()
             return next_event_id
         end,
         raise_event = function(id, data)
-            stubs.events[#stubs.events + 1] = { id = id, data = data }
+            -- Factorio fills in the name and tick of the event
+            data.name = id
+            data.tick = game.tick
+            stubs.events[#stubs.events + 1] = data
         end,
         register_metatable = function(_, metatable)
             registered_metatables[metatable] = true
@@ -79,13 +82,17 @@ function Stubs.new()
         }),
     })
 
-    local players_by_name, players_by_index, connected = {}, {}, {}
+    -- Stored by name, with an index metamethod so players are also found by
+    -- their player index, the same way game.players works
+    local players_by_index = {}
+    local players = setmetatable({}, { __index = players_by_index })
+    local connected = {}
     game = Stubs.strict("LuaGameScript", {
         tick = 1,
-        players = players_by_name,
+        players = players,
         connected_players = connected,
         print = function(message) stubs.printed[#stubs.printed + 1] = message end,
-        get_player = function(key) return players_by_name[key] or players_by_index[key] end,
+        get_player = function(key) return players[key] end,
     }, { "player" })
 
     --- Add a player to the stubbed game
@@ -102,7 +109,7 @@ function Stubs.new()
                 stubs.printed[#stubs.printed + 1] = { to = name, message }
             end,
         })
-        players_by_name[name] = player
+        rawset(players, name, player)
         players_by_index[index] = player
         if player.connected then connected[#connected + 1] = player end
         return player