-
Notifications
You must be signed in to change notification settings - Fork 2
KF-33 add tests for ThreadPool #74
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
d71797f to
a45fd64
Compare
a45fd64 to
2a2e108
Compare
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 kf::ThreadPool class to address ticket KF-33. The tests verify core ThreadPool functionality including thread creation, execution, cleanup, and move semantics.
- Adds test scenarios for normal operation, boundary conditions, and move semantics
- Includes precompiled header updates with C++ runtime helper functions for kernel-mode testing
- Fixes uninitialized array issue in ThreadPool header
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/ThreadPoolTest.cpp | New comprehensive test suite covering ThreadPool functionality with various scenarios |
| test/pch.h | Adds C++ runtime helper functions needed for kernel-mode testing environment |
| test/CMakeLists.txt | Includes the new ThreadPoolTest.cpp in the build configuration |
| include/kf/ThreadPool.h | Initializes m_threads array to prevent undefined behavior |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| KeDelayExecutionThread(KernelMode, FALSE, &interval); | ||
| auto p = static_cast<LONG*>(context); | ||
| InterlockedIncrement(p); | ||
| }; |
Copilot
AI
Aug 20, 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 magic number -10'000 should be documented or replaced with a named constant. This appears to be a 1ms delay in 100ns units, but this is not immediately clear.
| }; | |
| // 1 millisecond delay in 100ns units (negative for relative time) | |
| constexpr LONGLONG ONE_MILLISECOND_DELAY_100NS_UNITS = -10'000; | |
| constexpr auto fn = [](void* context) { | |
| LARGE_INTEGER interval; | |
| interval.QuadPart = ONE_MILLISECOND_DELAY_100NS_UNITS; | |
| KeDelayExecutionThread(KernelMode, FALSE, &interval); | |
| auto p = static_cast<LONG*>(context); | |
| InterlockedIncrement(p); | |
| }; |
|
|
||
| THEN("ThreadPool started up to kMaxCount threads") | ||
| { | ||
| REQUIRE(value == 64); |
Copilot
AI
Aug 20, 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 magic number 64 should be replaced with a reference to kMaxCount to make the test more maintainable. If kMaxCount changes, this test would fail unexpectedly.
| REQUIRE(value == 64); | |
| REQUIRE(value == kf::ThreadPool::kMaxCount); |
Task: https://jira.dev.local/jira/browse/KF-33