Files
Bubberstation/code/modules/unit_tests
Timberpoes 882e7db58f Stack traces when a cliented or ckeyed mob is Destroy()ed. (#72797)
## About The Pull Request

One thing that repeatedly pops up in admin channels is investigating
causes of death when a player just vanishes from the game. These are
almost universally qdeletion issues.

9 years ago `/mob/Destroy()` was commented with:
`//This makes sure that mobs with clients/keys are not just deleted from
the game.`

Whatever code may have existed back then has long since been replaced.

**I consider that Destroy()ing a cliented or ckeyed mob is a runtime
error case.**

Code which may result in deleting a mob should handle removing and/or
reassigning any client or ckey - or call generic procs that do so -
prior to destruction. This should ideally result in a clear log trail
that allows admins to see what happened. Where this isn't the case, a
stack trace will now be available to help narrow down the cause of
qdeletion so an issue report can be made, and so admins have SOME info
to investigate on.

An example of where this would help catch bugs is #72782 - It was
clearly unintended behaviour to qdel the mob in the first place and this
stack trace would have immediately highlighted exactly where the death
came from.
```
[2023-01-18 12:44:40.415] runtime error: Mob with client has been deleted. (code/modules/mob/mob.dm:29)
 - proc name:  stack trace (/proc/_stack_trace)
 -   source file: stack_trace.dm,4
 -   usr: Julia Watson (/mob/living/carbon/human)
 -   src: null
 -   usr.loc: the floor (111,143,2) (/turf/open/floor/iron)
 -   call stack:
 -  stack trace("Mob with client has been delet...", "code/modules/mob/mob.dm", 29)
 - Julia Watson (/mob/living/carbon/human): Destroy(0)
 - Julia Watson (/mob/living/carbon/human): Destroy(0)
 - Julia Watson (/mob/living/carbon/human): Destroy(0)
 - Julia Watson (/mob/living/carbon/human): Destroy(0)
 - qdel(Julia Watson (/mob/living/carbon/human), 0)
 - the mouse (/mob/living/basic/mouse): try consume cheese(Julia Watson (/mob/living/carbon/human))
 - the mouse (/mob/living/basic/mouse): tamed(the mouse (/mob/living/basic/mouse), Julia Watson (/mob/living/carbon/human))
 - /datum/callback (/datum/callback): Invoke(the mouse (/mob/living/basic/mouse), Julia Watson (/mob/living/carbon/human))
 - /datum/component/tameable (/datum/component/tameable): on tame(the mouse (/mob/living/basic/mouse), Julia Watson (/mob/living/carbon/human))
 - the mouse (/mob/living/basic/mouse):  SendSignal("simplemob_sentiencepotion", /list (/list))
 - the intelligence potion (/obj/item/slimepotion/slime/sentience): attack(the mouse (/mob/living/basic/mouse), Julia Watson (/mob/living/carbon/human), "icon-x=16;icon-y=7;left=1;butt...")
 - the mouse (/mob/living/basic/mouse): attackby(the intelligence potion (/obj/item/slimepotion/slime/sentience), Julia Watson (/mob/living/carbon/human), "icon-x=16;icon-y=7;left=1;butt...")
 - the intelligence potion (/obj/item/slimepotion/slime/sentience): melee attack chain(Julia Watson (/mob/living/carbon/human), the mouse (/mob/living/basic/mouse), "icon-x=16;icon-y=7;left=1;butt...")
 - Julia Watson (/mob/living/carbon/human): ClickOn(the mouse (/mob/living/basic/mouse), "icon-x=16;icon-y=7;left=1;butt...")
 - the mouse (/mob/living/basic/mouse): Click(the floor (112,143,2) (/turf/open/floor/iron), "mapwindow.map", "icon-x=16;icon-y=7;left=1;butt...")
 - 
```
See also #67300.

An example of where this would help identify causes of death where
previously there were none - Scenarios that were fixed by #62949 and
#66104 caused administrative headaches figuring out causes of death when
players in exploding crates just got literally fucking qdeleted. Which
was pretty trivial for players to do to other players.

Issue reports can be made and bugs can be fixed as we go along.

There are examples of quote "false positives" unquote.

Right click -> Delete, which can be used on any atom.
```
[2023-01-18 11:40:54.597] runtime error: Mob without client but with associated ckey has been deleted. (code/modules/mob/mob.dm:32)
 - proc name:  stack trace (/proc/_stack_trace)
 -   source file: stack_trace.dm,4
 -   usr: Norah Rader (/mob/dead/observer)
 -   src: null
 -   usr.loc: the floor (109,143,2) (/turf/open/floor/iron)
 -   call stack:
 -  stack trace("Mob without client but with as...", "code/modules/mob/mob.dm", 32)
 - Fulton Enderly (/mob/dead/observer): Destroy(0)
 - Fulton Enderly (/mob/dead/observer): Destroy(0)
 - qdel(Fulton Enderly (/mob/dead/observer), 0)
 - Timberpoes (/client): admin delete(Fulton Enderly (/mob/dead/observer))
 - Timberpoes (/client): Delete(Fulton Enderly (/mob/dead/observer))
```
 
Admin gibself
```
[2023-01-18 11:41:17.635] runtime error: Mob with client has been deleted. (code/modules/mob/mob.dm:29)
 - proc name:  stack trace (/proc/_stack_trace)
 -   source file: stack_trace.dm,4
 -   usr: Norah Rader (/mob/living/carbon/human)
 -   src: null
 -   usr.loc: the floor (109,145,2) (/turf/open/floor/iron)
 -   call stack:
 -  stack trace("Mob with client has been delet...", "code/modules/mob/mob.dm", 29)
 - Norah Rader (/mob/living/carbon/human): Destroy(0)
 - Norah Rader (/mob/living/carbon/human): Destroy(0)
 - Norah Rader (/mob/living/carbon/human): Destroy(0)
 - Norah Rader (/mob/living/carbon/human): Destroy(0)
 - qdel(Norah Rader (/mob/living/carbon/human), 0)
 - Norah Rader (/mob/living/carbon/human): gib(1, 1, 1, 0)
 - Norah Rader (/mob/living/carbon/human): gib(1, 1, 1, 0)
 - Timberpoes (/client): Gibself()
```

Over time these stack traces can be checked for how well the true cause
of death is logged, and whether there is a better procedure for deleting
cliented/ckeyed mobs than just qdel(target).
## Why It's Good For The Game

qdeletion of cliented or ckeyed mobs is death and often lacks any
specific logging.

This obfuscates bugs and hinders admin investigation.

stack_tracing highlights where this happens, revealing the previously
obfuscated bugs more clearly. It also provides relevant information on a
cause of death that can allow admin investigation to take place despite
the absence of logging.
## Changelog
🆑
admin: When a player-owned mob is deleted from the game world, a stack
trace is now dropped in the runtime logs. This allows admins and coders
to investigate these issues easier.
/🆑
2023-01-29 19:53:40 +08:00
..

Unit Tests

What is unit testing?

Unit tests are automated code to verify that parts of the game work exactly as they should. For example, a test to make sure that the amputation surgery actually amputates the limb. These are ran every time a PR is made, and thus are very helpful for preventing bugs from cropping up in your code that would've otherwise gone unnoticed. For example, would you have thought to check that beach boys would still work the same after editing pizza? If you value your time, probably not.

On their most basic level, when UNIT_TESTS is defined, all subtypes of /datum/unit_test will have their Run proc executed. From here, if Fail is called at any point, then the tests will report as failed.

How do I write one?

  1. Find a relevant file.

All unit test related code is in code/modules/unit_tests. If you are adding a new test for a surgery, for example, then you'd open surgeries.dm. If a relevant file does not exist, simply create one in this folder, then #include it in _unit_tests.dm.

  1. Create the unit test.

To make a new unit test, you simply need to define a /datum/unit_test.

For example, let's suppose that we are creating a test to make sure a proc square correctly raises inputs to the power of two. We'd start with first:

/datum/unit_test/square/Run()

This defines our new unit test, /datum/unit_test/square. Inside this function, we're then going to run through whatever we want to check. Tests provide a few assertion functions to make this easy. For now, we're going to use TEST_ASSERT_EQUAL.

/datum/unit_test/square/Run()
    TEST_ASSERT_EQUAL(square(3), 9, "square(3) did not return 9")
    TEST_ASSERT_EQUAL(square(4), 16, "square(4) did not return 16")

As you can hopefully tell, we're simply checking if the output of square matches the output we are expecting. If the test fails, it'll report the error message given as well as whatever the actual output was.

  1. Run the unit test

Open code/_compile_options.dm and uncomment the following line.

//#define UNIT_TESTS			//If this is uncommented, we do a single run though of the game setup and tear down process with unit tests in between

Then, run tgstation.dmb in Dream Daemon. Don't bother trying to connect, you won't need to. You'll be able to see the outputs of all the tests. You'll get to see which tests failed and for what reason. If they all pass, you're set!

How to think about tests

Unit tests exist to prevent bugs that would happen in a real game. Thus, they should attempt to emulate the game world wherever possible. For example, the quick swap sanity test emulates a real scenario of the bug it fixed occurring by creating a character and giving it real items. The unrecommended alternative would be to create special test-only items. This isn't a hard rule, the reagent method exposure tests create a test-only reagent for example, but do keep it in mind.

Unit tests should also be just that--testing units of code. For example, instead of having one massive test for reagents, there are instead several smaller tests for testing exposure, metabolization, etc.

The unit testing API

You can find more information about all of these from their respective doc comments, but for a brief overview:

/datum/unit_test - The base for all tests to be ran. Subtypes must override Run(). New() and Destroy() can be used for setup and teardown. To fail, use TEST_FAIL(reason).

/datum/unit_test/proc/allocate(type, ...) - Allocates an instance of the provided type with the given arguments. Is automatically destroyed when the test is over. Commonly seen in the form of var/mob/living/carbon/human/human = allocate(/mob/living/carbon/human/consistent).

TEST_FAIL(reason) - Marks a failure at this location, but does not stop the test.

TEST_ASSERT(assertion, reason) - Stops the unit test and fails if the assertion is not met. For example: TEST_ASSERT(powered(), "Machine is not powered").

TEST_ASSERT_NOTNULL(a, message) - Same as TEST_ASSERT, but checks if !isnull(a). For example: TEST_ASSERT_NOTNULL(myatom, "My atom was never set!").

TEST_ASSERT_NULL(a, message) - Same as TEST_ASSERT, but checks if isnull(a). If not, gives a helpful message showing what a was. For example: TEST_ASSERT_NULL(delme, "Delme was never cleaned up!").

TEST_ASSERT_EQUAL(a, b, message) - Same as TEST_ASSERT, but checks if a == b. If not, gives a helpful message showing what both a and b were. For example: TEST_ASSERT_EQUAL(2 + 2, 4, "The universe is falling apart before our eyes!").

TEST_ASSERT_NOTEQUAL(a, b, message) - Same as TEST_ASSERT_EQUAL, but reversed.

TEST_FOCUS(test_path) - Only run the test provided within the parameters. Useful for reducing noise. For example, if we only want to run our example square test, we can add TEST_FOCUS(/datum/unit_test/square). Should never be pushed in a pull request--you will be laughed at.

Final Notes

  • Writing tests before you attempt to fix the bug can actually speed up development a lot! It means you don't have to go in game and folllow the same exact steps manually every time. This process is known as "TDD" (test driven development). Write the test first, make sure it fails, then start work on the fix/feature, and you'll know you're done when your tests pass. If you do try this, do make sure to confirm in a non-testing environment just to double check.
  • Make sure that your tests don't accidentally call RNG functions like prob. Since RNG is seeded during tests, you may not realize you have until someone else makes a PR and the tests fail!
  • Do your best not to change the behavior of non-testing code during tests. While it may sometimes be necessary in the case of situations such as the above, it is still a slippery slope that can lead to the code you're testing being too different from the production environment to be useful.