-
Notifications
You must be signed in to change notification settings - Fork 981
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug: invalid filename given when assert fails in included subfile #645
Comments
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 |
sorry, I'm not sure what do you mean. you want me to split |
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. |
AFAIU, the problem is that the file name is registered inside 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 |
@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 Here is an example of it done correctly: https://github.com/ThrowTheSwitch/Ceedling/tree/master/examples/temp_sensor/test/support If That's my two cents. |
USECASE1:
USECASE2:
To sum up: you can call it "helper", OK, whatev. The point is: reusability.
Nah. I'd rather fix Unity to properly show where the problem is. Why make life harder?
OK, I'm new to C(++), I have no clue what is Anyway, from the rest of it, I'm guessing, for each reusable test part you want me to create:
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. |
+ showing proper files/lines in the test output on failed tests - polluting global namespace ThrowTheSwitch/Unity#645 (comment)
Hi. I agree the term ( 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:
Let's take your example above. If the test report tells you
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:
Then I use this function in my tests:
So I've written the example above with the same style you proposed. If we "fixed" Unity to report 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
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 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 |
@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!
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.
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.
"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
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.
Not entirely. It is smart in some ways. When the test succeeds, it shows the file and line where
WhyNotBothMeme.jpg This is where my proposed solution lays.
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.
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! :) |
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):
Its called once per compilation unit, so filename takes lets say 20 bytes.
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?
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. |
@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. |
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'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? |
The solution in #474 (comment) works like a charm! |
The result I get:
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 thefunctions.h
file.And that makes debugging confusing and hard.
The text was updated successfully, but these errors were encountered: