Paradise icon indicating copy to clipboard operation
Paradise copied to clipboard

Implement map tests for catching common errors.

Open warriorstar-orion opened this issue 2 years ago • 5 comments

DO NOT MERGE, this is just a prototype mostly for feedback from @AffectedArc07 and to see how it plays with CI

What Does This PR Do

  • 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 several 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
    • check to see if a tile contains an APC bump but no center node cable
    • check to see if a tile is a space turf and contains invalid objects such as airlocks
    • check to see if a tile is a space turf and contains a structure but is not nearspaced
  • Adds failure log

    • it was annoying during dev to try and catch errors when they were just logged to the world so here
    • it's not enabled by default because it changes the expectations of the CI runners (data/clean_run.lk is not emitted in these circumstances). it can be enabled by passing emit_failures = TRUE to the test runner's Finalize() proc.

When enabled, the failure log has a filename like data/test_run-2022-09-28T13_29_18.lk, and looks like this:

RUN  2022-09-28T13:29:18
FAIL Unit Tests failed!
FAIL /datum/map_per_tile_test/pipe_vent_checker
	127,89,2: pipe on same tile as vent or scrubber
	205,106,2: pipe on same tile as vent or scrubber
FAIL /datum/map_per_tile_test/cable_node_checker
	79,152,2: tile has multiple center cable nodes
	80,152,2: tile has multiple center cable nodes
	81,152,2: tile has multiple center cable nodes
	83,152,2: tile has multiple center cable nodes
	84,152,2: tile has multiple center cable nodes
	85,152,2: tile has multiple center cable nodes
	152,56,2: tile has multiple center cable nodes
	171,50,2: tile has multiple center cable nodes
	183,154,2: tile has multiple center cable nodes
PASS /datum/unit_test/aicard_icons
PASS /datum/unit_test/component_duping
PASS /datum/unit_test/config_sanity
PASS /datum/unit_test/crafting_lists
PASS /datum/unit_test/emote
FAIL /datum/unit_test/log_format
	RUSTG log format is not valid 8601 format. Expected '[2022-09-28T13:29:11] Log time format test', got '[2022-09-27T23:21:23] Log time format test'
PASS /datum/unit_test/map_templates
PASS /datum/unit_test/uplink_refs
PASS /datum/unit_test/spellbook_refs
PASS /datum/unit_test/reagent_id_typos
PASS /datum/unit_test/rustg_version
PASS /datum/unit_test/spawn_humans
PASS /datum/unit_test/spell_targeting
FAIL /datum/unit_test/sql_version
	SQL errors occured on startup. Please fix them.
PASS /datum/unit_test/subsystem_init
PASS /datum/unit_test/subsystem_metric_sanity
PASS /datum/unit_test/timer_sanity

Why It's Good For The Game

Automating checks for common map errors reduces cognitive overhead and maintenance burden for mappers and reviewers, and the payoff will extend forever into the future.

Testing

  1. Ran tests locally against all maps, and made fixes to all maps (#19208, #19211, #19213)
  2. Ran tests on CI

Changelog

:cl: add: Start building test suite specifically for map linting. /:cl:

warriorstar-orion avatar Sep 28 '22 17:09 warriorstar-orion

this is what they call mega poggers

S34NW avatar Sep 28 '22 17:09 S34NW

yes please

DogeDogIs avatar Sep 28 '22 17:09 DogeDogIs

PTAL. Also, currently it appears the "CI / Unit Tests + SQL Validation" will automatically cancel the unit tests for all maps when one returns a failure, instead of letting them all run to completion? I'm not sure if we want to keep that or not.

warriorstar-orion avatar Sep 28 '22 21:09 warriorstar-orion

@AffectedArc07 friendly ping about the test failure strategy for CI; it makes the most sense to set https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstrategyfail-fast to false for these tests as now they can return different results per map, and we don't want to kill the whole matrix when one fails because that would obscure the failures on other maps. If you'd like I can make that change here or after the fact.

warriorstar-orion avatar Oct 15 '22 16:10 warriorstar-orion

Yeah make it not kill all of them. Thats been an issue I have had for a while but never got round to fixing.

AffectedArc07 avatar Oct 15 '22 16:10 AffectedArc07

Yeah make it not kill all of them. Thats been an issue I have had for a while but never got round to fixing.

Should be good now.

warriorstar-orion avatar Oct 20 '22 14:10 warriorstar-orion

We should also test for multiple turfs on a given tile. This is technically legal as one turf will get added to the vis_contents of the other. But it leads to issues and we generally should forbid it.

S34NW avatar Oct 25 '22 14:10 S34NW

We should also test for multiple turfs on a given tile. This is technically legal as one turf will get added to the vis_contents of the other. But it leads to issues and we generally should forbid it.

@S34NW Since you have really in-depth mapping knowledge could you toss a to-do list in maybe a GitHub discussion of tests that should be added after this? Adding more tests now will mean the chance that existing maps will fail, which will require me to fix those in separate PRs before pulling this PR back to master in order for it to pass CI.

warriorstar-orion avatar Oct 29 '22 18:10 warriorstar-orion