From da05ffa8c90ac1c251d37cf9bb73f17defdc3c75 Mon Sep 17 00:00:00 2001 From: Lucy Date: Wed, 29 May 2024 14:24:19 -0400 Subject: [PATCH] [Port] unit test fixes (#1982) * Fixes Ticked File Enforcement and Missing Unit Test (and makes said Unit Test Compile) (and genericizes the C&D list to the base unit test datum) (#77632) Closes #77631 Hey there, Ticked File Enforcement simply wasn't catching files that were missed. That's a bit stupid, so I decided to look into what the issue might be, and whoopsie daisies I did double periods back in #76592 (020ac2405308eab83f314282499dfe777aba5874). ![image](https://github.com/tgstation/tgstation/assets/34697715/6023afe8-313d-4550-9a60-58a8bc211b4f) I also added some debug info and some more checks to prevent such a break from happening again on runtime of this script. I thought it was a weird string concatenation issue (and not the simple break I thought it was), so I rewrote how it adds `glob`s. I think it's cleaner so I'll keep it anyhow This PR also corrects the oversight of the missing unit test (introduced in #77218 (69827604c46952dd4393db8617cd494ade17bea2)) by reticking it in the `_unit_tests.dm` file, and also makes it compile because it didn't do that. I also then had to do some more work to get the unit test to work. * Genericizes the Create-and-Destroy "ignore" list to be a static list on `/datum/unit_test` to allow it to be shared between these types of tests that we need to test. * Adds that list to C&D and the broken unit test regarding fantasy bonuses * Fixes some actually broken that the unit test was made to catch (beam rifles, butterdogs and other slippery items, random ingredient boxes). * Adds cases for things that the unit test and overall framework really shouldn't be altering anyways (mythril), and was likely causing inappropriate stack traces on master Unit Tests WORK. Tools WORK. ![image](https://github.com/tgstation/tgstation/assets/34697715/9a59c0db-7a33-4546-918b-c73372a5b867) :cl: fix: Beam rifles will no longer inappropriately retain any bonuses they may gain from wizardry. fix: Inappropriate stack traces over bonuses being applied to components that gain bonuses innately (like Mythril stacks) should cease. /:cl: * fix unticked unit test * Dumb fix --------- Co-authored-by: san7890 --- code/__DEFINES/traits.dm | 2 ++ code/__HELPERS/~monkestation-helpers/atoms.dm | 6 ++++ code/datums/components/fantasy/_fantasy.dm | 2 +- code/datums/components/slippery.dm | 1 + code/datums/materials/basemats.dm | 2 ++ code/game/objects/items.dm | 16 +++++++-- code/game/objects/items/storage/storage.dm | 7 ++++ .../projectiles/guns/energy/beam_rifle.dm | 2 +- code/modules/unit_tests/_unit_tests.dm | 2 ++ .../unit_tests/human_through_recycler.dm | 2 +- .../unit_tests/modify_fantasy_variable.dm | 35 ++++++++++++++++--- tgstation.dme | 1 + .../ticked_file_enforcement.py | 11 ++++-- 13 files changed, 75 insertions(+), 14 deletions(-) create mode 100644 code/__HELPERS/~monkestation-helpers/atoms.dm diff --git a/code/__DEFINES/traits.dm b/code/__DEFINES/traits.dm index 13f334151663..f7aee2496aff 100644 --- a/code/__DEFINES/traits.dm +++ b/code/__DEFINES/traits.dm @@ -293,6 +293,8 @@ Remember to update _globalvars/traits.dm if you're adding/removing/renaming trai #define TRAIT_ANTIMAGIC_NO_SELFBLOCK "anti_magic_no_selfblock" /// The user can do things like use magic staffs without penalty #define TRAIT_MAGICALLY_GIFTED "magically_gifted" +/// This object innately spawns with fantasy variables already applied (the magical component is given to it on initialize), and thus we never want to give it the component again. +#define TRAIT_INNATELY_FANTASTICAL_ITEM "innately_fantastical_item" #define TRAIT_DEPRESSION "depression" #define TRAIT_BLOOD_DEFICIENCY "blood_deficiency" #define TRAIT_JOLLY "jolly" diff --git a/code/__HELPERS/~monkestation-helpers/atoms.dm b/code/__HELPERS/~monkestation-helpers/atoms.dm new file mode 100644 index 000000000000..d25f0e33b555 --- /dev/null +++ b/code/__HELPERS/~monkestation-helpers/atoms.dm @@ -0,0 +1,6 @@ +/atom/proc/effective_contents(list/typecache = null) + var/static/list/default_typecache + if(!typecache) + default_typecache ||= typecacheof(list(/obj/effect, /atom/movable/screen)) + typecache = default_typecache + return typecache_filter_list_reverse(src.contents, typecache) diff --git a/code/datums/components/fantasy/_fantasy.dm b/code/datums/components/fantasy/_fantasy.dm index 31f969e61932..98796d52bc1b 100644 --- a/code/datums/components/fantasy/_fantasy.dm +++ b/code/datums/components/fantasy/_fantasy.dm @@ -14,7 +14,7 @@ ///affixes expects an initialized list /datum/component/fantasy/Initialize(quality, list/affixes = list(), canFail=FALSE, announce=FALSE) - if(!isitem(parent)) + if(!isitem(parent) || HAS_TRAIT(parent, TRAIT_INNATELY_FANTASTICAL_ITEM)) return COMPONENT_INCOMPATIBLE src.quality = quality diff --git a/code/datums/components/slippery.dm b/code/datums/components/slippery.dm index 7d809f321b48..1211b8096597 100644 --- a/code/datums/components/slippery.dm +++ b/code/datums/components/slippery.dm @@ -62,6 +62,7 @@ knockdown_time = source.reset_fantasy_variable("knockdown_time", knockdown_time) paralyze_time = source.reset_fantasy_variable("paralyze_time", paralyze_time) var/previous_lube_flags = LAZYACCESS(source.fantasy_modifications, "lube_flags") + LAZYREMOVE(source.fantasy_modifications, "lube_flags") if(!isnull(previous_lube_flags)) lube_flags = previous_lube_flags diff --git a/code/datums/materials/basemats.dm b/code/datums/materials/basemats.dm index 60d159085a82..f177a5c93fcf 100644 --- a/code/datums/materials/basemats.dm +++ b/code/datums/materials/basemats.dm @@ -325,10 +325,12 @@ Unless you know what you're doing, only use the first three numbers. They're in . = ..() if(isitem(source)) source.AddComponent(/datum/component/fantasy) + ADD_TRAIT(source, TRAIT_INNATELY_FANTASTICAL_ITEM, REF(src)) // DO THIS LAST OR WE WILL NEVER GET OUR BONUSES!!! /datum/material/mythril/on_removed_obj(atom/source, amount, material_flags) . = ..() if(isitem(source)) + REMOVE_TRAIT(source, TRAIT_INNATELY_FANTASTICAL_ITEM, REF(src)) // DO THIS FIRST OR WE WILL NEVER GET OUR BONUSES DELETED!!! qdel(source.GetComponent(/datum/component/fantasy)) /datum/material/mythril/on_accidental_mat_consumption(mob/living/carbon/victim, obj/item/source_item) diff --git a/code/game/objects/items.dm b/code/game/objects/items.dm index f0692f3df038..fcd7f2f57fe9 100644 --- a/code/game/objects/items.dm +++ b/code/game/objects/items.dm @@ -231,7 +231,7 @@ var/throw_verb /// A lazylist used for applying fantasy values, contains the actual modification applied to a variable. - var/list/fantasy_modifications + var/list/fantasy_modifications = null /obj/item/Initialize(mapload) if(attack_verb_continuous) @@ -1617,8 +1617,13 @@ /// Modifies the fantasy variable /obj/item/proc/modify_fantasy_variable(variable_key, value, bonus, minimum = 0) - if(LAZYACCESS(fantasy_modifications, variable_key) != null) + var/result = LAZYACCESS(fantasy_modifications, variable_key) + if(!isnull(result)) + if(HAS_TRAIT(src, TRAIT_INNATELY_FANTASTICAL_ITEM)) + return result // we are immune to your foul magicks you inferior wizard, we keep our bonuses + stack_trace("modify_fantasy_variable was called twice for the same key '[variable_key]' on type '[type]' before reset_fantasy_variable could be called!") + var/intended_target = value + bonus value = max(minimum, intended_target) @@ -1630,9 +1635,14 @@ /// Returns the original fantasy variable value /obj/item/proc/reset_fantasy_variable(variable_key, current_value) var/modification = LAZYACCESS(fantasy_modifications, variable_key) + + if(isnum(modification) && HAS_TRAIT(src, TRAIT_INNATELY_FANTASTICAL_ITEM)) + return modification // we are immune to your foul magicks you inferior wizard, we keep our bonuses the way they are + LAZYREMOVE(fantasy_modifications, variable_key) - if(!modification) + if(isnull(modification)) return current_value + return current_value - modification /obj/item/proc/apply_fantasy_bonuses(bonus) diff --git a/code/game/objects/items/storage/storage.dm b/code/game/objects/items/storage/storage.dm index 4ecb596ec50b..e8a9e8ebfecb 100644 --- a/code/game/objects/items/storage/storage.dm +++ b/code/game/objects/items/storage/storage.dm @@ -12,6 +12,9 @@ /obj/item/storage/apply_fantasy_bonuses(bonus) . = ..() + if(isnull(atom_storage)) // some abstract types of storage (yes i know) don't get a datum + return + atom_storage.max_slots = modify_fantasy_variable("max_slots", atom_storage.max_slots, round(bonus/2)) atom_storage.max_total_storage = modify_fantasy_variable("max_total_storage", atom_storage.max_total_storage, round(bonus/2)) LAZYSET(fantasy_modifications, "max_specific_storage", atom_storage.max_specific_storage) @@ -25,9 +28,13 @@ atom_storage.max_specific_storage = WEIGHT_CLASS_TINY /obj/item/storage/remove_fantasy_bonuses(bonus) + if(isnull(atom_storage)) // some abstract types of storage (yes i know) don't get a datum + return ..() + atom_storage.max_slots = reset_fantasy_variable("max_slots", atom_storage.max_slots) atom_storage.max_total_storage = reset_fantasy_variable("max_total_storage", atom_storage.max_total_storage) var/previous_max_storage = LAZYACCESS(fantasy_modifications, "max_specific_storage") + LAZYREMOVE(fantasy_modifications, "max_specific_storage") if(previous_max_storage) atom_storage.max_specific_storage = previous_max_storage return ..() diff --git a/code/modules/projectiles/guns/energy/beam_rifle.dm b/code/modules/projectiles/guns/energy/beam_rifle.dm index b7dd4c657da9..450fc4d8f210 100644 --- a/code/modules/projectiles/guns/energy/beam_rifle.dm +++ b/code/modules/projectiles/guns/energy/beam_rifle.dm @@ -78,7 +78,7 @@ . = ..() delay = modify_fantasy_variable("delay", delay, -bonus * 2) aiming_time = modify_fantasy_variable("aiming_time", aiming_time, -bonus * 2) - recoil = modify_fantasy_variable("aiming_time", aiming_time, round(-bonus / 2)) + recoil = modify_fantasy_variable("recoil", recoil, round(-bonus / 2)) /obj/item/gun/energy/beam_rifle/remove_fantasy_bonuses(bonus) delay = reset_fantasy_variable("delay", delay) diff --git a/code/modules/unit_tests/_unit_tests.dm b/code/modules/unit_tests/_unit_tests.dm index d28aa8124a4e..ae5669d104f1 100644 --- a/code/modules/unit_tests/_unit_tests.dm +++ b/code/modules/unit_tests/_unit_tests.dm @@ -128,6 +128,7 @@ #include "heretic_rituals.dm" #include "high_five.dm" #include "holidays.dm" +#include "human_through_recycler.dm" #include "hunger_curse.dm" #include "hydroponics_extractor_storage.dm" #include "hydroponics_validate_genes.dm" @@ -151,6 +152,7 @@ #include "mob_chains.dm" #include "mob_faction.dm" #include "mob_spawn.dm" +#include "modify_fantasy_variable.dm" #include "modsuit.dm" #include "modular_map_loader.dm" #include "mouse_bite_cable.dm" diff --git a/code/modules/unit_tests/human_through_recycler.dm b/code/modules/unit_tests/human_through_recycler.dm index 7d554d72690b..ad850f017b50 100644 --- a/code/modules/unit_tests/human_through_recycler.dm +++ b/code/modules/unit_tests/human_through_recycler.dm @@ -18,7 +18,7 @@ TEST_ASSERT(chewer.bloody, "The emagged recycler did not become bloody after crushing the assistant!") // Now, let's test to see if all of their clothing got properly deleted. - TEST_ASSERT_EQUAL(length(assistant.contents), 0, "Assistant still has items in its contents after being put through an emagged recycler!") + TEST_ASSERT_EQUAL(length(assistant.effective_contents()), 0, "Assistant still has items in its contents after being put through an emagged recycler!") // Consistent Assistants will always have the following: ID, PDA, backpack, a uniform, a headset, and a pair of shoes. If any of these are still present, then the recycler did not properly delete the assistant's clothing. // However, let's check for EVERYTHING just in case, because we don't want to miss anything. // This is just what we expect to be deleted. diff --git a/code/modules/unit_tests/modify_fantasy_variable.dm b/code/modules/unit_tests/modify_fantasy_variable.dm index d78c1d1dac74..72dc776af32c 100644 --- a/code/modules/unit_tests/modify_fantasy_variable.dm +++ b/code/modules/unit_tests/modify_fantasy_variable.dm @@ -1,21 +1,46 @@ // Unit test to make sure that there are no duplicate keys when modify_fantasy_variable is called when applying fantasy bonuses. // Also to make sure the fantasy_modifications list is null when fantasy bonuses are removed. +/datum/unit_test/modify_fantasy_variable + priority = TEST_LONGER + /datum/unit_test/modify_fantasy_variable/Run() + var/list/applicable_types = subtypesof(/obj/item) - uncreatables - for(var/obj/item/path as anything in subtypesof(/obj/item)) + for(var/obj/item/path as anything in applicable_types) var/obj/item/object = allocate(path) + // objects will have fantasy bonuses inherent to their type (like butterdogs and the slippery component), so we need to take this into account + var/number_of_extant_bonuses = LAZYLEN(object.fantasy_modifications) + +#define TEST_SUCCESS LAZYLEN(object.fantasy_modifications) == number_of_extant_bonuses + // Try positive object.apply_fantasy_bonuses(bonus = 5) object.remove_fantasy_bonuses(bonus = 5) - TEST_ASSERT_NULL(object.fantasy_modifications) + TEST_ASSERT(TEST_SUCCESS, generate_failure_message(object)) + // Then negative object.apply_fantasy_bonuses(bonus = -5) object.remove_fantasy_bonuses(bonus = -5) - TEST_ASSERT_NULL(object.fantasy_modifications) + TEST_ASSERT(TEST_SUCCESS, generate_failure_message(object)) + // Now try the extremes of each object.apply_fantasy_bonuses(bonus = 500) object.remove_fantasy_bonuses(bonus = 500) - TEST_ASSERT_NULL(object.fantasy_modifications) + TEST_ASSERT(TEST_SUCCESS, generate_failure_message(object)) + object.apply_fantasy_bonuses(bonus = -500) object.remove_fantasy_bonuses(bonus = -500) - TEST_ASSERT_NULL(object.fantasy_modifications) + TEST_ASSERT(TEST_SUCCESS, generate_failure_message(object)) + +/// Returns a string that we use to describe the failure of the test. +/datum/unit_test/modify_fantasy_variable/proc/generate_failure_message(obj/item/failed_object) + var/list/cached_modifications = failed_object.fantasy_modifications + var/length_of_modifications = LAZYLEN(cached_modifications) + var/list/failure_messages = list("Error found when adding+removing fantasy bonuses for [failed_object.type].") + failure_messages += "The length of the fantasy_modifications list was [length_of_modifications]." + if(length_of_modifications) + failure_messages += "The fantasy_modifications list was [cached_modifications.Join(", ")]." + + return failure_messages.Join(" ") + +#undef TEST_SUCCESS diff --git a/tgstation.dme b/tgstation.dme index 4fb2152e1028..0f57f218419a 100644 --- a/tgstation.dme +++ b/tgstation.dme @@ -554,6 +554,7 @@ #include "code\__HELPERS\sorts\TimSort.dm" #include "code\__HELPERS\~monkestation-helpers\announcements.dm" #include "code\__HELPERS\~monkestation-helpers\antags.dm" +#include "code\__HELPERS\~monkestation-helpers\atoms.dm" #include "code\__HELPERS\~monkestation-helpers\icon_smoothing.dm" #include "code\__HELPERS\~monkestation-helpers\roundend.dm" #include "code\__HELPERS\~monkestation-helpers\time.dm" diff --git a/tools/ticked_file_enforcement/ticked_file_enforcement.py b/tools/ticked_file_enforcement/ticked_file_enforcement.py index 59ac016b7533..86c399c73554 100644 --- a/tools/ticked_file_enforcement/ticked_file_enforcement.py +++ b/tools/ticked_file_enforcement/ticked_file_enforcement.py @@ -37,7 +37,7 @@ def post_error(string): post_error(f"Excluded file {full_file_path} does not exist, please remove it!") sys.exit(1) -file_extensions = (".dm", ".dmf") +file_extensions = ("dm", "dmf") reading = False lines = [] @@ -64,7 +64,12 @@ def post_error(string): scannable_files = [] for file_extension in file_extensions: - scannable_files += glob.glob(scannable_directory + f"**/*.{file_extension}", recursive=True) + compiled_directory = f"{scannable_directory}/**/*.{file_extension}" + scannable_files += glob.glob(compiled_directory, recursive=True) + +if len(scannable_files) == 0: + post_error(f"No files were found in {scannable_directory}. Ticked File Enforcement has failed!") + sys.exit(1) for code_file in scannable_files: dm_path = "" @@ -148,4 +153,4 @@ def compare_lines(a, b): post_error(f"The include at line {index + offset} is out of order ({line}, expected {sorted_lines[index]})") sys.exit(1) -print(green(f"Ticked File Enforcement: [{file_reference}] All includes are in order!")) +print(green(f"Ticked File Enforcement: [{file_reference}] All includes (for {len(scannable_files)} scanned files) are in order!"))