diff --git a/code/__DEFINES/dcs/flags.dm b/code/__DEFINES/dcs/flags.dm index 7543157319f..9b181e226a9 100644 --- a/code/__DEFINES/dcs/flags.dm +++ b/code/__DEFINES/dcs/flags.dm @@ -23,11 +23,15 @@ /// When this is used your Detach proc should have the same signature as your Attach proc #define ELEMENT_COMPLEX_DETACH (1 << 2) /** - * Stops lists used as arguments for the element from being sorted by the dcs_check_list_arguments unit test. - * For when changing the position of the keys is undesiderable, like for color matrices. + * Elements with this flag will have their datum lists arguments compared as is, + * without the contents being sorted alpha-numerically first. + * This is good for those elements where the position of the keys matter, like in the case of color matrices. */ #define ELEMENT_DONT_SORT_LIST_ARGS (1<<3) -/// Elements with this flag will be ignored by the test (I would rather put some faith than have contributors stringify connect loc lists). +/** + * Elements with this flag will be ignored by the dcs_check_list_arguments test. + * A good example is connect_loc, for which it's pratically undoable unless we force every signal proc to have a different name. + */ #define ELEMENT_NO_LIST_UNIT_TEST (1<<4) // How multiple components of the exact same type are handled in the same datum diff --git a/code/modules/unit_tests/dcs_check_list_arguments.dm b/code/modules/unit_tests/dcs_check_list_arguments.dm index 8089896ba3d..67d7417062b 100644 --- a/code/modules/unit_tests/dcs_check_list_arguments.dm +++ b/code/modules/unit_tests/dcs_check_list_arguments.dm @@ -1,18 +1,36 @@ /** - * list arguments for bespoke elements are treated just like any other datum: as a text ref in the ID. - * Using un-cached lists in AddElement() and RemoveElement() calls will just create new elements over - * and over. That's what this unit test is for. It's not a catch-all, but it does a decent job at it. + * list arguments for bespoke elements are treated as a text ref in the ID, like any other datum. + * Which means that, unless cached, using lists as arguments will lead to multiple instance of the same element + * being created over and over. + * + * Because of how it works, this unit test checks that these list datum args + * do not share similar contents (when rearranged in descending alpha-numerical order), to ensure that + * the least necessary amount of elements is created. So, using static lists may not be enough, + * for example, in the case of two different critters using the death_drops element to drop ectoplasm on death, since, + * despite being static lists, the two are different instances assigned to different mob types. + * + * Most of the time, you won't encounter two different static lists with similar contents used as element args, + * meaning using static lists is accepted. However, should that happen, it's advised to replace the instances + * with various string_x procs: lists, assoc_lists, assoc_nested_lists or numbers_list, depending on the type. + * + * In the case of an element where the position of the contents of each datum list argument is important, + * ELEMENT_DONT_SORT_LIST_ARGS should be added to its flags, to prevent such issues where the contents are similar + * when sorted, but the element instances are not. + * + * In the off-chance the element is not compatible with this unit test (such as for connect_loc et simila), + * you can also use ELEMENT_NO_LIST_UNIT_TEST so that they won't be processed by this unit test at all. */ /datum/unit_test/dcs_check_list_arguments /** - * This unit test requires every (tangible) atom to have been created at least once - * so its search is more accurate. That's why it's run after create_and_destroy. + * This unit test requires every (unless ignored) atom to have been created at least once + * for a more accurate search, which is why it's run after create_and_destroy is done running. */ priority = TEST_AFTER_CREATE_AND_DESTROY /datum/unit_test/dcs_check_list_arguments/Run() + var/we_failed = FALSE for(var/element_type in SSdcs.arguments_that_are_lists_by_element) - // Keeps tracks of the lists that shouldn't be compared with again. + // Keeps track of the lists that shouldn't be compared with again. var/list/to_ignore = list() var/list/superlist = SSdcs.arguments_that_are_lists_by_element[element_type] for(var/list/current as anything in superlist) @@ -27,7 +45,11 @@ bad_lists += list(compare) to_ignore[compare] = TRUE if(bad_lists) + we_failed = TRUE //Include the original, unsorted list in the report. It should be easier to find by the contributor. var/list/unsorted_list = superlist[current] - TEST_FAIL("found [length(bad_lists)] identical lists used as argument for element [element_type]. List: [json_encode(unsorted_list)].\n\ - Make sure it's a cached list, or use one of the string_list proc. Also, use the ELEMENT_DONT_SORT_LIST_ARGS flag if the key position of your lists matters.") + TEST_FAIL("Found [length(bad_lists)] datum list arguments with similar contents for [element_type]. Contents: [json_encode(unsorted_list)].") + ///Let's avoid sending the same instructions over and over, as it's just going to clutter the CI and confuse someone. + if(we_failed) + TEST_FAIL("Ensure that each list is static or cached. string_lists() (as well as similar procs) is your friend here.\n\ + Check the documentation from dcs_check_list_arguments.dm for more information!")