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

ctests: stop including C files in the test files #432

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

daniloegea
Copy link
Collaborator

@daniloegea daniloegea commented Jan 5, 2024

With these changes, a copy of libnetplan without static functions will be build. All the test files will be linked against this separate library. With that, there is no need to include the C files in the test files anymore.

To make this possible, all the static keywords in function declarations were replaced with a macro that will be defined at compilation time.

Description

My goal is to make #425 easier to be implemented. Also, refactoring the C code is less likely to break the build of test files...

Checklist

  • Runs make check successfully.
  • Retains 100% code coverage (make check-coverage).
  • New/changed keys in YAML format are documented.
  • (Optional) Adds example YAML for new feature.
  • (Optional) Closes an open bug in Launchpad.

@daniloegea daniloegea requested a review from slyon January 5, 2024 17:56
@slyon
Copy link
Collaborator

slyon commented Jan 8, 2024

This approach sounds reasonable to me. Especially with the default STATIC -> static fallback in place, as long as UNITTESTS is not defined.

CC @fnordahl WDYT? Would that work for you?

With these changes, a copy of libnetplan without static functions will be
build. All the test files will be linked against this separate
library. With that, there is no need to include the C files in the test
files anymore.

To make this possible, all the static keywords in function declarations
were replaced with a macro that will be defined at compilation time.
@daniloegea
Copy link
Collaborator Author

Rebased on top of the last batch of commits.

@daniloegea daniloegea marked this pull request as ready for review January 16, 2024 16:42
Copy link
Collaborator

@slyon slyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice refactoring that makes testing easier. It doesn't affect the production code.

LGTM, let's get this merged!

@daniloegea daniloegea merged commit 517d0f8 into canonical:main Jan 16, 2024
13 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants