ThrowTheSwitch / Ceedling

Ruby-based unit testing and build system for C projects
http://throwtheswitch.org
Other
597 stars 246 forks source link

Not getting coverage when I #include a .c file #946

Closed Joel-Skarper closed 2 weeks ago

Joel-Skarper commented 1 month ago

Hi,

I have exactly the same problem as #926. I want to test the internals of a .c file (I have a state machine and between the poisons of #including a .c and accessing the static elements or doing it "the right way" and setting the state of my tests via the public API (i.e. complex tests) I pick the first).

So my test file

#include "spard.h"

#include "spard.c"
TEST_SOURCE_FILE("spard.c") // I have no idea how to use this :S

//My tests
...

Everything compiles and works, but I still get

-----------------------------
GCOV: ✅ OVERALL TEST SUMMARY
-----------------------------
TESTED:  17
PASSED:  17
FAILED:   0
IGNORED:  0

---------------------------
GCOV: CODE COVERAGE SUMMARY
---------------------------

test_spard
----------
⚠️ WARNING: Found no coverage results for test_spard::spard.c

Running Gcovr Coverage Reports
------------------------------
Generating HTML coverage report in 'build/artifacts/gcov/gcovr'...
Done in 0.492 seconds.

Ceedling operations completed in 4.77 seconds

I have two installations of Ceedling. On Windows, I have Ceedling:: 0.31.1, but I just installed the daily available today (Ceedling => 1.0.0-9fc30fb) in WSL. The results I pasted are from the WSL.

The issue I linked suggests, on v0.31, to add a fake "spard.h" in support, so Ceedling pulls "spard.c", but because I already #included "spard.c", all hell breaks loose in the linking step, as I have two definitions of everything.

Is there a way to get coverage of an #included .c? I'm happy if it's only possible in v1.0, I hope to get rid of 0.31 soon.

Joel-Skarper commented 2 weeks ago

I've been trying to work on this but I'm starting to think that what I want is not achievable without modification of the code-under-test.

In #926, the user wanted to test some .c files that had no corresponding .h file. This prevented the .c files from being compiled (as they were included in the test file) and because of that, there was no coverage data. It was suggested that he could create some empty .h files in the support folder, and let Ceedling grab them. This in turn, would pull the .c files, which would be then compiled and code coverage would be generated. Happy days.

My case is slightly different. I want to test the private part of my code. I know it can be tested via the public interface, but testing a state machine (for example) can get really annoying, when you can just simply modify the variables to get the desired starting state.

  1. My first attempt was to include the .c file in my test. As mentioned, this prevents the .c file from being compiled (Ceedling doens't need it, all the code needed to test foo.c is in test_foo.c. Alternatively, as mentioned in #926, I could include foo.h, which would trigger Ceedling to pull foo.c, but this will cause linker errors, as now variables, typedefs, etc would be defined twice.
  2. My second attempt was to get all the private types and static global variables into their own header (foo_privateTypesAndGlobals.h) that I included in both foo.c and test_foo.c. This compiled, but caused a now obvious side effect, in that all the variables exported that way were now generated for each compilation unit, so myGlobalArray existed for both test_foo.c and foo.c, but they were effectively different variables, and tests failed. Plus, this doesn't give me access to static functions.
  3. The only thing I can think of, right now, is the old #ifdef TEST #define STATIC static #else #define STATIC.

And now I just can't think of anything else. It would be great if at least I could get a "yeah, that's not possible, go with 3".

Letme commented 2 weeks ago

Honestly, from first post I did not know what you want, and I still have no idea why option 3 is not suitable? You have a strange test file with that include of .c.

Btw, you can put a extern of static global variables in you header file guarded by ifdef TEST which I assume is what you actually wanted within option 2.

I would never include another source file, because then what is the point of header files and interfaces. It just hides all the global variables and interfaces behind that include. You have a linker, you have link time optimizations, so if you are worried that code is not.going to be optimal enough then use that.

Joel-Skarper commented 2 weeks ago

Honestly, from first post I did not know what you want, and I still have no idea why option 3 is not suitable?

Sorry for not being clear, sometimes you are so deep into a problem that you think that everyone understands what you mean 😅. About why is not 3 a valid option... well, is not that is not valid... I just prefer other options (see below)

I would never include another source file, because then what is the point of header files and interfaces

If that's your opinion, I think that's great, and if you don't like that approach, is perfectly fine. In my case, I think including the ".c" file is (in my totally subjective opinion) a quite elegant way to access the private side of the code without having to change the code for testability.

I would never include another source file, because then what is the point of header files and interfaces. It just hides all the global variables and interfaces behind that include.

I understand the logic behind hiding some of the code, and I understand that I am bending the rules by accessing it tests. I could 100% test the module with just the public interface, but my tests would be longer and more difficult to parse for any colleagues reviewing (or me trying to fix them when I change something in six months). Sure, I could change the public interface to make testing easier, but then again, you would be adding stuff to the code that is only needed for testing... it makes me as uneasy as including a .c file makes you 😆. I think it's a "pick your own poison" situation, and while I choose the include the .c route, you prefer the other.

Btw, you can put a extern of static global variables in you header file guarded by ifdef TEST which I assume is what you actually wanted within option 2.

You mean something like this?

#ifdef TEST
    extern uint32_t myHiddenVar;
#else
    static uint32_t myHiddenVar = 0;
#end

I guess it would work for variables, but it wouldn't work for static functions, right?

I will go with the STATIC redefine, I just was curious to know if what I wanted to do was achievable as it led me through a rabbit hole, and there was a comment in the issue I link by mkarlesky saying that "something in v1.0 made it possible" but I couldn't get it to work, hence my ticket.

Letme commented 2 weeks ago

What I thought for private variables (you could also spin out the part in ifdef TEST to func_private.h) and funcs:

func.h

#ifdef TEST
extern uint32_t myHiddenVar;
uint32_t myHiddenfunc(void);
#endif

func.c

STATIC uint32_t myHiddenVar = 0; /* But this you will anyway need to
                                  * reset in SetUp to ensure you do
                                  * not pollute your tests */

STATIC uint32_t myHiddenFunc(void)
{
 /* bla */
}

Your route will hit the problem if you include that c file in 2 or 3 tests, which is usually what happens when number of tests grow. So you could have TestStateMachine, TestPublicApi, TestEdges, TestBugs ... Then you really want to avoid using compiler to produce the binary, but you want to use linker.

Joel-Skarper commented 2 weeks ago

Ok, I see what you mean!

I guess I could also add the extern statements to the test file instead of to func.c, right? I'll do some exploring tomorrow. Thanks for the ideas!

Letme commented 2 weeks ago

You could add extern statements to test file, but that will lead to a mess (and you might forget stuff between various test files). Point is that you want private and public API in the 'compilation unit/component' files, not spread around in multiple test files. Your idea means that every time I include func.h (or mock), I need to copy/paste bunch of declarations in the TestFunc.c to get access to those private variables and functions, while in my proposal the included header file has all the information inside. You could also move it to func_private.h to make a more distinct differentiation.

And you want declaration in header files (.h) and definition in source files (.c), because header files (declarations) are portable, while source files can lead to problems if there are multiple inclusions as you had in option 1 (hence header files have include guards).

I am glad I can provide some help. I understand my opinion is not aligned with yours, but it is also a reason nobody replied to your question: simply because your approach is something that has been an actively discouraged practice in the C world. And when something is advised against like religion, it takes some more context to explain why it is so.

Joel-Skarper commented 2 weeks ago

Programmers love their dogmas, certainly. I have something that I think, with what I know now, works. I don't think that including a c file on a test file is unheard of (I certainly didn't invent the technique, and I can find a bunch of opinionated programmers in the web that suggest it, although they might be as wrong as I am).

I took your advice, in any case, and I have something that seems to do what I want to do, so thanks for that.

As a TL;DR for a poor soul in the same situation in the future, just move to the static word redefinition trick if you really need to test static parts of your code AND need code coverage reports.

mvandervoord commented 2 weeks ago

Hi. Sorry to pop onto this thread late. It sounds like you've found a work-around for now, but that root issue (that Coverage results aren't generated for C files included using the TEST_SOURCE_FILE macro) still exists? If that's true, I'd like to keep this issue open so we remember to fix it.

Joel-Skarper commented 2 weeks ago

Hi Mark,

I tried to use the TEST_SOURCE_FILE macro but I could not find any documentation about it and I'm not sure if what I did was correct. Just to be sure, the idea is that you could #include a .c file in your test, and then use the macro, as in

// in test_foo.c

#include "foo.h" 
#include <unity.h>

#include "foo.c" // to access the static bits
TEST_SOURCE_FILE("foo.c")

[...]

And with that, coverage info for foo.c would be generated?

This is the minimal code I'm working with. If I try what I described above, with today's ceedling (v1.0.0-ec4171a), I get an exception

👟 Building Objects
-------------------
Compiling test_foo.c...
🧨 EXCEPTION: 'Default Test Compiler' (gcc) terminated with exit code [1] and output >>
test/test_foo.c:5:18: error: expected declaration specifiers or ‘...’ before string constant
    5 | TEST_SOURCE_FILE("foo.c")
      |                  ^~~~~~~
In file included from /usr/include/setjmp.h:30,
                 from build/vendor/unity/src/unity_internals.h:16,
                 from build/vendor/unity/src/unity.h:22,
                 from test/test_foo.c:7:
/usr/include/x86_64-linux-gnu/bits/types/struct___jmp_buf_tag.h:32:5: error: unknown type name ‘__jmp_buf’
   32 |     __jmp_buf __jmpbuf;         /* Calling environment.  */
      |     ^~~~~~~~~
🌱 Ceedling could not complete operations because of errors

joel.zip

mvandervoord commented 2 weeks ago

Ah, I missed the triple-inclusion thing in your earlier posts. TEST_SOURCE_FILE is an alternative to #include-ing the C file directly... for situations where there is no header. It includes the file in the list of files to build, but doesn't work around the static functions, etc. It doesn't help your situation at all.

The error you're getting is unrelated. Unlike the version you typed above, the test file in your sample.zip has the inclusion of unity.h last. It needs to happen BEFORE you use the TEST_SOURCE_FILE macro, because the macro is defined in unity.h

In any case, since you are able to include the C file directly, using TEST_SOURCE_FILE shouldn't make a difference when calculating coverage. I'll have to dig into why coverage isn't being calculated for you.

mvandervoord commented 2 weeks ago

Well, this includes a bit of a catch-22. You're directly including the C file in order to get access to the contents... but by doing that, the C file is automatically being called part of your test (just as any header would). So when you go to compile, the C file doesn't get instrumented for tracking (because it's "part of the test").

I'm not sure there is a simpler way around this than to do the STATIC trick and include the header normally.

mkarlesky commented 2 weeks ago

I am guessing the list of C files built up with TEST_SOURCE_FILE() that are then compiled and linked with the test are not ending up in the list that coverage compilation uses. I haven't looked, but I have a hunch that's the gap here.

I'd say using the macro is the most correct approach; it's probably just not fully hooked up behind the scenes. Should be an easy fix if so.

mkarlesky commented 2 weeks ago

@Joel-Skarper @mvandervoord I just tried an experiment with the latest prerelease build of 1.0.0 using TEST_SOURCE_FILE() to cause an unrelated .c file to be compiled and linked into a test executable that was then run with coverage using the gcov: variant of the corresponding test: task. Coverage worked fine. The additional source file was compiled with coverage (per Ceedling's progress output) and the resulting GCov reporting correctly listed the additional file. In the simple version of my experiment, no calls were made to the functions in the additional .c file. GCov showed 0% coverage for the additional file. I then tried calling a function in the supplemental source file, and GCov showed the expected coverage results.

Digesting this whole thread, then:

  1. I would recommend the tried and true STATIC trick. It's the simplest and most robust way to make "private" functions testable.
  2. Using TEST_SOURCE_FILE() does cause the right thing to happen with GCov coverage builds. However, given (1), using this feature in your test is not appropriate.
  3. Here is the documentation for TEST_SOURCE_FILE() for 1.0.0.
Joel-Skarper commented 2 weeks ago

Thanks all for your responses, I really appreciate you took the time to help me with this one, even when it proved to be a bit unorthodox. I'll take your advice and move to the STATIC trick in my testing, and I learned a thing or two about the building process.

Cheers!