-
Notifications
You must be signed in to change notification settings - Fork 62
Fix #1042: Add explicit config setup to authorization unit tests #1101
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
base: main
Are you sure you want to change the base?
Fix #1042: Add explicit config setup to authorization unit tests #1101
Conversation
93b66d1 to
6047d80
Compare
mlitre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for picking up this issue! I think there are several other tests where we could be more explicit, such as: validate_token_local_auth_list_enabled_accepted, validate_token_local_auth_list_enabled_unknown_no_remote_authorization, validate_token_local_auth_list_enabled_unknown_websocket_disconnected, validate_token_local_auth_list_enabled_connectivity_manager_connected_accepted, validate_token_emaid_offline_no_certificate_contract_validation_offline_allowed_certificate_valid_no_authorize_offline, validate_token_emaid_offline_no_certificate_contract_validation_offline_allowed_certificate_valid_localauthlistctrlr_enabled, validate_token_emaid_offline_no_certificate_contract_validation_offline_allowed_certificate_expired and validate_token_auth_cache_lifetime_expired.
Once those tests are also updated, I think everything should be good. Please make sure to run clang-format after your modifications, I saw some format changes that could cause issues in the CI.
6330f09 to
fd19e41
Compare
|
Hi @mlitre |
|
Hi @stephenredhat looks good to me, if you could just fix the tests by adding the missing device model argument in some of the calls to set the variables. Thanks! |
|
Hi @mlitre I’ve updated all set_local_pre_authorize and set_local_authorize_offline calls to explicitly include device_model, as suggested. The changes are now committed and pushed. Let me know if there’s anything else you'd like to see. Thanks again for your help! |
|
Hi @stephenredhat, thanks for taking the time to contribute! The changes look good to me, but could you please fix the issues seen in the CI? I believe there is format error, a dco error (please sign off your commits as per the contributing guidelines) and there seems to be a test that fails, if you could rectify that as well thank you! |
13751e4 to
f6f9ce0
Compare
- Added explicit config setup for initial authorization tests - Added explicit config setup for remaining authorization unit tests Signed-off-by: Stephen Oresanya <soresany@redhat.com>
f6f9ce0 to
8def1d1
Compare
…fline calls to match new signature Signed-off-by: Stephen Oresanya <soresany@redhat.com>
…tion tests Ensures calls match function definitions by including the device_model argument. Also reformatted with clang-format for CI compliance. Signed-off-by: Stephen Oresanya <soresany@redhat.com>
|
Hi @stephenredhat thanks for updating everything, things look almost perfect now! Could you just take a look at the failing test case and see if there is a fix required in the code or if the test case is wrong? |
Describe your changes
This PR resolves inconsistent test behavior in test_authorization.cpp by explicitly setting key configuration values in all authorize_req-related unit tests. Specifically, each test now directly configures:
These changes ensure that test results are deterministic and isolated from global or default state, as described in issue #1042.
Issue ticket number and link
Fixes #1042
#1042
Checklist before requesting a review