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

test: replace deprecated libcheck macros #5654

Closed
wants to merge 2 commits into from

Conversation

sumit-bose
Copy link
Contributor

The fail* macros are deprecated by libcheck some time ago. Recently a fix
for a different issue in those macros cause a 'too many arguments for
format' compiler warning which won't be fixed on the libckeck side since
the macros are deprecated.

This patch replaces the deprecated macros with the new ones:

  • fail -> ck_abort_msg
  • fail_unless -> ck_assert_msg
  • fail_if -> sss_ck_fail_if_msg

The fail_if macro does not have a corresponding new version and I added a
local replacement sss_ck_fail_if_msg which is based on ck_assert_msg.

The common_check.h header file adds libcheck related macros which are
not needed by cmocka test, using common.h is sufficient here.
The fail* macros are deprecated by libcheck some time ago. Recently a
fix for a different issue in those macros cause a 'too many arguments
for format' compiler warning which won't be fixed on the libckeck side
since the macros are deprecated.

This patch replaces the deprecated macros with the new ones:

 - fail -> ck_abort_msg
 - fail_unless -> ck_assert_msg
 - fail_if -> sss_ck_fail_if_msg

The fail_if macro does not have a corresponding new version and I added
a local replacement sss_ck_fail_if_msg which is based on ck_assert_msg.
@alexey-tikhonov
Copy link
Member

Thank you for the patches. I agree in general.

I only have a questions about sss_ck_fail_if_msg:

  • _ck_assert_failed that you used for implementation doesn't seem to belong to "official"/public libcheck API
  • sss_ck_fail_if_msg defines absolutely the same error message as fail_unless (while condition is negated)
  • do we need sss_ck_fail_if_msg at all? why couldn't we use fail_unless with negated expr?

@alexey-tikhonov alexey-tikhonov self-assigned this Jun 4, 2021
@sumit-bose
Copy link
Contributor Author

Thank you for the patches. I agree in general.

I only have a questions about sss_ck_fail_if_msg:

* `_ck_assert_failed` that you used for implementation doesn't seem to belong to "official"/public libcheck API

* `sss_ck_fail_if_msg` defines absolutely the same error message as `fail_unless` (while condition is negated)

* do we need `sss_ck_fail_if_msg` at all? why couldn't we use `fail_unless` with negated expr?

Hi,

yes, I see sss_ck_fail_if_msg as a temporary solution and that's why I used _ck_assert_failed because I didn't found a good and easy to use replacement with the public API.

All usage of sss_ck_fail_if_msg should be replaced by ck_assert_msg with the expression negated. I didn't want to do this with a simple replace expr->!(expr), like the other changes in this patch, because I guess the resulting condition might be hard to understand if expr already contains a non-trivial condition. So for a proper change each of the conditions should be handled manually for with I didn't had the time yet, I just wanted to get rid of the compiler warnings which might overshadow more important warnings.

Feel free to defer that patch until someone has the time to check the conditions.

bye,
Sumit

@mzidek-gh
Copy link
Contributor

Hi,

I think it would be better to merge this patch while it is still applicable to avoid rebases in the future and open a ticket to remove sss_ck_fail_if_msg later. Abandoning the deprecated API and changing the conditions in one PR would require more brain energy and IMO it is better to do so in two steps :) So unless someone started to convert the conditions already or there is a different plan for this patch I would give +1 for this patch as is (+ opening the ticket I mentioned). The patch is already validated with green tests.

Just my 2c

@alexey-tikhonov
Copy link
Member

Ok.

@pbrezina pbrezina added the Ready to push Ready to push label Jul 20, 2021
@pbrezina
Copy link
Member

Pushed PR: #5654

  • master
    • 7fdff74 - test: replace deprecated libcheck macros
    • cdc75c5 - tests: do not use libcheck include file in cmocka tests

@pbrezina pbrezina added Pushed and removed Accepted Ready to push Ready to push labels Jul 20, 2021
@pbrezina pbrezina closed this Jul 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants