ThrowTheSwitch / Ceedling

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

File sourced using `TEST_SOURCE_FILE` is build, even if the preproccer should remove it #928

Closed pileghoff closed 2 months ago

pileghoff commented 2 months ago

A minimal example of the issue:

#if 0
TEST_SOURCE_FILE("does_not_compile.c")
#endif

void test_a(void)
{
    TEST_ASSERT_EQUAL_INT(1, 1);
}

This will fails with the error:

đź‘ź Collecting Test Context
--------------------------
Parsing test_minimal.c for build directive macros...
🧨 EXCEPTION: CeedlingException ==> File 'does_not_compile.c' specified with TEST_SOURCE_FILE() in test_minimal cannot be found in the source file collection
Backtrace ==>
/var/lib/gems/3.0.0/gems/ceedling-1.0.0/lib/ceedling/test_invoker_helper.rb:73:in `block in validate_build_directive_source_files'
/var/lib/gems/3.0.0/gems/ceedling-1.0.0/lib/ceedling/test_invoker_helper.rb:54:in `each'
/var/lib/gems/3.0.0/gems/ceedling-1.0.0/lib/ceedling/test_invoker_helper.rb:54:in `validate_build_directive_source_files'
/var/lib/gems/3.0.0/gems/ceedling-1.0.0/lib/ceedling/test_invoker.rb:110:in `block (2 levels) in setup_and_invoke'
/var/lib/gems/3.0.0/gems/ceedling-1.0.0/lib/ceedling/test_invoker.rb:109:in `each'
/var/lib/gems/3.0.0/gems/ceedling-1.0.0/lib/ceedling/test_invoker.rb:109:in `block in setup_and_invoke'
/var/lib/gems/3.0.0/gems/ceedling-1.0.0/lib/ceedling/build_batchinator.rb:26:in `build_step'
/var/lib/gems/3.0.0/gems/ceedling-1.0.0/lib/ceedling/test_invoker.rb:85:in `setup_and_invoke'
/var/lib/gems/3.0.0/gems/ceedling-1.0.0/lib/ceedling/rules_tests.rake:48:in `block (2 levels) in <top (required)>'
/usr/share/rubygems-integration/all/gems/rake-13.0.6/lib/rake/task.rb:281:in `block in execute'
/usr/share/rubygems-integration/all/gems/rake-13.0.6/lib/rake/task.rb:281:in `each'
/usr/share/rubygems-integration/all/gems/rake-13.0.6/lib/rake/task.rb:281:in `execute'
/usr/share/rubygems-integration/all/gems/rake-13.0.6/lib/rake/task.rb:219:in `block in invoke_with_call_chain'
/usr/share/rubygems-integration/all/gems/rake-13.0.6/lib/rake/task.rb:199:in `synchronize'
/usr/share/rubygems-integration/all/gems/rake-13.0.6/lib/rake/task.rb:199:in `invoke_with_call_chain'
/usr/share/rubygems-integration/all/gems/rake-13.0.6/lib/rake/task.rb:188:in `invoke'
/usr/share/rubygems-integration/all/gems/rake-13.0.6/lib/rake/application.rb:160:in `invoke_task'
/usr/share/rubygems-integration/all/gems/rake-13.0.6/lib/rake/application.rb:116:in `block (2 levels) in top_level'
/usr/share/rubygems-integration/all/gems/rake-13.0.6/lib/rake/application.rb:116:in `each'
/usr/share/rubygems-integration/all/gems/rake-13.0.6/lib/rake/application.rb:116:in `block in top_level'
/usr/share/rubygems-integration/all/gems/rake-13.0.6/lib/rake/application.rb:125:in `run_with_threads'
/usr/share/rubygems-integration/all/gems/rake-13.0.6/lib/rake/application.rb:110:in `top_level'
/var/lib/gems/3.0.0/gems/ceedling-1.0.0/bin/cli_helper.rb:228:in `run_rake_tasks'
/var/lib/gems/3.0.0/gems/ceedling-1.0.0/bin/cli_handler.rb:224:in `build'
/var/lib/gems/3.0.0/gems/ceedling-1.0.0/bin/cli.rb:316:in `build'
/var/lib/gems/3.0.0/gems/thor-1.3.2/lib/thor/command.rb:28:in `run'
/var/lib/gems/3.0.0/gems/thor-1.3.2/lib/thor/invocation.rb:127:in `invoke_command'
/var/lib/gems/3.0.0/gems/thor-1.3.2/lib/thor.rb:538:in `dispatch'
/var/lib/gems/3.0.0/gems/ceedling-1.0.0/bin/cli.rb:93:in `start'
/var/lib/gems/3.0.0/gems/ceedling-1.0.0/bin/ceedling:125:in `<top (required)>'
/usr/local/bin/ceedling:25:in `load'
/usr/local/bin/ceedling:25:in `<main>'
🌱 Ceedling could not complete operations because of error

Due to the fact that TEST_SOURCE_FILE("does_not_compile.c") is not included after preprocessing, i dont expect ceedling to actually build the file. This seemed to work on 0.31, but is broken in 1.0.0-533d636

We use this in our setup with different build flags (and obviously not just #if 0).

pileghoff commented 2 months ago

Our big issue is actually the following case, just in case it helps with a more realistic use-case:

#ifdef FEATURE_A
#include "feature_a_dependency.h"
TEST_SOURCE_FILE("feature_a.c")

void test_a(void)
{
    TEST_ASSERT_EQUAL_INT(1, feature_a());
}
#endif

This fails to link if FEATURE_A is not set, since feature_a.c is built, but it relies on functions in feature_a_dependency.[ch], which is not built.

mkarlesky commented 2 months ago

Hi, @pileghoff. This is an interesting edge case. We'll need to add some documentation to address it. There's a variety of preprocessing steps Ceedling performs — optional and mandatory. TEST_SOURCE_FILE() is only a “marker”, if you will (provided by Unity, actually). It's a do-nothing macro that Ceedling can scan for but ultimately is processed away to nothing. In order for that marker to do its job, it must be scanned for first before any other preprocessing steps. This is a simple string-based scan. The actual compiler toolchain C preprocessor will later completely wipe it away.

Previous versions of Ceedling had a (unreliable) means of preserving a certain class of macros like TEST_SOURCE_FILE() (at the time it mainly targeted Unity's test range macros). We had to pull that out in the big revamp for 1.0.0. It's on the list to figure out how to restore that functionality in a more resilient way. When we do restore it—assuming we find a good way to do it—it will handle the case you've reported here.

Would it work to split your test files by feature? With Ceedling 1.0.0's features for building each test file as a mini project, you could provide each test executable build the #define symbols it needs (i.e. FEATURE_A for only some test executables) with the TEST_SOURCE_FILE() statement in the open inside those with no conditional compilation guards around it. If you had a lot of overlap of test cases and wanted to avoid duplication, I believe you could #include a .c file in the feature-specific test files containing the test cases in common and enable Ceedling’s preprocessing for your test files. You'd need to name the file that contains test cases in common something other than the convention Ceedling recognizes for test files meant to become test executables.

pileghoff commented 2 months ago

Its not really feasible for us to split out the different features into different tests, but we dont use TEST_SOURCE_FILE that often, so i would much rather find a workaround for those few cases. The only time where we need it, is for 3rd party code where the header and source filenames dont match, and in those cases we could just make some test specific header files with matching names (or include the c files as you mention.)

Just wanted to report this as it seemed like a regression in the rc (for which we are very exited btw! good job!).

mkarlesky commented 2 months ago

Thank you so much for the report. Come to think of it (checks docs) we did document this unsupported feature interaction in the Changelog — that is admittedly now a lot to digest.

Ceedling is not yet capable of preserving build directive macros through preprocessing of test files. If, for example, you wrap these macros within conditional compilation preprocessing statements, they will not work as you expect.

I just clarified that statement a bit and improved the relevant discussion in BreakingChanges that did not adequately address this. CeedlingPacket does not address this at all but should. I've made a note to update that doc too.

Of course, the best solution is to simply cause Ceedling to do the right thing in these scenarios. We hope to get there soon!