Bug: invalid filename given when assert fails in included subfile

[MacDada]

/* Test.cpp */
#include <unity.h>
#include "functions.h"

void test_something_specific_to_this_test_file_ok() {
    TEST_ASSERT_TRUE(true);
}

void test_something_specific_to_this_test_file_fail() {
    TEST_ASSERT_TRUE(false); // line 10
}

int main() {
    UNITY_BEGIN();

    RUN_TEST(test_something_specific_to_this_test_file_ok); // line 16
    RUN_TEST(test_something_specific_to_this_test_file_fail); // line 17

    RUN_TEST(test_something_more_universal_from_the_other_file); // line 19

    return UNITY_END();
}

/* functions.h */
#include <unity.h>

void test_something_more_universal_from_the_other_file() {
    TEST_ASSERT_TRUE(false); // line 5
}

The result I get:

Building...
Testing...
test/native/test_including/Test.cpp:16: test_something_specific_to_this_test_file_ok	[PASSED]
test/native/test_including/Test.cpp:10: test_something_specific_to_this_test_file_fail: Expected TRUE Was FALSE	[FAILED]
test/native/test_including/Test.cpp:5: test_something_more_universal_from_the_other_file: Expected TRUE Was FALSE	[FAILED]

Program received signal SIGINT (Interrupt: 2)
----------- native:native/test_including [ERRORED] Took 2.93 seconds -----------

For the last test, the line given 5 is OK, but the filename is wrong: Test.cpp – in reality, the assert that failed is in the functions.h file.

And that makes debugging confusing and hard.

[Letme]

Why would you have test source in the header file? If you want to use helper functions, then rather define that and put implementation into source file and just use declaration in header file functions.h

[MacDada]

Why would you have test source in the header file? If you want to use helper functions, then rather define that and put implementation into source file and just use declaration in header file functions.h

sorry, I'm not sure what do you mean.

you want me to split functions.h into functions.h and functions.cpp? I don't think that would change the issue of naming the wrong file in the test results…

[Letme]

I am quite sure it would. You see inline functions (or what you do with include) are pasted into the file on that exact line. This can result in all different clashes, not to mention include problems. Also it is kinda good practice not to have sources in header files, but use source file for that. Try it, and let us know.

[MacDada]

I am quite sure it would.

Nope:

[MacDada]

AFAIU, the problem is that the file name is registered inside UNITY_BEGIN macro. The TEST_ASSERT_*macros register only the line.

So, in the end, we have a mishmash of a right line and wrong file.

I guess the solutions could be to continue registering file in UNITY_BEGIN (to name the test file that we're evaluating), but ALSO register the file in TEST_ASSERT_* macros, so that we can properly show the name and line of the file that called it.

[mvandervoord]

@MacDada your understanding is correct. The filename is set in UNITY_BEGIN. This is intentionally hard to override to make people pause to think about what they're doing.

There are two good reasons to have assertions in a file: because it's a test file, or because it's helping a test file. In your case, you're putting assertions in a non-test file. What's the purpose of functions.h? Is it a series of helpers? If so, you should go "all in" on treating it like a helper. You should be passing the line number into your functions with macros so that the reported file + line are the line in the TEST file which went wrong. Your helper file is there to check specific things, but the developer needs to know where their test went wrong.

Here is an example of it done correctly: https://github.com/ThrowTheSwitch/Ceedling/tree/master/examples/temp_sensor/test/support

If functions.h isn't a test and it isn't a helper... it probably shouldn't have test assertions in it. If you disagree, I'd be interested in hearing about your usecase. :)

That's my two cents.

[MacDada]

What's the purpose of functions.h? Is it a series of helpers?

functions.h is just an example to make a point that Unity is reporting wrong file paths.

USECASE1:

• I have my own TEST_ASSERT_INSTANCE_OF function that I want to reuse in multiple tests.
• I guess I might have more assertions that might be missing in Unity (not everything must be in Unity ofc)

USECASE2:

• I have a BaseClass with some functions virtual, and some others implemented.
• I want to create multiple implementations that will inherit from BaseClass.
• I don't want to repeat myself when writing tests checking that everything is working as expected.
• Therefore I want to create a BaseClassTest that would be reused in multiple InheritingClassTest(s).
• It doesn't have to be a test class, I'm happy to reuse functions/macros/whatever.

To sum up: you can call it "helper", OK, whatev. The point is: reusability.

You should be passing the line number into your functions with macros so that the reported file + line are the line in the TEST file which went wrong.

Nah. I'd rather fix Unity to properly show where the problem is. Why make life harder?

Here is an example of it done correctly: https://github.com/ThrowTheSwitch/Ceedling/tree/master/examples/temp_sensor/test/support

OK, I'm new to C(++), I have no clue what is #if 0 supposed to mean o.O It won't work?

Anyway, from the rest of it, I'm guessing, for each reusable test part you want me to create:

• a function to be reused (OK)
• a macro to call the function (make life harder)
• manually handle passing the line (make life harder)
• maybe a front function+macro to call multiple of those above (make life harder)

OK, if no other way, I may do that. I'm suggesting to fix Unity instead of making life harder from the Developer / UnityConsumer POV.

[mvandervoord]

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".

2. 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

[MacDada]

Hi.

@mvandervoord First things first, THANKS your answer(s). My background is in web development, and as a hobby I started with home automation stuff, DIY, microcontrollers, etc. -> and unfortunately I'm mostly disappointed by the community:

"low quality" code everywhere, bad examples, documentation that explain only basic stuff or explaining nothing, or even worse: bringing more confusion; abandoned repositories with hundreds of issues that nobody responds to, finally I ask for help and after 3 days of full-time fighting and hair pulling I close the threads by finding the answer myself – all of that is happening to me for the last few months.

You bring me hope!

Having a shared vocabulary is helpful for keeping conversations efficient.

Yes, ofc. But it can also be eye-closing (no abstraction can be better than wrong abstraction). I didn't want to go into helper voc, because I feel it may narrow me down to the current status quo – which I guess would not satisfy my needs.

( 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 ).

YES. That's true. I'm here because my journey is Arduino -> NodeMcu -> C++ -> PlatformIO => Unity is the default testing framework for PIO. I do have a "todo" list and "try other testing frameworks" is on it.

But since I'm not on deadlines, I make this my opportunity to learn new stuff and try everything. So I want to explore Unity to the point I know its strengths and weaknesses, before I move on into the different directions.

Quickly I noticed simple-ness of Unity. I find it very neat, smart, interesting. But yeah, it is not tailored to OOP/C++ thinking.

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.

"Proper" is indeed "opinionated". I say it is a bug, you say it is a feature ;-) I get that ;-)

I would argue then (as I'm "opinionated" as well) that it is better to report nothing than report a wrong thing. If you make a point that assertions should not be outside of the "test file", then it should be loud and clear. And with strong examples how to achieve stuff in a different way efficiently.

So, to solidify the current state of things, when assertion fails in a different file than where UNITY_BEGIN is -> make that clear, don't confuse the user with wrong information. Would be better to skip the line number. Even better to tell "the truth" to what file the line numer belongs to ;-)

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.

Yeah, coming from "laughed at" PHP -> "HOW TO VAR_DUMP IN C++" :D C++ is a frustration to me in each and every step, it's like going back to being a cavemen. I will not be bashing C – that's a completely different issue.

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

Not entirely. It is smart in some ways. When the test succeeds, it shows the file and line where RUN_TEST is. When test fails it shows the file and line where assertions is. So it is reporting according to the context. I will come back to that.

[…the tradeoff what to report: the place it actually failed or the test function where it failed…]

WhyNotBothMeme.jpg

This is where my proposed solution lays.

• I noticed that UNITY_BEGIN_ registers the file name -> great, we know the test file.
• 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.
• I'm not proposing to ditch registering the file in UNITY_BEGIN -> I'm saying to do both.

This way, the files are different -> we can show a bigger picture. I know we don't have reflection to show as rich info as I get in PHPUnit. Let's do what we can.

Building...
Testing...
test/native/test_including/Test.cpp:16: test_something_specific_to_this_test_file_ok	[PASSED]
test/native/test_including/Test.cpp:10: test_something_specific_to_this_test_file_fail: Expected TRUE Was FALSE	[FAILED]
test/native/test_including/Test.cpp: test_something_more_universal_from_the_other_file: [FAILED]
    - in test/native/test_including/functions.h:5: Expected TRUE Was FALSE	

This is an example of expected output – notice for "proper" code (defined by the current status quo) it works the same. But, if you do include other files that do assertions, it is doing the job of reporting where exactly it failed while maintaining the current job of reporting in which test it did. No more lies ;-)

And it can be achieved by collecting the file, apart from the line, in which the assertion failed.

I know, what if we are 5 levels deep in the includes, how to report that? Not possible. We have no reflection. But we do what we can.

As for the "IDE clickability": with the current implementation, for included files, it doesn't work. It sends me to a wrong place. I propose to fix it.

AND, if you're still opinionated that including files with assertions is wrong: just don't do it ;-) This fix does not change anything for people who don't include other files, but it does fix the issue for people who do.

---

Open source, I know, somebody has to do that. I can try to hack around for a proof of concept. I'm still a noob, though. Thanks again! :)

[Letme]

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.

[MacDada]

I am coming from world with 2kb RAM (ok we have 4kb nowadays on Cortex-s) and ~20kb of flash

@Letme Okey dokey. Not an issue on ESP8266/ESP32.

So… if Unity must be efficient on 2 KB of RAM, then every byte counts -> therefore I guess this issue can be closed as "won't fix" for this very reason.

I would suggest to make it clear in the docs, though.

[mvandervoord]

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.

[MacDada]

BTW, one more thing I noticed – also connected to C++ (namespaces), so maybe not necessary to fix, but if the fix would be easy then why not ;) When a function is namespaced, the RUN_TEST macro does not work…

[cdwijs]

I'm also coming from very small microcontrollers. I agree with the notion that remembering filenames costs a lot of memory. but unity doesn't have to remember the filenames for long. It only needs to remember the filename until the RUN_TEST is finished.

Therefore I propose TEST_ASSERT_ to register the file name, and the line number, overwriting the previous filename and line number. If an assertion fails, it can print the filename, and the line number. Afterwards, unity doesn't need to remember them anymore, as they already are printed, and the test is finished.

This way unity uses just as much (or as little) memory as it does now (one filename and one line number), but it reports the actual place (file and linenumber) where the assert didn't succeed.

Would this be a good solution, or do I miss something here?

[robsonos]

The solution in #474 (comment) works like a charm!