Unity: Bug: invalid filename given when assert fails in included subfile

Hi. I agree the term helper isn’t important here. I’m using it because it’s the term used in the Unity documentation and examples to encapsulate the first usecase you listed. Having a shared vocabulary is helpful for keeping conversations efficient. This is just a turn of phrase commonly used in the testing world.

( A quick aside: The second usecase you mentioned is only related to C++, and this is a C testing framework, not a C++ testing framework. It CAN be used to test C++, but if you’re primarily using C++, GoogleTest or some other C++ framework is likely a better fit for your needs ).

I fear I may not have sufficiently explained why Unity is designed this way, since you’re still using labels like “broken” for this design choice. The behavior your discussing is a choice and it’s a choice that matches much of Unity’s design: to encourage people to write proper code. Unity (and the related tools) are admittedly “opinionated software”. I’m going to do my best to explain why it has this particular opinion. In the end, you still may not agree with it, and that’s completely fine. My hope is that you will at least understand it’s not broken.

C obviously isn’t like higher level languages. It doesn’t have any means of reflection, so the only way it can report something is if it’s explicitly given that information. You’re running into this headache now.

Embedded Systems (and honestly most C applications) are designed for memory and speed efficiency. When this is not the case, most devs migrate to other languages.

So Unity is balancing these two needs, as it’s purpose is to be the best embedded software platform testing tool it can be. It’d be awesome to report full stack traces or the like, but instead we’re focused on reporting the best details we can in a limited footprint.

Unity has settled on a the compromise of reporting one file, one line, one test name, one diff string, and one custom message.

So the question you bring up in your question above is “what file should be reported?”

Centralizing groups of assertions together to be reused from test to test is a GREAT idea. We highly promote doing just that. It’s incredibly useful and keeps your tests running efficiently. There are two things that you are doing that are against the way it’s meant to run:

1. Tests should only be in test files.

Let’s take your example above. If the test report tells you test_something_more_universal_from_the_other_file in functions.h line 5 failed, you don’t know if it failed when it was running Test.cpp or any other module. Which one caused the issue? It’s unknown. We haven’t helped the developer learn anything other than it’s broken “somewhere”.

1. Reporting should be from the test file’s perspective.

Again, we’re definitely pro-reuse, even when it’s related to test code. If I have multiple tests that all have the same needs, I should absolutely centralize those assertions into a single collection. When one of them fails, I want it to report the most useful information possible. The best way to do this, is to report what line IN THE TEST actually failed. If there are extra details, that’s what the custom message is for.

Maybe an example will help. Let’s say I want to write a function that verifies it can stuff another number into a ring buffer and that the ring buffer isn’t overflowing yet. Something like this:

//functions.h

void verify_we_can_add_number(int num)
{
    int retval = RingBuffer_AddNumber(num);
    TEST_ASSERT_EQUAL_INT(0, retval);
    TEST_ASSERT_EQUAL_FALSE( RingBuffer_IsFull() );
}

Then I use this function in my tests:

void test_RingBuffer_SHOULD_AcceptFourIntsWithoutFailing(void)
{
    verify_we_can_add_number(34);  //line 7
    verify_we_can_add_number(2);   //line 8
    verify_we_can_add_number(222);   //line 9
    verify_we_can_add_number(18);   //line 10
}

void test_RingBuffer_SHOULD_AcceptAndPullNumbers(void)
{
    verify_we_can_add_number(34);    //line 15

   // Other stuff happens... 
}

So I’ve written the example above with the same style you proposed. If we “fixed” Unity to report functions.h, and the test failed, where did it fail? Is it line 7? line 9? Maybe line 15? Maybe it wasn’t even Test.cpp at all, but another test file? I’ve still got the function name, test_RingBuffer_SHOULD_AcceptFourIntsWithoutFailing being reported… so at least I can rule out some of the other files after a bit of brainwork… but I still don’t know where in the test it failed. As you probably know, tests can get a lot more complicated than the example above.

There’s something else subtle going on here, which is that I’ve lost the ability to jump to the failing test. Many Unity users are using Unity from within an IDE and it’s super-useful to click on the failing test and jump to it… but if we’ve redirected the filename to be the helper file instead, I can no longer jump to the actual test that is failing.

This is a good moment to point out that when the functions in your functions.h file (or whatever helper) are well designed, you should never need to open them to look. Let’s say it looked more like this:

//functions.h (updated)

void verify_we_can_add_number(int num, int line)
{
    int retval = RingBuffer_AddNumber(num);
    UNITY_TEST_ASSERT_EQUAL_INT_MESSAGE(0, retval, line, "Ring Buffer Return Value Non-Zero");
    UNITY_TEST_ASSERT_EQUAL_FALSE_MESSAGE( RingBuffer_IsFull(), line, "Ring Buffer Reports Full" );
}
#define VERIFY_WE_CAN_ADD_NUMBER(int num) verify_we_can_add_number(num, __LINE__)

With just a few minor tweaks, we’ve made it so the developer gets all the information they need. When they run the failing test above, they get a message reporting that it’s Test.cpp that failed in test_RingBuffer_SHOULD_AcceptFourIntsWithoutFailing on line 10 and that Ring Buffer Reports Full. They click the error in their IDE, it brings them to line 10 and they see that when they attempted to add the 4th item, the ring buffer started to report that it was full.

You’re correct that it requires a little more work to write a good helper function. Writing a function intended for reuse is always more work. It should be noted, though, that it’s not much work (it’s really just following the same pattern each time) and that (most importantly) the work is at the point you’re writing the function, instead of when it mysteriously fails days, weeks, or even years later… or for another developer completely… It’s a little bit of work upfront, with the intention of saving a lot of work for your future self.

Well, if you’ve made it this far, I hope you can see why things are the way they are. I’d love it if you even agreed in the end, but I understand that these are topics about tradeoffs, and not everyone is going to see them the same way. In either case, thanks for talking it out. I appreciate anyone who is willing to help shape open source tools to be the best they can be.

😃 Mark

If you don’t mind leaving it open for now. At a minimum, I’d like to improve the documentation. Better, I’d like to find an efficient way to handle situations like this. I’d rather not have that solution involve the need to pass an additional argument to every call… but I’m confident that’s not the only possible solution.

I am coming from world with 2kb RAM (ok we have 4kb nowadays on Cortex-s) and ~20kb of flash, so lets put your request in real world perspective (when not running on the PC):

I noticed that UNITY_BEGIN_ registers the file name -> great, we know the test file.

Its called once per compilation unit, so filename takes lets say 20 bytes.

I noticed that TEST_ASSERT_* registers the line -> great. I proposed that TEST_ASSERT_* could also register the file name -> so that we know ALSO in which file it failed.

Its called for each test at least once, so lets say you have 10 test cases, hence 200 bytes - that is 10x more, just to print a filename?

I’m not proposing to ditch registering the file in UNITY_BEGIN -> I’m saying to do both.

So you would add 220 bytes, since you want to do both, so 11x bigger footprint than what we have - and that is for a quite simple test suite. Imagine how bad this scales: the more asserts you add, each time you lose 20 bytes for the filename. And we assert various register values, and memory location values because in the end you want to know what is written to which port and what is read from it. Keep in mind that flashing (starting simulator or real chip) takes the most time (running the test cases is faster), so each binary we want to flash can now be smaller, hence we increase the number of times we need to flash the binary, hence your 10 times bigger footprint amounts to also much bigger runtime. Forget about your object orientation - nowadays you will be hunting for compiler optimizations. Every function call is a couple of bytes (needs to push to stack, pop from stack) in code size, so your layering and OO will soon lose an appeal.

At best this could be a configurable feature, but I am quite sure that if it is disabled by default, someone who just starts like you will not know about it, while others will treat it with caution due to bigger memory footprint.