Skip to content
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

Project configuration :defines: can be confusing, when using :mixins: with additional defines #984

Open
mikewzr opened this issue Jan 10, 2025 · 2 comments

Comments

@mikewzr
Copy link

mikewzr commented Jan 10, 2025

When upgrading to Ceedling 1.0.0, i recognized some strange behavior in the Ceedling :defines: config section. There are two ways to specify definies that apply to all the test files:

:defines:
  :test:
    - UNIT_TEST
    - CONFIG_IDF_TARGET_LINUX

and 2)

:defines:
  :test:
    :*:
      - UNIT_TEST
      - CONFIG_IDF_TARGET_LINUX

I additionally have a mixin, just specifying some additional variables for single tests:

:defines:
  :test:
    :test_DBC_single_lookup_table:
      - UNIT_TEST_DBC_SINGLE_LT
    :test_DBC_idx_val_lookup_table:
      - UNIT_TEST_DBC_IDX_VAL_LT
    :test_DBC_idx_format_lookup_table:
      - UNIT_TEST_DBC_IDX_FORMAT_LT

The problem is, when using variant 1), the specific defines of the mixins do not apply. Ceedling does ignore them without any warning. Everything works as expected, when using variant 2).
I think the best thing would be, to remove variant 1) completely. If that breaks to much existing configs, at least a warning would necessary to warn the users about this behavior.

@mkarlesky
Copy link
Member

A very good point, @mikewzr. That is a validation combo that has not been considered. We'll add that.

I'll grant you that it is a bit confusing. We wrestled on this one. There's always a balance between simplicity and power user scenarios as well as factoring in backwards compatibility / familiarity. The simple list convention has been available for a long time. With all the other breaking changes, etc., etc. we decided to keep it, document it well, and add lots of validation around it. The scenario you're describing is simply one that slipped through the validation cracks. Thank you for highlighting it.

@mkarlesky
Copy link
Member

@mikewzr A brach handling this issue has just been pushed to the repository. It needs more testing and documentation revisions before it's added to the next prerelease (probably 1.1.0 as this is more new features than a bug fix, per se).

Here's what now happens:

  1. If :defines or :flags config or mixin conventions mismatch (array and hash), the array is promoted to the hash-style * matcher automatically with a notice logging this modification.
  2. We added an option to the use of deep_merge() to handle the case of merging config and mixin values that could be single strings for one and an array of strings for the other. This single primitive string or string array convention is available here and there throughout configuration handling.
  3. If some oddball edge case slips through both of the above, Ceedling now logs a warning explaining which mixin value will clobber a config value in a merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants