Added Unit Tests, Increased Coverage, and Fixed Compilation and I/O Issues by ipekoke · Pull Request #69 · DUNE-DAQ/datahandlinglibs

ipekoke · 2025-08-07T13:02:30Z

Added unit tests for ReadoutTypes, SkiplistLatencyBufferModel and IterableQueueModel classes.
Fixed an issue in BufferedFileWriter where excessive I/O operations were causing the run to take too long.
Removed the explicit constructor with size parameter from IterableQueueModel, as it could not be called.
RecorderConcept now inherits from MonitorableObject.
Fixed a compilation error in EmptyFragmentRequestHandlerModel where the previous call to send(std::move(fragment), inherited::m_fragment_send_timeout_ms) did not compile because the send() function expects a std::chrono::milliseconds duration, not a raw integer.
Fixed a comment in IterableQueueModel that said back() gives a pointer to the current write index when it should be the last written element.
Lcov only includes header files that were included and declared in a .cpp file. A .cxx test file was created, with object declarations from the models directory.
Fake classes were added in the testutils directory to be used in unit testing.
Unit test line coverage increased from 10.01% to 36.1%. Function coverage increased from 12.8% to 27.2%.

…n and end functions

…onversion for fragment send timeout create a test app to include files to lcov

…s, increase coverage for data move callback registry

… handler model and fix fake readout type

…dler model

denizergonul

I did the first round of my review to ask for some quick changes. I'll review more thoroughly soon.

denizergonul · 2025-09-16T08:49:29Z

        }
        else {
-	  //TLOG() << "Add element " << element->get_timestamp();      
+	        TLOG() << "Add element " << element->get_timestamp();      


Undo uncommenting (I assume for test purposes)

denizergonul · 2025-09-16T09:01:54Z

+#include "datahandlinglibs/models/DefaultRequestHandlerModel.hpp"
+#include "datahandlinglibs/models/TaskRawDataProcessorModel.hpp"
+#include "datahandlinglibs/models/EmptyFragmentRequestHandlerModel.hpp"
+//#include "daqdataformats/Fragment.hpp"  // for FragmentType


We can remove the commented-out line.

denizergonul · 2025-09-16T09:03:10Z

+namespace datahandlinglibs{
+namespace unittest{
+
+using namespace dunedaq::datahandlinglibs;


Don't think this is necessary since you're already inside the namespace declared above.

denizergonul · 2025-09-16T09:25:30Z

+        }
+        ts +=25;
+        rl.limit();
+        //std::this_thread::sleep_for(std::chrono::milliseconds(5));


Remove comments

denizergonul · 2025-09-16T09:28:53Z

      } else {
        TLOG() << tname << ": Didn't manage to get SKL head and tail!";
      }
+      std::cout << "out of if" << std::endl;


Remove debug logs. Stick to either std::cout or TLOG.

denizergonul · 2025-09-16T10:05:20Z

+//cant change request timeout so cant enter if statement? 
+//needs 32 seconds


Please remove debug logs or make them descriptive enough.

You can also use labels like TODO: or FIXME: where appropriate.

denizergonul · 2025-09-16T10:06:53Z

+/**
+ * num_frames needs to be constexpr.
+ * In get_fragment_pieces there is a case where the result depends on it.
+ * 
+ */ 
+/*
+struct Changed_num_frames_FakeReadoutType : unittest::FakeReadoutType {
+    static constexpr size_t fixed_num_frames = 4;
+    size_t get_num_frames() const override {return fixed_num_frames;}
+    size_t get_payload_size() const override 
+    {
+    return fixed_num_frames * get_frame_size();
+    }
+
+    virtual Changed_num_frames_FakeReadoutType* begin()  override{ return reinterpret_cast<Changed_num_frames_FakeReadoutType*>(data); }
+    virtual Changed_num_frames_FakeReadoutType* end()  override { return begin() + fixed_num_frames; }
+
+    char data[fixed_payload_size];
+    static constexpr size_t fixed_payload_size = fixed_frame_size * fixed_num_frames;
+};
+BOOST_AUTO_TEST_CASE(DefaultRequestHandlerModel_issue_request_kFound_num_frames_more_than_one)
+{
+    auto latency_buffer = std::make_shared<SkipListLatencyBufferModel<Changed_num_frames_FakeReadoutType>>();
+    auto error_registry = std::make_unique<dunedaq::datahandlinglibs::FrameErrorRegistry>();
+    unittest::FakeRequestHandlerType<Changed_num_frames_FakeReadoutType,SkipListLatencyBufferModel<Changed_num_frames_FakeReadoutType>> testhandler(latency_buffer, error_registry);
+
+    testhandler.test_start();
+    for(int i = 1; i<10; i++)
+    {
+        Changed_num_frames_FakeReadoutType elem;
+        elem.set_timestamp(i);
+        TLOG()<<elem.get_num_frames()<< " " << elem.get_timestamp();
+        latency_buffer->write(std::move(elem));
+    }
+
+    BOOST_REQUIRE_EQUAL(testhandler.get_m_num_requests_found(),0);
+
+    testhandler.issue_request(create_request(5,2));
+
+    auto start = std::chrono::steady_clock::now();
+    while (testhandler.get_m_handled_requests() < 1) {
+        std::this_thread::sleep_for(std::chrono::milliseconds(50));
+        if (std::chrono::steady_clock::now() - start > std::chrono::seconds(6)) {
+            BOOST_FAIL("Timeout: handler never processed the request");
+        }
+    }
+    BOOST_REQUIRE_EQUAL(testhandler.get_m_handled_requests(),1);
+    BOOST_REQUIRE(testhandler.get_m_response_time_acc()>0);
+    BOOST_REQUIRE_EQUAL(testhandler.get_m_response_time_acc(),testhandler.get_m_response_time_min());
+    BOOST_REQUIRE_EQUAL(testhandler.get_m_response_time_acc(),testhandler.get_m_response_time_max());
+    BOOST_REQUIRE_EQUAL(testhandler.get_m_num_requests_found(),1);
+}
+*/


Remove commented-out lines if there is no need for them.

denizergonul · 2025-09-16T10:08:23Z

+        BOOST_REQUIRE(queue.write(std::move(i)));
+
+    BOOST_REQUIRE(queue.isFull());
+    BOOST_REQUIRE(!queue.write(10));  // Attempt to write when full


These kind of comments combined with the name of the function are very useful. Please make sure every test has them as much as possible.

denizergonul · 2025-09-16T10:11:27Z

+    processor.add_postprocess_task([&post_func_two_called](const ROType* /*elem*/) { post_func_two_called = true;});
+    processor.make_queues();
+
+    ROType* post_pro_elem = new ROType();


Is it okay that we're not deleting these? If possible, we can use smart pointers instead which are less prone to user errors.

denizergonul · 2025-09-16T10:12:26Z

+    using DefaultRequestHandlerModel<ReadoutType, LatencyBufferType>::DefaultRequestHandlerModel;
+
+
+    int get_m_handled_requests () const {return (this->m_handled_requests).load();}


Please remove the m prefix from function names.

Ipek Oke added 12 commits July 10, 2025 11:27

unit tests for ReadoutTypes

b3a2765

removed first test, changed the others and added a test to check begi…

7c88da6

…n and end functions

added comments

6f084c7

BufferedReadWrite_test fix

4d43317

CMakeLists change for BufferedReadWrite_test

7dbe833

started unit tests for IterableQueueModel

d999652

RecorderConcept inherits from opmonlib::MonitorableObject, fix type c…

2bcd718

…onversion for fragment send timeout create a test app to include files to lcov

create utilities for unit tests, fixes for IterableQueueModel_test

dc3c592

seperate bloated unit tests into smaller ones

94676f4

Merge remote-tracking branch 'origin/develop' into tests/unit_tests

a28af2c

add corner cases and remove redundant checks in unit tests

7cca627

add unit test for get_skiplist function in SkiplistLatencyBufferModel

e2facb3

ipekoke requested review from denizergonul and roland-sipos August 7, 2025 13:02

ipekoke changed the title ~~Tests/unit tests~~ Added Unit Tests, Increased Coverage, and Fixed Compilation and I/O Issues Aug 7, 2025

Ipek Oke added 4 commits August 11, 2025 14:35

add unit tests for TaskRawDataProcessorModel

6ba0c5e

fix skiplist test app cleaner functionality and add multiple producer…

58d3d0a

…s, increase coverage for data move callback registry

Merge remote-tracking branch 'origin/develop' into tests/unit_tests

94fc411

add unit tests for DefaultRequestHandlerModel

a30d06d

ShyamB97 added the maintenance Addresses a user request or a change in another part of the system label Aug 27, 2025

Ipek Oke added 3 commits September 15, 2025 10:43

add unit tests for data handling model, fix tests for default request…

f0feb07

… handler model and fix fake readout type

Merge remote-tracking branch 'origin/develop' into tests/unit_tests

e4040d3

update fake latency buffer

94738cf

denizergonul mentioned this pull request Sep 16, 2025

Ambiguous constructor in IterableQueueModel? #26

Open

denizergonul linked an issue Sep 16, 2025 that may be closed by this pull request

Ambiguous constructor in IterableQueueModel? #26

Open

change some function names used in unit tests for default request han…

5c783ed

…dler model

denizergonul requested changes Sep 16, 2025

View reviewed changes

Ipek Oke added 2 commits September 16, 2025 12:58

combined two fake test raw data processors

e164311

carry fake data handling model to unittestutilities and change its name

9161c71

ShyamB97 assigned denizergonul Apr 2, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Unit Tests, Increased Coverage, and Fixed Compilation and I/O Issues#69

Added Unit Tests, Increased Coverage, and Fixed Compilation and I/O Issues#69
ipekoke wants to merge 22 commits into
developfrom
tests/unit_tests

ipekoke commented Aug 7, 2025

Uh oh!

denizergonul left a comment

Uh oh!

denizergonul Sep 16, 2025

Uh oh!

denizergonul Sep 16, 2025

Uh oh!

denizergonul Sep 16, 2025

Uh oh!

denizergonul Sep 16, 2025

Uh oh!

denizergonul Sep 16, 2025

Uh oh!

denizergonul Sep 16, 2025

Uh oh!

denizergonul Sep 16, 2025

Uh oh!

denizergonul Sep 16, 2025

Uh oh!

denizergonul Sep 16, 2025

Uh oh!

denizergonul Sep 16, 2025

Uh oh!

denizergonul Sep 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		//cant change request timeout so cant enter if statement?
		//needs 32 seconds

		using DefaultRequestHandlerModel<ReadoutType, LatencyBufferType>::DefaultRequestHandlerModel;


		int get_m_handled_requests () const {return (this->m_handled_requests).load();}

Conversation

ipekoke commented Aug 7, 2025

Uh oh!

denizergonul left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants