Skip to content

Conversation

@raffael0
Copy link
Member

@raffael0 raffael0 commented May 23, 2025

This PR adds testing support for the LLServer and tests of the Sequencemanager. It also fixes the "start bug" which caused a lot of problems. The "END" field in sequences is now executed as well.

To do this I had to change quite a lot of the code base to even make it somewhat testable. Most notable is that I had to turn a lot of classes into Singletons which I don't really like but at least it works now

Right now I am only testing the start bug and interpolation. More suggestions are welcome :)

Todo:

  • Add an automatic test runner with github actions
  • Add more Tests

Tests to add:

  • 3 value interpolation
  • Actions not in Timestamp order. NOTE This does not work rn but I'm postponing the fix for later as it's rather complex and not really urgent
  • Sub Timestamps where total subtimestamp time is larger than next parent time step
  • What happens when first timestamp is before start/last after end
  • Negative timestamp
  • What happens when valve position is below for example zero.
  • Do non-utf8 characters crash the sequencemanager?
  • Test what happens when you have a timestamp in the END field which is after the end of the sequence

I'm looking forward for some feedback on my testing abstractions and how I do my tests before I commit to writing a lot of them.

@raffael0 raffael0 changed the base branch from main to feature/local-development May 23, 2025 18:41
@raffael0 raffael0 force-pushed the feature/testing-gtest branch from fa70e83 to 49a0fb8 Compare May 23, 2025 19:05
@raffael0 raffael0 requested a review from Copilot May 23, 2025 19:25

This comment was marked as outdated.

@raffael0 raffael0 force-pushed the feature/testing-gtest branch 4 times, most recently from 3e56ee6 to cc7920e Compare May 24, 2025 20:45
@raffael0 raffael0 changed the base branch from feature/local-development to main May 24, 2025 20:45
@raffael0 raffael0 force-pushed the feature/testing-gtest branch 2 times, most recently from e3021ff to 29e46ae Compare May 24, 2025 21:04
@raffael0 raffael0 force-pushed the feature/testing-gtest branch from 077a73a to d1d1637 Compare June 23, 2025 17:50
@miDeb
Copy link
Member

miDeb commented Jun 23, 2025

I manually merged the main branch into this branch because the automatic merge caused a conflict (the same variables ended up declared twice)

@raffael0 raffael0 force-pushed the feature/testing-gtest branch from a518518 to 351046e Compare June 27, 2025 22:04
@raffael0 raffael0 force-pushed the feature/testing-gtest branch from 351046e to 8181087 Compare June 27, 2025 22:12
@miDeb miDeb mentioned this pull request Jun 28, 2025
@raffael0 raffael0 requested a review from Copilot July 1, 2025 21:56
Copy link

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

Adds testing support and refactors file I/O to enable unit tests for the LLServer’s SequenceManager, fixes the “start” bug, and ensures the “END” field is executed.

  • Introduce JSON test data and GoogleTest cases for SequenceManager behavior
  • Replace direct filesystem calls with a FileSystemAbstraction singleton for mocking
  • Update SequenceManager to use the new abstraction, add interpolation helpers, and correct timing

Reviewed Changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/data/SequenceManagerTestData.h New JSON fixtures for start/execution, interpolation, swaps, UTF8
tests/SequenceManagerTest.cpp Unit tests for start-once, interpolation, abort, UTF8 handling
tests/CMakeLists.txt Setup of GoogleTest and test target
src/utility/FileSystemAbstraction.cpp New concrete implementation of filesystem abstraction
src/utility/LoopTimer.cpp Added time field but missing update in wait()
src/utility/JSONMapping.cpp Switched file I/O to FileSystemAbstraction
src/utility/Config.cpp Switched config load to FileSystemAbstraction
src/SequenceManager.cpp Integration of FileSystemAbstraction, interpolation refactor
include/utility/LoopTimer.hpp Made getters const
include/utility/Singleton.h Added SetInstance() for DI
include/SequenceManager.h Added interpolation API and new member for filesystem
include/LLInterface.h Made methods (and destructor) virtual for mocking
include/EventManager.h Made methods (and destructor) virtual for mocking
CMakeLists.txt Split into a library and executable; enable testing
.github/workflows/gtest.yml CI workflow for building and running GoogleTests
Comments suppressed due to low confidence (4)

include/SequenceManager.h:70

  • The doc comment is missing the word “time” after “end”. E.g. “The first element is the end time, and the second element is …”.
		 *                           The first element is the end ,

include/LLInterface.h:50

  • Declared a virtual destructor but no definition is provided. Add LLInterface::~LLInterface() = default; in a .cpp to avoid linker errors.
		virtual ~LLInterface();

include/EventManager.h:75

  • Declared a virtual destructor but no definition is provided. Implement EventManager::~EventManager() = default; or provide an empty definition in the .cpp.
    virtual ~EventManager();

tests/SequenceManagerTest.cpp:8

  • [nitpick] This include of <utility> isn’t used in the test. You can remove it to keep dependencies minimal.
#include <utility>

@raffael0 raffael0 requested a review from miDeb July 2, 2025 12:01
@raffael0
Copy link
Member Author

raffael0 commented Jul 2, 2025

@miDeb
Can you review the testing abstractions/stuff i moved to singletons?
I think it would be best to merge it as is and then continue in another PR,
since you are also basing your PR on it

@raffael0 raffael0 marked this pull request as ready for review July 2, 2025 21:09
raffael0 and others added 3 commits July 2, 2025 23:40
Added release preset which should be used
Added Options to Cmake for disabling subfeatures
@raffael0 raffael0 merged commit e11f04d into main Jul 3, 2025
1 check passed
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