Test double buffer accessor ns by nshehz · Pull Request #551 · ChimeraTK/DeviceAccess

nshehz · 2026-03-10T16:14:35Z

No description provided.

killenb

You put a lot of effort in identifying the test cases that need to be covered. However, basically all tests have issues:

They are not testing along the public API and will break when refactoring
They are often testing implementation details instead of wanted behaviour. Some do not have any logical test coverage at all.
Many tests are fully covered by the UnifiedBackendTest, and most can be implemented to be checked there. There are only two test cases which do not work out of the box
- The test with two accessors locking each other in different threads
- The test that the double buffering is turned off while reading (is missing a callback in the dummy. We should extend the dummy here. I was just missing this in a test this week, but could work around it because my accessors are writeable. There is a write callback). But it's also not cleanly testable here without the callback.

We should move all tests that can be covered by the UnifiedBackendTest to a testDoubleBufferAccessorUnified, and only have the ones that cannot be handles there in this dedicated test.

One test case is missing: Two different registers which are using the same double buffering handshare (e.g. FD and the according macro pulse number). This will also be a dedicated test at the moment, because the unified backend test cannot handle multiple registers.

killenb · 2026-03-13T08:19:56Z

+  accessor.doPreRead(TransferType::read);
+  accessor.doReadTransferSynchronously();
+  accessor.doPostRead(TransferType::read, true);


Why do you call these individually? It should just be accessor.read().

killenb · 2026-03-13T08:20:21Z

+  accessor.doPreRead(TransferType::read);
+  accessor.doReadTransferSynchronously();
+  accessor.doPostRead(TransferType::read, true);


Why do you call these individually? It should just be accessor.read().

killenb · 2026-03-13T08:22:33Z

+  /* expose protected members */
+  template<typename T>
+  class TestableDoubleBufferAccessor : public DoubleBufferAccessor<T> {
+   public:
+    using DoubleBufferAccessor<T>::DoubleBufferAccessor;
+    using DoubleBufferAccessor<T>::buffer_2D;
+  };


Don't. You should test observable behaviour, not implementation details. Internal interfaces might be subject to refactoring, which breaks the test.

killenb · 2026-03-13T08:24:41Z

+
+  auto dbInfo = registerInfo.doubleBuffer.value();
+
+  TestableDoubleBufferAccessor<int> accessor(


Just get the accessor through the regular device interface. Use it as normal code would.

BTW. This call is wrong. It needs the mutex that is shared by the backend. You can't just give "any" mutex.

killenb · 2026-03-13T08:26:29Z

+
+BOOST_AUTO_TEST_CASE(test_current_buffer_selection) {
+  Device device;
+  device.open("(dummy?map=simpleJsonFile.jmap)");


We should get our own jmap file. The "simple" now contains everything, which is very crowded and difficult to understand. This is an additional hurdle when trying to debug stuff. A stripped down map file makes this easier.

killenb · 2026-03-13T08:56:59Z

+BOOST_AUTO_TEST_CASE(test_exception_does_not_leave_lock) {
+  Device device;
+  device.open("(dummy?map=simpleJsonFile.jmap)");
+
+  auto backend = boost::dynamic_pointer_cast<NumericAddressedBackend>(device.getBackend());
+  BOOST_REQUIRE(backend);
+
+  auto mutex = std::make_shared<detail::CountedRecursiveMutex>();
+
+  auto registerInfo = backend->getRegisterInfo("DAQ.FD");
+  auto dbInfo = registerInfo.doubleBuffer.value();
+
+  TestableDoubleBufferAccessor<int> accessor(
+      dbInfo, backend, mutex, RegisterPath("/DAQ/FD"), 16384, 0, AccessModeFlags{});
+
+  // Acquire lock via pre-read
+  accessor.doPreRead(TransferType::read);
+  BOOST_CHECK_EQUAL(mutex->useCount(), 1); // lock acquired
+
+  // Simulate exception during transfer
+  bool exceptionThrown = false;
+  try {
+    throw std::runtime_error("simulated transfer exception");
+  }
+  catch(const std::runtime_error&) {
+    exceptionThrown = true;
+    // mandate: still call postRead to release lock
+    accessor.doPostRead(TransferType::read, false);
+  }
+
+  BOOST_CHECK(exceptionThrown);
+  BOOST_CHECK_EQUAL(mutex->useCount(), 0); // lock released
+}
+


This test does not test anything. The accessor does not know about the exception at all.
You are just calling doPreaRead() and doPostRead() and do "something" in between.

The correct test would be: Use the exception backend to raise an exception during transfer. Then check that the next transfer in another thread can be executed (no deadlock). This is observable behaviour from the outside, that's all that counts.

But don't implement it here. It is all covered by the UnifiedBackendTest.

killenb · 2026-03-13T09:45:21Z

+// Test that hasNewData=false does not swap buffer_2D
+BOOST_AUTO_TEST_CASE(test_has_new_data_false) {
+  Device device;
+  device.open("(dummy?map=simpleJsonFile.jmap)");
+  auto backend = boost::dynamic_pointer_cast<NumericAddressedBackend>(device.getBackend());
+  auto mutex = std::make_shared<detail::CountedRecursiveMutex>();
+  auto registerInfo = backend->getRegisterInfo("DAQ.FD");
+  auto dbInfo = registerInfo.doubleBuffer.value();
+
+  TestableDoubleBufferAccessor<int> accessor(dbInfo, backend, mutex, RegisterPath("/DAQ/FD"), 4, 0, AccessModeFlags{});
+
+  auto buf0 = device.getTwoDRegisterAccessor<int>("/DAQ/FD.BUF0");
+  auto inactive = device.getOneDRegisterAccessor<int>("/DAQ/DOUBLE_BUF/INACTIVE_BUF_ID");
+  inactive[0] = 1;
+  inactive.write();
+
+  buf0[0] = {100, 200, 300, 400};
+  buf0.write();
+
+  // Initial read
+  accessor.doPreRead(TransferType::read);
+  accessor.doReadTransferSynchronously();
+  accessor.doPostRead(TransferType::read, true);
+
+  auto oldBuffer0 = accessor.buffer_2D[0];
+
+  // update reg
+  buf0[0] = {20, 40, 60, 80};
+  buf0.write();
+
+  // hasNewData=false → buffer_2D should not update
+  accessor.doPreRead(TransferType::read);
+  accessor.doReadTransferSynchronously();
+  accessor.doPostRead(TransferType::read, false);
+
+  BOOST_CHECK_EQUAL_COLLECTIONS(
+      oldBuffer0.begin(), oldBuffer0.end(), accessor.buffer_2D[0].begin(), accessor.buffer_2D[0].end() // still old
+  );
+  // hasNewData=true → buffer_2D should update
+  accessor.doPreRead(TransferType::read);
+  accessor.doReadTransferSynchronously();
+  accessor.doPostRead(TransferType::read, true);
+
+  BOOST_CHECK_EQUAL_COLLECTIONS(
+      buf0[0].begin(), buf0[0].end(), accessor.buffer_2D[0].begin(), accessor.buffer_2D[0].end() // now updated
+  );
+}


Again, you are testing deep down of the stuff where the framework is responsible. Calling doPostRead() with newData flag true and false is done by the TransferElement. This is all covered by the UnifiedBackendTest. No need to write dedicated tests here.

killenb · 2026-03-13T10:03:41Z

+// Test multiple channels
+BOOST_AUTO_TEST_CASE(test_multiple_channels) {
+  Device device;
+  device.open("(dummy?map=simpleJsonFile.jmap)");
+  auto backend = boost::dynamic_pointer_cast<NumericAddressedBackend>(device.getBackend());
+  auto mutex = std::make_shared<detail::CountedRecursiveMutex>();
+  auto dbInfo = backend->getRegisterInfo("DAQ.FD").doubleBuffer.value();
+
+  TestableDoubleBufferAccessor<int> accessor(dbInfo, backend, mutex, "/DAQ/FD", 4, 0, AccessModeFlags{});
+
+  auto buf0 = device.getTwoDRegisterAccessor<int>("/DAQ/FD.BUF0");
+  auto inactive = device.getOneDRegisterAccessor<int>("/DAQ/DOUBLE_BUF/INACTIVE_BUF_ID");
+  inactive[0] = 1;
+  inactive.write();
+
+  // simulate 2 channels
+  buf0[0] = {16, 20, 24, 32};
+  buf0[1] = {100, 200, 300, 400};
+  buf0.write();
+
+  accessor.doPreRead(TransferType::read);
+  accessor.doReadTransferSynchronously();
+  accessor.doPostRead(TransferType::read, true);
+
+  BOOST_CHECK_EQUAL_COLLECTIONS(
+      buf0[0].begin(), buf0[0].end(), accessor.buffer_2D[0].begin(), accessor.buffer_2D[0].end());
+  BOOST_CHECK_EQUAL_COLLECTIONS(
+      buf0[1].begin(), buf0[1].end(), accessor.buffer_2D[1].begin(), accessor.buffer_2D[1].end());
+}


What does this test that has anything to do with double buffering that is not tested above?

killenb · 2026-03-13T10:04:39Z

+  BOOST_CHECK(!accessor1->mayReplaceOther(accessor1));
+
+  BOOST_CHECK(accessor1->mayReplaceOther(accessor2));
+}


Again, this is covered by the UnifiedBackendTest. No need to write a dedicated test.

killenb · 2026-03-13T10:13:14Z

+// Edge case: numberOfWords = 0
+BOOST_AUTO_TEST_CASE(test_zero_words) {
+  Device device;
+  device.open("(dummy?map=simpleJsonFile.jmap)");
+  auto backend = boost::dynamic_pointer_cast<NumericAddressedBackend>(device.getBackend());
+  auto mutex = std::make_shared<detail::CountedRecursiveMutex>();
+  auto registerInfo = backend->getRegisterInfo("DAQ.FD");
+  auto dbInfo = registerInfo.doubleBuffer.value();
+
+  TestableDoubleBufferAccessor<int> accessor(dbInfo, backend, mutex, RegisterPath("/DAQ/FD"), 0, 0, AccessModeFlags{});
+
+  BOOST_CHECK_EQUAL(accessor.getNumberOfSamples(), 0);
+}


This is not an edge cage but a wrong test. An Accessor with 0 elements should not exist, it should even have an assertion that it never is instantiated.

Correct behaviour: If call device::getTwoDRegisterAccessor() (or OneD), with nWords =0, the backend's getRegisterAccessor_impl is returning an accessor of the full length specified in the map file.

I don't know if this is covered in the UnifiedBackendTest. If not, it should be added there (and in the specification if it is not there). Should be checked.

killenb

Please squash all commits into one. It just adds one file. The history how this was done can be squashed.

nshehz added 3 commits March 9, 2026 14:53

test: initial commit for testdoublebufferaccessor.

434e1cf

tests: extended testDoubleBufferAccessor

4d98647

fix: removed forgotten debug cout.

e3e54aa

nshehz assigned killenb Mar 10, 2026

nshehz requested a review from killenb March 10, 2026 16:15

nshehz assigned nshehz and unassigned killenb Mar 10, 2026

killenb requested changes Mar 13, 2026

View reviewed changes

killenb reviewed Mar 13, 2026

View reviewed changes


		auto dbInfo = registerInfo.doubleBuffer.value();

		TestableDoubleBufferAccessor<int> accessor(

Conversation

nshehz commented Mar 10, 2026

Uh oh!

killenb 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!

killenb left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants