Skip to content

Conversation

@Kellesi
Copy link
Collaborator

@Kellesi Kellesi commented Aug 14, 2025

@Kellesi Kellesi requested a review from Copilot August 14, 2025 18:08
Copy link
Contributor

Copilot AI left a 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 unit tests for the kf::Event class, covering both NotificationEvent and SynchronizationEvent types with various scenarios including timeouts, multiple waiters, and state management operations.

  • Adds extensive test coverage for Event functionality including initialization, setting/clearing states, waiting with/without timeouts
  • Implements multi-threaded tests to verify proper synchronization behavior between NotificationEvent and SynchronizationEvent types
  • Integrates the new test file into the CMake build system

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
test/EventTest.cpp New comprehensive test file covering Event class functionality with single and multi-threaded scenarios
test/CMakeLists.txt Adds EventTest.cpp to the build configuration

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Member

@SergiusTheBest SergiusTheBest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests with another thread have a fatal flaw: a thread is not started right away, it takes an arbitrary time to spin up a thread. I suggest just to call wait() in the main thread. It should be able to cover necessary cases (and be simpler).

@Kellesi
Copy link
Collaborator Author

Kellesi commented Aug 19, 2025

@SergiusTheBest but if I just call wait() it in the main thread, it will hang waiting forever

@SergiusTheBest
Copy link
Member

@SergiusTheBest but if I just call wait() it in the main thread, it will hang waiting forever

@Kellesi No, if you set 0 timeout.

@Kellesi
Copy link
Collaborator Author

Kellesi commented Aug 20, 2025

@SergiusTheBest timeout 0 won't actually wait, it just checks the current state and returns immediately. That makes it impossible to test cases where a thread should really be blocked on an Event. That means we cannot test scenarios where threads are supposed to wait and later be released - for example, verifying that only one thread wakes up for a SynchronizationEvent, or that all waiters are released for a NotificationEvent.
I only added threads in tests where they were really needed. If you see any cases that could be simplified, please point them out, because in my opinion, threads are required here.

@SergiusTheBest
Copy link
Member

should really be blocked on an Event

@Kellesi - We don't test it even now having multiple threads as we signal an event before a thread begins waiting. I suggest to trust the OS in terms of thread scheduling and focus on the event state and transitions. And for that wait(() with 0 timeout is sufficient. It can check every test case we have now.

@SergiusTheBest
Copy link
Member

@Kellesi An example (I didn't update step description):

   GIVEN("NotificationEvent with multiple waiters and NotificationEvent for complition")
   {
       kf::Event triggerEvent(NotificationEvent, false);

       WHEN("A wait is performed with no timeout and the event is not set")
       {
           THEN("Both threads are waiting")
           {
               REQUIRE(STATUS_TIMEOUT == triggerEvent.wait(&kZeroTimeout));
               REQUIRE(STATUS_TIMEOUT == triggerEvent.wait(&kZeroTimeout));
           }

           triggerEvent.set();

           THEN("After trigger event is set, both threads are released from waiting")
           {
               REQUIRE(STATUS_SUCCESS == triggerEvent.wait(&kZeroTimeout));
               REQUIRE(STATUS_SUCCESS == triggerEvent.wait(&kZeroTimeout));
           }
       }
   }

@Kellesi
Copy link
Collaborator Author

Kellesi commented Aug 20, 2025

Removed threads from the last two tests, but kept it for the test case that checks waiting with timeout = nullptr.

@SergiusTheBest SergiusTheBest merged commit fab2e52 into main Dec 22, 2025
2 checks passed
@SergiusTheBest SergiusTheBest deleted the KF-30-test-Event branch December 22, 2025 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants