From 574a0a4b529420a1a4da4199993eef1b6d75387d Mon Sep 17 00:00:00 2001 From: bbassie <17990055+bbassie@users.noreply.github.com> Date: Wed, 26 Aug 2026 10:48:43 +0000 Subject: [PATCH] Address final review nits on the assertions - Suite.empty(value, name) checks a table has no entries and reports what it held, and Suite.throws(fn, message, name) asserts a function errors with the given message, so an unrelated error no longer passes as the expected one. - Scalar comparisons and zero length checks go through eq and empty, and the announced messages and print output are compared by content, so a failure shows the actual data. - get_roles() is asserted through sorted names rather than a count. Co-Authored-By: Claude Fable 5 --- exp_roles/test/lua/assignment.lua | 30 ++++++++++++++++----------- exp_roles/test/lua/holders.lua | 6 +++--- exp_roles/test/lua/lookup.lua | 14 +++++++------ exp_roles/test/lua/players.lua | 4 +++- exp_roles/test/lua/sync.lua | 34 +++++++++++++++++++------------ test/lua/framework.lua | 20 ++++++++++++++++++ 6 files changed, 73 insertions(+), 35 deletions(-) diff --git a/exp_roles/test/lua/assignment.lua b/exp_roles/test/lua/assignment.lua index 406fa6fd..9a99dd58 100644 --- a/exp_roles/test/lua/assignment.lua +++ b/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, empty = Suite.test, Suite.check, Suite.eq, Suite.empty --- alice is a moderator and bob a regular, with changes sent to the controller local function setup(env) @@ -35,7 +35,9 @@ test(".assign() applies locally and is sent to the controller", function(env) 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.printed, { + { "exp-roles.game-message-assign", "bob", "Moderator", "alice" }, + }, "the change is announced") eq(env.sounds, { "bob:utility/achievement_unlocked" }, "the assign sound plays") local sd = env.Roles._script_data() @@ -44,14 +46,15 @@ test(".assign() applies locally and is sent to the controller", function(env) env.reset_log() moderator:assign(players.bob) - check(#env.sent == 0 and #env.events == 0, "assigning a held role is a no-op") + empty(env.sent, "assigning a held role sends nothing") + empty(env.events, "assigning a held role raises nothing") end) test(".assign() defaults the by player to game.player", function(env) local players = setup(env) game.player = players.alice env.R("Jail"):assign(players.bob, { silent = true }) - check(env.events[1].by_player_index == players.alice.index, "the acting player is game.player") + eq(env.events[1].by_player_index, players.alice.index, "the acting player is game.player") end) test("receive_assignment_updates() releases the local hold once confirmed", function(env) @@ -59,7 +62,8 @@ test("receive_assignment_updates() releases the local hold once confirmed", func env.R("Moderator"):assign(players.bob) env.reset_log() env.Roles.receive_assignment_updates{ env.assignment("bob", { "Regular", "Moderator" }) } - check(#env.events == 0 and #env.printed == 0, "a confirmation raises nothing") + empty(env.events, "a confirmation raises no events") + empty(env.printed, "a confirmation announces nothing") local sd = env.Roles._script_data() check(sd.local_players.bob == nil and sd.pending.bob == nil, "the role is no longer held locally") @@ -84,7 +88,7 @@ test(".unassign() removes a synced role", function(env) unassigned = { regular.id }, }, }, "the event carries the change") - check(#env.printed == 0, "silent suppresses the announcement") + empty(env.printed, "silent suppresses the announcement") eq(env.sounds, { "bob:utility/game_lost" }, "the unassign sound plays") end) @@ -97,7 +101,7 @@ test("reject_assignment() rolls the assignment back", function(env) env.reset_log() env.Roles.reject_assignment{ name = "alice", role_ids = { jail.id } } check(not jail:has_player(players.alice), "the rejected role is rolled back") - check(#env.sent == 0, "the rollback is not sent to the controller") + empty(env.sent, "the rollback is not sent to the controller") eq(env.events, { { name = env.Roles.events.on_player_roles_changed, @@ -117,7 +121,7 @@ test(".assign() with local_only never reaches the controller", function(env) local players = setup(env) local regular = env.R("Regular") regular:assign(players.alice, { silent = true, local_only = true }) - check(#env.sent == 0, "the assignment is not sent") + empty(env.sent, "the assignment is not sent") check(regular:has_player(players.alice), "the role applies") local sd = env.Roles._script_data() @@ -129,7 +133,8 @@ test(".assign() with local_only never reaches the controller", function(env) env.reset_log() regular:unassign(players.alice, { local_only = true }) - check(not regular:has_player(players.alice) and #env.sent == 0, "removed without sync") + check(not regular:has_player(players.alice), "the role is removed") + empty(env.sent, "the removal is not sent") end) test(".assign() of a higher priority role suppresses the rest", function(env) @@ -142,20 +147,21 @@ test(".assign() of a higher priority role suppresses the rest", function(env) 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].assigned, { jail.id }, "the event lists the jail role") - check(#env.events[1].unassigned == 0, "the suppressed roles are not listed as unassigned") + empty(env.events[1].unassigned, "the suppressed roles are not listed as unassigned") env.reset_log() jail:unassign(players.alice, { by_player_name = "", silent = true }) eq(Suite.sorted(Suite.names(env.Roles.get_player_roles(players.alice))), { "Moderator", "Player" }, "unjail restores the roles") - check(#env.events[1].assigned == 0, "the restored roles are not listed as assigned") + empty(env.events[1].assigned, "the restored roles are not listed as assigned") eq(env.events[1].unassigned, { jail.id }, "the unjail event lists the jail role") end) test(".assign() ignores the server", function(env) setup(env) env.R("Moderator"):assign(env.server) - check(#env.sent == 0 and #env.events == 0, "assigning to the server is a no-op") + empty(env.sent, "assigning to the server sends nothing") + empty(env.events, "assigning to the server raises nothing") end) return Suite.run() diff --git a/exp_roles/test/lua/holders.lua b/exp_roles/test/lua/holders.lua index 486767c8..8ee4c91f 100644 --- a/exp_roles/test/lua/holders.lua +++ b/exp_roles/test/lua/holders.lua @@ -46,9 +46,9 @@ end) test(".print() reaches online holders", function(env) setup(env) env.reset_log() - check(env.R("Moderator"):print("hello") == 1, "print returns the number of players reached") - check(#env.printed == 1 and env.printed[1].to == "alice", "only online holders are reached") - check(env.R("Jail"):print("hello") == 0, "a role with no online holders reaches nobody") + eq(env.R("Moderator"):print("hello"), 1, "print returns the number of players reached") + eq(env.printed, { { "hello", to = "alice" } }, "only online holders receive the message") + eq(env.R("Jail"):print("hello"), 0, "a role with no online holders reaches nobody") end) test(".print() over get_higher_roles() can repeat", function(env) diff --git a/exp_roles/test/lua/lookup.lua b/exp_roles/test/lua/lookup.lua index 56db829d..2479e6c8 100644 --- a/exp_roles/test/lua/lookup.lua +++ b/exp_roles/test/lua/lookup.lua @@ -5,18 +5,20 @@ local test, check, eq = Suite.test, Suite.check, Suite.eq test("get_role() returns the role with the given clusterio id", function(env) env.initialise{} check(env.Roles.get_role(env.R("Regular").id) == env.R("Regular"), "the role is found by id") - check(env.Roles.get_role(9) == nil, "deleted roles are not known") + eq(env.Roles.get_role(9), nil, "deleted roles are not known") end) test("get_role_by_name() searches the roles", function(env) env.initialise{} check(env.Roles.get_role_by_name("Moderator") == env.R("Moderator"), "the role is found by name") - check(env.Roles.get_role_by_name("Gone") == nil, "deleted roles are not known") + eq(env.Roles.get_role_by_name("Gone"), nil, "deleted roles are not known") end) test("get_roles() returns every role", function(env) env.initialise{} - check(#env.Roles.get_roles() == 5, "every role is returned with deleted roles skipped") + eq(Suite.sorted(Suite.names(env.Roles.get_roles())), + { "Cluster Admin", "Jail", "Moderator", "Player", "Regular" }, + "every role is returned with deleted roles skipped") end) test("get_ordered_roles() returns the most privileged first", function(env) @@ -60,9 +62,9 @@ end) test("initialise() decodes the role records", function(env) env.initialise{} - check(env.R("Cluster Admin").short_hand == "SYS", "short hand decoded") - check(env.R("Moderator").short_hand == "Moderator", "short hand falls back to the name") - check(env.R("Moderator").color.g == 170, "color decoded") + eq(env.R("Cluster Admin").short_hand, "SYS", "short hand decoded") + eq(env.R("Moderator").short_hand, "Moderator", "short hand falls back to the name") + eq(env.R("Moderator").color, { r = 0, g = 170, b = 0 }, "color decoded") check(env.R("Jail").priority == 1 and env.R("Jail").block_auto_assign, "priority and block auto assign decoded") end) diff --git a/exp_roles/test/lua/players.lua b/exp_roles/test/lua/players.lua index 57f85ded..a13dce6c 100644 --- a/exp_roles/test/lua/players.lua +++ b/exp_roles/test/lua/players.lua @@ -43,7 +43,9 @@ test("get_player_highest_role() errors when a player has no roles", function(env env.Roles.receive_role_updates{ { id = env.R("Player").id, name = "Player", permissions = {}, meta = { id = 0, order = 5 }, is_deleted = true }, } - check(not pcall(env.Roles.get_player_highest_role, players.carol), "no default role and no held roles errors") + Suite.throws(function() + env.Roles.get_player_highest_role(players.carol) + end, "Player has no roles", "no default role and no held roles errors") end) test("player_has_permission()", function(env) diff --git a/exp_roles/test/lua/sync.lua b/exp_roles/test/lua/sync.lua index 4bdc043a..2a94875d 100644 --- a/exp_roles/test/lua/sync.lua +++ b/exp_roles/test/lua/sync.lua @@ -1,6 +1,6 @@ --- Tests for the sync entry points 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, empty = Suite.test, Suite.check, Suite.eq, Suite.empty --- alice is a moderator and carol has only the default role, both connected local function setup(env) @@ -23,9 +23,12 @@ end test("initialise() applies triggers and raises empty events", function(env) setup(env) 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].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") + eq(#env.events, 2, "the event is raised once per connected player") + empty(env.events[1].assigned, "the events assign nothing") + empty(env.events[1].unassigned, "the events unassign nothing") + empty(env.printed, "initialise is silent") + empty(env.sent, "initialise sends nothing") + empty(env.sounds, "initialise plays no sounds") end) test("initialise() skips deleted assignments", function(env) @@ -46,10 +49,10 @@ test("initialise() is authoritative for pending roles", function(env) env.reset_log() env.initialise{ env.assignment("alice", { "Moderator" }) } - check(sd.pending.carol == nil, "pending roles are cleared") + eq(sd.pending.carol, nil, "pending roles are cleared") check(not env.R("Moderator"):has_player(players.carol), "an unconfirmed role is given up") eq(sd.local_players.carol, { env.R("Jail").id }, "local only roles are kept") - check(#env.sent == 0, "initialise sends nothing") + empty(env.sent, "initialise sends nothing") end) test("receive_assignment_updates() applies controller changes", function(env) @@ -67,7 +70,9 @@ test("receive_assignment_updates() applies controller changes", function(env) unassigned = {}, }, }, "the event carries the change with no by player") - check(#env.printed == 1 and env.printed[1][4] == "", "the change is announced by the server") + eq(env.printed, { + { "exp-roles.game-message-assign", "carol", "Moderator", "" }, + }, "the change is announced by the server") check(env.admin_state.carol == true, "triggers follow the assignment") env.reset_log() @@ -81,7 +86,8 @@ test("receive_assignment_updates() for players not on this map raises nothing", setup(env) env.reset_log() env.Roles.receive_assignment_updates{ env.assignment("zed", { "Moderator" }) } - check(#env.events == 0 and #env.printed == 0, "no event and no announcement") + empty(env.events, "no event is raised") + empty(env.printed, "nothing is announced") end) test("receive_role_updates() applies to the holders", function(env) @@ -92,13 +98,13 @@ test("receive_role_updates() applies to the holders", function(env) { id = regular_id, name = "Regular", permissions = { "exp_scenario.command.jail" }, meta = { id = 0, order = 3 } }, } 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].assigned == 0, "role change events are empty") + eq(#env.events, 2, "a role change raises for every connected player") + empty(env.events[1].assigned, "role change events are empty") env.Roles.receive_role_updates{ { id = regular_id, name = "Regular", permissions = {}, meta = { id = 0, order = 3 }, is_deleted = true }, } - check(env.Roles.get_role(regular_id) == nil, "a deleted role is removed") + eq(env.Roles.get_role(regular_id), nil, "a deleted role is removed") end) test("receive_role_updates() moves the default role", function(env) @@ -116,12 +122,14 @@ test("set_emit_events() gates what is sent and defaults to enabled", function(en env.Roles.set_emit_events(false) env.reset_log() env.R("Regular"):assign(players.alice) - check(#env.sent == 0, "nothing is sent while disabled") + empty(env.sent, "nothing is sent while disabled") check(env.R("Regular"):has_player(players.alice), "the assignment still applies locally") env.Roles.set_emit_events() env.R("Jail"):assign(players.alice, { silent = true }) - check(#env.sent == 1, "no argument enables sending") + eq(env.sent, { + { channel = "exp_roles:assignment_update", data = { name = "alice", assign = { env.R("Jail").id } } }, + }, "no argument enables sending") end) test("on_load() restores the state after a save and load", function(env) diff --git a/test/lua/framework.lua b/test/lua/framework.lua index dfda4fc9..eadca54c 100644 --- a/test/lua/framework.lua +++ b/test/lua/framework.lua @@ -91,6 +91,26 @@ function Framework.suite(extend_env) Suite.check(deep_equal(a, b), name, ("%s ~= %s"):format(repr(a, 1), repr(b, 1))) end + --- Check a table has no entries + function Suite.empty(value, name) + Suite.check(next(value) == nil, name, repr(value, 1)) + end + + --- Check a function raises an error containing the message + --- @param fn fun() + --- @param message string + --- @param name string + function Suite.throws(fn, message, name) + local ok, err = pcall(fn) + if ok then + Suite.fail(name, "did not error") + elseif not tostring(err):find(message, 1, true) then + Suite.fail(name, ("%s does not contain %s"):format(tostring(err), message)) + else + Suite.pass(name) + end + end + --- Names of an array of values with a name property, such as roles, --- players, forces, or events --- @param values { name: string }[]