Skip to content

UI tests: Add filter/sort UI tests for the Event Table and Compute kernel table - #1019

Open
jeremynugara wants to merge 5 commits into
mainfrom
jnugara/uitest-data-accuracy
Open

jeremynugara wants to merge 5 commits into
mainfrom
jnugara/uitest-data-accuracy

Conversation

@jeremynugara

@jeremynugara jeremynugara commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Three headless UI tests that drive real filter and sort widgets and check the row counts the UI shows. Expected counts come from the displayed table data, not SQL against the schema.

  • compute_kernel_table_sort_by_duration: clicks the Duration header, checks the shown values are in order both directions.
  • compute_kernel_table_filter_duration: filters on Duration, checks the surviving row count matches a count worked out in C++ from the shown values.
  • sys_event_table_filter_restricts_rows: filters the Event Table, checks the row count drops.

The tests read the real ImGui ids the widget captures at render time (ImGui::GetItemID) instead of rebuilding them by hand.

Merged main for #1027, which moved the Event Table filter behind Advanced mode. The event-table test now switches to Advanced before filtering.

@jeremynugara jeremynugara changed the title Add data-accuracy UI tests for the Summary, Event Table, and Compute kernel tables UI tests: Add data-accuracy UI tests for the Summary, Event Table, and Compute kernel tables Aug 20, 2026
@tomk-amd

Copy link
Copy Markdown
Collaborator

I'm not sure that this test is a good idea, at least not as it's currently done. The custom SQL queries tie this test to a specific schema.

The model module of the application goes to great lengths to decouple the UI from any underlying data source (schema), while this test is strongly tied to a specific schema implementation.

@tomk-amd

Copy link
Copy Markdown
Collaborator

Below is a AI generated PR suggestion


The model layer exists so the UI never needs to know whether durations come from rocpd_kernel_dispatch, a v4 rocpd_timestamp join, or a future format. This PR reintroduces that knowledge in app_tests.cpp:

  • Direct #include "sqlite3.h" and read-only SQLite queries
  • Hard-coded table names (rocpd_kernel_dispatch, rocpd_region, …)
  • Schema branching (DbHasTimestampTable)
  • Fixture-specific assumptions (HIP launch region tracks, duration > 2000, pid/tid matching, skipping when the Summary "Others" bucket is present)

That makes these less "UI tests" and more integration tests with an embedded SQL oracle, run through the ImGui harness because that's where the runner lives.

Why I think that's a problem

  1. Wrong layer — Schema-specific oracles already belong in src/model/tests and src/controller/tests, which are designed to break when query factories or adapters change. Duplicating that logic in the UI test target creates another place to maintain on every schema evolution.

  2. Refactor noise — We could change internal storage or aggregation without changing user-visible behavior, yet these tests would fail until the oracle SQL is updated. That's the opposite of what we want from view-layer regression tests.

  3. Fixture lock-in — Several tests only really exercise rocpd-transpose.db (or skip on other shapes). CI passes, but the suite encodes sample-specific reasoning at the view boundary.

The Test Peer pattern (TopKernelsTestPeer, etc.) is the right approach for UI tests: read view state through test-only shims without widening production APIs. Raw SQLite in the same file steps outside that pattern.

Suggested path forward

Please consider splitting this PR:

Keep in UI tests (app_tests.cpp) Move to model/controller tests
Widget-driven behavior: sort header click → verify row order via peer / DataProvider Summary duration/count vs independent SQL oracle
Filter/clear restore, tab activation, async drain patterns Event Table filter vs rocpd_region SQL count
In-memory oracles derived from displayed data (e.g. filter expected count from parsed table rows) Schema branching helpers (DbScalarQuery, DbHasTimestampTable)

For data-accuracy checks that must stay end-to-end, prefer an oracle through DataProvider / Controller API (what the view already uses) rather than bypassing the stack with direct SQL. If SQL oracles are needed, they fit naturally next to existing coverage in rocprofvis_controller_system_tests.cpp and rocprofvis_dm_system_tests.cpp.

Happy to re-review once the SQL-oracle pieces are relocated (or if we explicitly decide to accept fixture-locked E2E oracles in the UI harness and document that trade-off).

@jeremynugara

Copy link
Copy Markdown
Contributor Author

Thanks Tom, that makes sense. I'll take a look at splitting it along those lines and follow up when ready.

@jeremynugara
jeremynugara force-pushed the jnugara/uitest-data-accuracy branch from 3786cde to c32aa58 Compare August 26, 2026 18:45
Add compute_kernel_table_sort_by_duration, compute_kernel_table_filter_duration,
and sys_event_table_filter_restricts_rows, plus a ReachKernelTableOrSkip helper.
Each restores the tab, selection, time range, and filter state it mutates before
asserting so nothing leaks into later tests in the reused process.
@jeremynugara
jeremynugara force-pushed the jnugara/uitest-data-accuracy branch from c32aa58 to 70e2cc2 Compare August 26, 2026 19:44
@jeremynugara jeremynugara changed the title UI tests: Add data-accuracy UI tests for the Summary, Event Table, and Compute kernel tables UI tests: Add filter/sort UI tests for the Event Table and Compute kernel table Aug 26, 2026
… of rebuilding them

The sort and filter tests addressed the Duration header and per-column filter
box by rehashing ImGui's id from the label plus table id ($$2/##filter), with
comments citing widget source line numbers. That coupling breaks silently on any
PushID or layout change in the widget.

Capture the ids ImGui actually assigns at render time (ImGui::GetItemID) into
IMGUI_ENABLE_TEST_ENGINE-guarded members, exposed via KernelMetricTableTestPeer,
matching the existing FlameWindowId pattern. Tests now read the real ids; the
table_win reconstruction plumbing is gone.
…rows

PR #1027 gated the SQL WHERE filter box and Submit button behind Advanced filter
mode; the table now defaults to Basic. The test typed its predicate assuming the
box was always present, so after merging main it aborted on the missing widgets.

Open the funnel Filter Mode menu and select Advanced before driving the filter.
… instead of rebuilding them"

This reverts commit dad7360.
@jeremynugara

Copy link
Copy Markdown
Contributor Author

Reworked according to your review suggestions. Took out the SQL oracle approach. Also merged main in since #1027 affected the Event Table.

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.

2 participants