-
Notifications
You must be signed in to change notification settings - Fork 2
KF-30 add tests for ConditionVariable #71
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
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.
Pull Request Overview
This PR adds comprehensive test coverage for the ConditionVariable class, implementing various test scenarios to validate its behavior with different timeout settings, thread interactions, and notification patterns. The tests cover both predicate-based and non-predicate waiting, single and multiple thread scenarios, and edge cases.
Key changes:
- Added 292 lines of test code covering all major ConditionVariable functionality
- Enhanced ConditionVariable implementation to properly handle exclusive lock checks
- Updated CMakeLists.txt to include the new test file
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| test/ConditionVariableTest.cpp | Comprehensive test suite for ConditionVariable with scenarios for timeouts, predicates, and multi-threading |
| test/CMakeLists.txt | Added ConditionVariableTest.cpp to the build configuration |
| include/kf/ConditionVariable.h | Fixed waitFor implementation to check exclusive lock state and handle return values properly |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| external.release(); | ||
| } | ||
| auto status = m_semaphore.wait(timeout); | ||
| auto result = (NT_SUCCESS(status) && status != STATUS_TIMEOUT) ? Status::Success : Status::Timeout; |
Copilot
AI
Aug 18, 2025
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.
The condition logic is redundant. If NT_SUCCESS(status) is true, then status cannot be STATUS_TIMEOUT since STATUS_TIMEOUT is not a success code. Consider simplifying to: auto result = NT_SUCCESS(status) ? Status::Success : Status::Timeout;
| auto result = (NT_SUCCESS(status) && status != STATUS_TIMEOUT) ? Status::Success : Status::Timeout; | |
| auto result = NT_SUCCESS(status) ? Status::Success : Status::Timeout; |
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.
STATUS_TIMEOUT is evaluated as success by NT_SUCCESS(status), but here it should be Status::Timeout
a3db2bb to
864cc75
Compare
Task: https://jira.dev.local/jira/browse/KF-30