-
Notifications
You must be signed in to change notification settings - Fork 749
Enable -Werror for unit tests and fix resulting compiler warnings #5668
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?
Conversation
|
Looks like there's also a failure on mac builds. I think you probably just need to add an if-def S2N_CLONE_SUPPORTED around the s2n_unit_test_clone_child_process function to get those builds to pass. |
tests/unit/s2n_safety_test.c
Outdated
| } | ||
|
|
||
| #if S2N_CLONE_SUPPORTED | ||
| static int s2n_unit_test_clone_child_process(struct s2n_unit_test_clone *data) |
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.
This is not the correct place for this change. The fork_generation_number_test is the testfile that is causing the issue.
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 pointing that out — I’ll move the guard to fork_generation_number_test and update the PR accordingly.
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.
@maddeleine I’ve moved the S2N_CLONE_SUPPORTED guard and s2n_unit_test_clone_child_process into s2n_fork_generation_number_test, and reverted s2n_safety_test to only include the initialization fix. Please let me know if this looks correct now.
Goal
Enable
-Werrorfor all unit test builds and fix the warnings that appear when compiling tests with this flag enabled.Why
Unit tests previously did not compile with
-Werror, which allowed several warnings to accumulate unnoticed.Applying warnings-as-errors improves build hygiene, enforces consistency with the main library build, and prevents future regressions.
How
Build System
CMakeLists.txtto apply-Werrorto all unit test targets whenUNSAFE_TREAT_WARNINGS_AS_ERRORS=ONis set.Warning Fixes
tests/unit/s2n_safety_test.c
tests/unit/s2n_tls12_handshake_test.c
memcmp().tests/unit/s2n_tls13_handshake_state_machine_test.c
memcmp()checks.Verification
Callouts
UNSAFE_TREAT_WARNINGS_AS_ERRORSmechanism to keep the change opt-in, as requested in the issue discussion.Testing
Built and tested with:
Verified:
Related
Resolves #5650
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.