From 4ace2d6c2b01b6a92df74afa70376c8354ed4d91 Mon Sep 17 00:00:00 2001 From: warriorstar-orion Date: Sat, 5 Nov 2022 11:32:17 -0400 Subject: [PATCH] Implement map tests for catching common errors. (#19204) * Implement map tests for catching common errors. - Adds test runner: - to make it easier to track things across test types - for example to ensure a fully specified log can be emitted - Adds map tile test type: - when writing a test, coders implement CheckTile, which is handed a single turf - when the test runner runs these tests, it iterates over all turfs in the specified z-level, and runs each test's CheckTile on each turf in turn. - Adds two sample map tile tests: - check to see if a pipe exists on the same tile as a scrubber or vent - check to see if a tile contains two cables, each with a center node * Review #1: - Replace nested loops over map tiles with `block` - Remove check for valid turf in individual tests, I think it's safe to assume `block` will always return legit turfs - Added proper duration tracking for old tests - Gave log file an appropriate extension - Actually use `Fail` for tests * whoops * add more tests suggested by @Vi3trice * Add some more tests courtesy @Bm0n and @Vi3trice * windows are okay in space as long as it's nearspace * Add failure threshold to prevent excessive logging. Once this threshold is reached, a test will stop being processed for every tile. Note that this applies to `log_world` and `text2file` equally when logging large amounts of failures. * Document each test. * Remove unnecessary reboot * Let all map tests run to completion in CI matrix. * I know what alphabetical means --- .github/workflows/ci.yml | 1 + code/controllers/subsystem/ticker.dm | 4 +- code/game/world.dm | 9 +- code/modules/unit_tests/_unit_tests.dm | 4 +- code/modules/unit_tests/map_tests.dm | 111 ++++++++++++++++++++++++ code/modules/unit_tests/test_runner.dm | 114 +++++++++++++++++++++++++ code/modules/unit_tests/unit_test.dm | 62 -------------- 7 files changed, 239 insertions(+), 66 deletions(-) create mode 100644 code/modules/unit_tests/map_tests.dm create mode 100644 code/modules/unit_tests/test_runner.dm diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index da305acf3d7..ff2b4fbf7b3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -56,6 +56,7 @@ jobs: name: Unit Tests + SQL Validation runs-on: ubuntu-20.04 strategy: + fail-fast: false # Let all map tests run to completion matrix: maptype: ['/datum/map/cyberiad', '/datum/map/delta', '/datum/map/metastation'] byondtype: ['STABLE', 'BETA'] diff --git a/code/controllers/subsystem/ticker.dm b/code/controllers/subsystem/ticker.dm index b4c56bbeb90..35598ab3944 100644 --- a/code/controllers/subsystem/ticker.dm +++ b/code/controllers/subsystem/ticker.dm @@ -329,7 +329,9 @@ SUBSYSTEM_DEF(ticker) SSnightshift.check_nightshift(TRUE) #ifdef UNIT_TESTS - RunUnitTests() + // Run map tests first in case unit tests futz with map state + GLOB.test_runner.RunMap() + GLOB.test_runner.Run() #endif // Do this 10 second after roundstart because of roundstart lag, and make it more visible diff --git a/code/game/world.dm b/code/game/world.dm index a62f7395050..59f53b8c02c 100644 --- a/code/game/world.dm +++ b/code/game/world.dm @@ -1,5 +1,9 @@ GLOBAL_LIST_INIT(map_transition_config, list(CC_TRANSITION_CONFIG)) +#ifdef UNIT_TESTS +GLOBAL_DATUM(test_runner, /datum/test_runner) +#endif + /world/New() // IMPORTANT // If you do any SQL operations inside this proc, they must ***NOT*** be ran async. Otherwise players can join mid query @@ -66,7 +70,8 @@ GLOBAL_LIST_INIT(map_transition_config, list(CC_TRANSITION_CONFIG)) #ifdef UNIT_TESTS - HandleTestRun() + GLOB.test_runner = new + GLOB.test_runner.Start() #endif @@ -140,7 +145,7 @@ GLOBAL_LIST_EMPTY(world_topic_handlers) // If we were running unit tests, finish that run #ifdef UNIT_TESTS - FinishTestRun() + GLOB.test_runner.Finalize() return #endif diff --git a/code/modules/unit_tests/_unit_tests.dm b/code/modules/unit_tests/_unit_tests.dm index f085af33153..5147bf47b6e 100644 --- a/code/modules/unit_tests/_unit_tests.dm +++ b/code/modules/unit_tests/_unit_tests.dm @@ -2,6 +2,7 @@ //Keep this sorted alphabetically #ifdef UNIT_TESTS +#include "aicard_icons.dm" #include "announcements.dm" #include "component_tests.dm" #include "config_sanity.dm" @@ -9,6 +10,7 @@ #include "emotes.dm" #include "log_format.dm" #include "map_templates.dm" +#include "map_tests.dm" #include "purchase_reference_test.dm" #include "reagent_id_typos.dm" #include "rustg_version.dm" @@ -17,7 +19,7 @@ #include "sql.dm" #include "subsystem_init.dm" #include "subsystem_metric_sanity.dm" +#include "test_runner.dm" #include "timer_sanity.dm" #include "unit_test.dm" -#include "aicard_icons.dm" #endif diff --git a/code/modules/unit_tests/map_tests.dm b/code/modules/unit_tests/map_tests.dm new file mode 100644 index 00000000000..bb6cfc3264b --- /dev/null +++ b/code/modules/unit_tests/map_tests.dm @@ -0,0 +1,111 @@ +/** + * Map per-tile test. + * + * Per-tile map tests iterate over each tile of a map to perform a check, and + * fails the test if a tile does not pass the check. A new test can be + * written by extending /datum/map_per_tile_test, and implementing the check + * in CheckTile. + */ +/datum/map_per_tile_test + var/succeeded = TRUE + var/list/fail_reasons + var/failure_count = 0 + +/datum/map_per_tile_test/proc/CheckTile(turf/T) + Fail("CheckTile() called parent or not implemented") + +/datum/map_per_tile_test/proc/Fail(turf/T, reason) + succeeded = FALSE + LAZYADD(fail_reasons, "[T.x],[T.y],[T.z]: [reason]") + failure_count++ + +/** + * Check atmos pipes are not routed under unary devices such as vents and + * scrubbers. + */ +/datum/map_per_tile_test/pipe_vent_checker + var/list/pipe_roots = list( + /obj/machinery/atmospherics/pipe/manifold/hidden/supply, + /obj/machinery/atmospherics/pipe/simple/hidden/scrubbers + ) + var/list/unary_roots = list( + /obj/machinery/atmospherics/unary + ) + +/datum/map_per_tile_test/pipe_vent_checker/CheckTile(turf/T) + var/has_pipe = FALSE + var/has_unary = FALSE + + for(var/pipe_root in pipe_roots) + if(locate(pipe_root) in T.contents) + has_pipe = TRUE + + if(locate(/obj/machinery/atmospherics/unary) in T.contents) + has_unary = TRUE + + if(has_pipe && has_unary) + Fail(T, "pipe on same tile as vent or scrubber") + +/** + * Check that only one cable node exists on a tile. + */ +/datum/map_per_tile_test/cable_node_checker + +/datum/map_per_tile_test/cable_node_checker/CheckTile(turf/T) + var/center_nodes = 0 + + for(var/obj/structure/cable/cable in T.contents) + if(cable.d1 == 0 || cable.d2 == 0) + center_nodes++ + + if(center_nodes > 1) + Fail(T, "tile has multiple center cable nodes") + +/** + * Check to ensure that APCs have a cable node on their tile. + */ +/datum/map_per_tile_test/apc_cable_node_checker + +/datum/map_per_tile_test/apc_cable_node_checker/CheckTile(turf/T) + var/missing_node = TRUE + var/obj/machinery/power/apc/apc = locate(/obj/machinery/power/apc) in T.contents + if(apc) + for(var/obj/structure/cable/cable in T.contents) + if(cable.d1 == 0 || cable.d2 == 0) + missing_node = FALSE + + if(missing_node) + Fail(T, "tile has an APC bump but no center cable node") + +/** + * Check to ensure pipe trunks exist under disposals devices. + */ +/datum/map_per_tile_test/disposal_with_trunk_checker + +/datum/map_per_tile_test/disposal_with_trunk_checker/CheckTile(turf/T) + var/obj/machinery/disposal/disposal = locate(/obj/machinery/disposal) in T.contents + if(disposal) + if(!locate(/obj/structure/disposalpipe/trunk) in T.contents) + Fail(T, "tile has disposal unit/chute but no pipe trunk") + +/** + * Check for certain objects that should never be over space turfs. + */ +/datum/map_per_tile_test/invalid_objs_over_space_checker + var/list/invalid_types = list( + /obj/machinery/door/airlock + ) + +/datum/map_per_tile_test/invalid_objs_over_space_checker/CheckTile(turf/T) + for(var/invalid_type in invalid_types) + if(isspaceturf(T) && locate(invalid_type) in T.contents) + Fail(T, "space turf contains at least one invalid object of type [invalid_type]") + +/** + * Check that structures in space are always in near-station space. + */ +/datum/map_per_tile_test/structures_in_farspace_checker + +/datum/map_per_tile_test/structures_in_farspace_checker/CheckTile(turf/T) + if(T.loc.type == /area/space && locate(/obj/structure) in T.contents) + Fail(T, "tile contains at least one structure found in non-near space area") diff --git a/code/modules/unit_tests/test_runner.dm b/code/modules/unit_tests/test_runner.dm new file mode 100644 index 00000000000..47dac9e79bf --- /dev/null +++ b/code/modules/unit_tests/test_runner.dm @@ -0,0 +1,114 @@ +// Logging large amounts of test failures can cause performance issues +// significant enough to hit GitHub Actions timeout thresholds. This can happen +// intentionally (if there's a lot of legitimate map errors), or accidentally if +// a test condition is written incorrectly and starts e.g. logging failures for +// every single tile. +#define MAX_MAP_TEST_FAILURE_COUNT 20 + +/datum/test_runner + var/datum/unit_test/current_test + var/failed_any_test = FALSE + var/list/test_logs = list() + var/list/durations = list() + +/datum/test_runner/proc/Start() + //trigger things to run the whole process + Master.sleep_offline_after_initializations = FALSE + // This will have the ticker set the game up + // Running the tests is part of the ticker's start function, because I cant think of any better place to put it + SSticker.force_start = TRUE + + +/datum/test_runner/proc/RunMap(z_level = 2) + CHECK_TICK + + var/list/tests = list() + + for(var/I in subtypesof(/datum/map_per_tile_test)) + tests += new I + test_logs[I] = list() + durations[I] = 0 + + for(var/turf/T in block(locate(1, 1, z_level), locate(world.maxx, world.maxy, z_level))) + for(var/datum/map_per_tile_test/test in tests) + if(test.failure_count < MAX_MAP_TEST_FAILURE_COUNT) + var/duration = REALTIMEOFDAY + test.CheckTile(T) + durations[test.type] += REALTIMEOFDAY - duration + + if(test.failure_count >= MAX_MAP_TEST_FAILURE_COUNT) + test.Fail(T, "failure threshold reached at this tile") + + for(var/datum/map_per_tile_test/test in tests) + if(!test.succeeded) + failed_any_test = TRUE + test_logs[test.type] += test.fail_reasons + + QDEL_LIST(tests) + + +/datum/test_runner/proc/Run() + CHECK_TICK + + for(var/I in subtypesof(/datum/unit_test)) + var/datum/unit_test/test = new I + test_logs[I] = list() + + current_test = test + var/duration = REALTIMEOFDAY + + test.Run() + + durations[I] = REALTIMEOFDAY - duration + current_test = null + + if(!test.succeeded) + failed_any_test = TRUE + test_logs[I] += test.fail_reasons + + qdel(test) + + CHECK_TICK + + SSticker.reboot_helper("Unit Test Reboot", "tests ended", 0) + + +/datum/test_runner/proc/Finalize(emit_failures = FALSE) + var/time = world.timeofday + set waitfor = FALSE + var/list/fail_reasons + if(GLOB) + if(GLOB.total_runtimes != 0) + fail_reasons = list("Total runtimes: [GLOB.total_runtimes]") + if(!GLOB.log_directory) + LAZYADD(fail_reasons, "Missing GLOB.log_directory!") + if(failed_any_test) + LAZYADD(fail_reasons, "Unit Tests failed!") + else + fail_reasons = list("Missing GLOB!") + + if(!fail_reasons) + text2file("Success!", "data/clean_run.lk") + + var/list/result = list() + result += "RUN [time2text(time, "YYYY-MM-DD")]T[time2text(time, "hh:mm:ss")]" + + for(var/reason in fail_reasons) + result += "FAIL [reason]" + + for(var/test in test_logs) + if(length(test_logs[test]) == 0) + result += "PASS [test] [durations[test] / 10]s" + else + result += "FAIL [test] [durations[test] / 10]s" + result += "\t" + test_logs[test].Join("\n\t") + + for(var/entry in result) + log_world(entry) + + if(emit_failures) + var/filename = "data/test_run-[time2text(time, "YYYY-MM-DD")]T[time2text(time, "hh_mm_ss")].log" + text2file(result.Join("\n"), filename) + + sleep(0) //yes, 0, this'll let Reboot finish and prevent byond memes + del(world) //shut it down diff --git a/code/modules/unit_tests/unit_test.dm b/code/modules/unit_tests/unit_test.dm index 8fd15ce81cc..6ef3c82d85c 100644 --- a/code/modules/unit_tests/unit_test.dm +++ b/code/modules/unit_tests/unit_test.dm @@ -6,11 +6,6 @@ You may use /New() and /Destroy() for setup/teardown respectively You can use the run_loc_bottom_left and run_loc_top_right to get turfs for testing */ -/// VARS FOR UNIT TESTS -GLOBAL_DATUM(current_test, /datum/unit_test) -GLOBAL_VAR_INIT(failed_any_test, FALSE) -GLOBAL_VAR(test_log) - /datum/unit_test //Bit of metadata for the future maybe var/list/procs_tested @@ -43,60 +38,3 @@ GLOBAL_VAR(test_log) reason = "FORMATTED: [reason != null ? reason : "NULL"]" LAZYADD(fail_reasons, reason) - -/proc/RunUnitTests() - CHECK_TICK - - for(var/I in subtypesof(/datum/unit_test)) - var/datum/unit_test/test = new I - - GLOB.current_test = test - var/duration = REALTIMEOFDAY - - test.Run() - - duration = REALTIMEOFDAY - duration - GLOB.current_test = null - GLOB.failed_any_test |= !test.succeeded - - var/list/log_entry = list("[test.succeeded ? "PASS" : "FAIL"]: [I] [duration / 10]s") - var/list/fail_reasons = test.fail_reasons - - qdel(test) - - for(var/J in 1 to LAZYLEN(fail_reasons)) - log_entry += "\tREASON #[J]: [fail_reasons[J]]" - log_world(log_entry.Join("\n")) - - CHECK_TICK - - SSticker.reboot_helper("Unit Test Reboot", "tests ended", 0) - - -// OTHER MISC PROCS RELATED TO UNIT TESTS // - -/world/proc/HandleTestRun() - //trigger things to run the whole process - Master.sleep_offline_after_initializations = FALSE - // This will have the ticker set the game up - // Running the tests is part of the ticker's start function, because I cant think of any better place to put it - SSticker.force_start = TRUE - -/world/proc/FinishTestRun() - set waitfor = FALSE - var/list/fail_reasons - if(GLOB) - if(GLOB.total_runtimes != 0) - fail_reasons = list("Total runtimes: [GLOB.total_runtimes]") - if(!GLOB.log_directory) - LAZYADD(fail_reasons, "Missing GLOB.log_directory!") - if(GLOB.failed_any_test) - LAZYADD(fail_reasons, "Unit Tests failed!") - else - fail_reasons = list("Missing GLOB!") - if(!fail_reasons) - text2file("Success!", "data/clean_run.lk") - else - log_world("Test run failed!\n[fail_reasons.Join("\n")]") - sleep(0) //yes, 0, this'll let Reboot finish and prevent byond memes - del(src) //shut it down