Add comprehensive unit tests and improve input validation#13
Closed
Add comprehensive unit tests and improve input validation#13
Conversation
- Plot_widget and Plot_time_axis setters for t-range, available t-range and v-range now silently ignore non-finite or mis-ordered inputs instead of letting NaN/inf propagate into layout math and rendered time<->pixel conversions. - Plot_widget::set_view validates each optional range independently and no longer partially applies an available-range when the primary range is missing. - Narrow the torn-read window in set_rendered_v/t_range by invalidating before publishing new endpoints; document that a seqlock would be required for a full fix. Also: - Plot_config::field_count was out of sync with the struct (20 vs. the 22 actual fields); bumping it to match keeps the plot_config_equivalent static_assert honest so future fields still force comparator updates. - Document Series_renderer::initialize as infallible (shader compilation is lazy) and clarify the data_snapshot_t lifetime contract so consumers know hold must be retained for the view to remain valid. - Add a threading-model summary to Plot_widget so call sites know which methods are GUI-thread vs. render-thread, and that invalid range inputs are ignored rather than partially applied. https://claude.ai/code/session_012kn22AT9HdT6aoJoi8rz4Y
Address the remaining items from the earlier review:
- Segregate Plot_widget's render-thread callbacks. The two _from_renderer
methods still need to be addressable because Plot_renderer posts to
them through QMetaObject::invokeMethod, but they no longer carry
Q_INVOKABLE, so QML cannot call them, and a header comment marks them
as internal-only.
- Route shader and asset-loading failures through Plot_config::log_error.
Plot_core::render now forwards the callback to the Asset_loader and
Primitive_renderer (matching what plot_renderer.cpp does for the Qt
path), and Series_renderer::get_or_load_shader logs each failure mode
(no asset loader, empty vert stage, asset lookup miss, compile/link
failure) so silent fallbacks stop swallowing bugs.
- Document the float-precision tradeoff of Data_access_policy::get_value
and get_range so users know to subtract large biases before narrowing
from double to float.
- Align the CMake project VERSION with the 1.0.4 tag referenced in the
README FetchContent example so install metadata and docs no longer
disagree.
- Add three new core-only tests:
* test_core_algo covers LOD level selection, contiguous and segmented
timestamp binary searches, and the axis-label formatter.
* test_asset_loader covers missing-asset logging, embedded lookup,
override-directory precedence and fallback, and shader helper.
* test_concurrent_series runs a ring-buffer Data_source under two
readers plus a writer and verifies the shared_ptr-based hold
semantics keep snapshots safe after further writes.
All seven tests pass on both core-only and Qt builds.
https://claude.ai/code/session_012kn22AT9HdT6aoJoi8rz4Y
Previously set_vbar_width_from_renderer and set_auto_v_range_from_renderer were public (without Q_INVOKABLE) because Plot_renderer's pimpl nested class (impl_t) could not take pointer-to-member expressions to private methods of Plot_widget — the friend class Plot_renderer declaration does not extend to nested types. Move the hooks into Plot_widget's private section and expose two static forwarders on Plot_renderer (post_vbar_width_from_renderer and post_auto_v_range_from_renderer). The forwarders form the pointer-to- member inside Plot_renderer's access context (where friendship applies) and pass it to QMetaObject::invokeMethod; impl_t calls those forwarders instead, which it can do because nested classes share their enclosing class's access rights. Net result: the methods no longer appear in the header's public section and can never be called by application code or QML, while the queued- invocation mechanism is preserved. https://claude.ai/code/session_012kn22AT9HdT6aoJoi8rz4Y
Public-header changes: - Move plot_renderer.h out of include/vnm_plot/qt/ into src/qt/. The class is only constructed by Plot_widget::createRenderer() and returned as the QQuickFramebufferObject::Renderer base type, so users never had a reason to see it. - Drop the four sub-renderer accessors from Plot_core (primitives, series_renderer, chrome_renderer, text_renderer). Nothing under examples/, benchmark/, benchmark_headless/, or tests/ used them; they were leaky pass-throughs that exposed renderer internals. Only asset_loader() — needed for asset overrides — stays. - Stop core.h umbrella from pulling in default_shaders.h and vertex_layout.h. vertex_layout.h still reaches users transitively through access_policy.h; default_shaders.h is renderer-internal. - Sync vnm_plot.h k_version_* with the 1.0.4 CMake project version that the README also references. - Rename Q_PROPERTY timeAxis → time_axis to match every other property in Plot_widget; update PlotIndicator.qml, PlotView.qml, README. Dedup / simplification: - Extract update_config_field<>() so set_dark_mode / set_grid_visibility / set_preview_visibility / set_line_width_px share one lock+revision +signal+update body instead of repeating it four times. - Extract clamp_t_range_to_available() and use it from both set_available_t_range and set_view, removing one verbatim copy of the pan-clamping logic. - Use hash_mix_u64 consistently across hash_data_sources and hash_series_snapshot (the helper existed but only the third hasher was using it). - Add data_snapshot_t::is_valid() and replace the `!snap || count == 0 || stride == 0` check at every site (six call sites across core, qt, and benchmark). Test cleanup: - Move TEST_ASSERT and RUN_TEST into tests/test_macros.h and include it from all seven test binaries instead of duplicating the macros in each. All 7 tests still pass on the full Qt build. https://claude.ai/code/session_012kn22AT9HdT6aoJoi8rz4Y
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds three new comprehensive unit test suites covering core algorithm functionality, asset loading, and concurrent data sources. It also hardens input validation across the codebase by rejecting non-finite values in range setters and improving error logging in shader compilation.
Key Changes
New Test Suites
Input Validation Hardening
std::isfinite()checks in Plot_widget and Plot_time_axis range setters to reject NaN and infinity valuesDocumentation & Clarity
data_snapshot_texplaining the hold-based buffer pinning mechanismBuild System
Version & Config
Implementation Details
https://claude.ai/code/session_012kn22AT9HdT6aoJoi8rz4Y