Conversation
Signed-off-by: Alejandro Hernandez Cordero <ahcorde@gmail.com>
NP. On it. |
PavelGuzenfeld
left a comment
PavelGuzenfeld
left a comment
There was a problem hiding this comment.
Thanks for the tests @ahcorde! The two added tests correctly validate the core bug from #769 — getData() on an empty cache now returns false, and returns true after insertion.
Missing unit test coverage — the fix in #908 also changed these methods:
clearList()— now resetspopulated_tofalse. Without a test, a regression here means cleared caches would still return stale data.getListLength()— now returns 0 when empty, 1 when populated. Previously always returned 1.getParent()— now returns 0 on empty cache instead of reading uninitializedframe_id_.
Sanitizer considerations — the original issue was actually discovered under ASan (--mixin asan-gcc). The bug caused callers to read uninitialized TransformStorage fields when the cache was empty. Worth verifying:
- ASan — confirms no reads of uninitialized/out-of-bounds memory on the empty-cache path
- UBSan — the old code returned
storage_.frame_id_from a default-constructedTransformStorage; depending on the type, this could be undefined behavior
Integration test idea — a tf2_ros level test that sets up a StaticTransformBroadcaster, queries a frame before any static transform is published, and confirms the lookup fails cleanly rather than returning garbage data. This would catch the bug at the user-facing API level, not just the cache internals.
Thread safety — StaticCache can be read/written from different threads (executor callbacks vs. transform lookups). A test that hammers insertData() and getData() concurrently under TSan would confirm the populated_ flag doesn't introduce a data race. The flag is a plain bool, not std::atomic<bool> — worth verifying this is safe under the existing mutex in BufferCore.
resetStorage() helper placement — it's defined as a free function between test cases. Moving it into the test fixture or an anonymous namespace would be cleaner and avoid potential ODR issues if more test files are added later.
Error string/code validation — the EmptyCacheRetrieval test checks the return value but not the error_str or error_code output parameters. The fix in #908 sets error_str = "Static cache is empty" and error_code = TF2_LOOKUP_ERROR — asserting on those would prevent someone from accidentally removing the error reporting while keeping the return false.
|
Pulls: #920
|
3 participants
Description
@PavelGuzenfeld do you mind to review this one ?
Add test to check static cache functionality. related with #769 and #908