Conversation
d71797f to
a45fd64
Compare
a45fd64 to
2a2e108
Compare
There was a problem hiding this comment.
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); | ||
| }; |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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