Use `egui_kittest` for the graph view #8578

grtlr · 2025-01-03T18:03:27Z

What

Title.

This might be a good starting point for other views wanting to adopt egui_kittest.

Big shout out to @Wumpf how did all the query-crafting voodoo 🔮.

Co-authored-by: Andreas Reich <[email protected]>

github-actions · 2025-01-03T18:04:00Z

Web viewer built successfully. If applicable, you should also test it:

I have tested the web viewer

Result	Commit	Link	Manifest
✅	`26f73e5`	https://rerun.io/viewer/pr/8578	`+nightly` `+main`

^{Note: This comment is updated whenever you push a commit.}

crates/viewer/re_view_graph/tests/basic.rs

Wumpf · 2025-01-03T18:19:05Z

tests/python/release_checklist/check_graph_view.py

@@ -15,18 +15,12 @@
 * `graph` has directed edges, while `graph2` has undirected edges.
 * `graph` and `graph2` are shown in two different viewers.
 * There is a third viewer, `Both`, that shows both `graph` and `graph2` in the same viewer.
-* The `coincident` viewer shows two nodes, `A` and `B`, at the same position


less manual testing 🥳

Yeah! For some reason, I still have some minor non-determinism in fjadra so I can't really get rid of the remaining checks, but I'm looking into that separately.

grtlr · 2025-01-07T08:27:00Z

@rerun-bot full-check

github-actions · 2025-01-07T08:27:30Z

Started a full build: https://github.com/rerun-io/rerun/actions/runs/12647844918

grtlr · 2025-01-07T09:26:08Z

@rerun-bot full-check

github-actions · 2025-01-07T09:26:39Z

Started a full build: https://github.com/rerun-io/rerun/actions/runs/12648750684

abey79

Great stuff! Of course, I wish more of the boilerplate could be more cleanly factored. I spent a bunch of time thinking about that, left some thoughts, but ultimately couldn't come up with much actionable suggestions.

We need to keep writing such tests and have a dedicated workshop at next meet-up with @Wumpf to see how we can better factor stuff.

abey79 · 2025-01-07T08:56:25Z

crates/store/re_log_types/src/time_point/timeline.rs

-impl Default for Timeline {
-    fn default() -> Self {
-        Self {
-            name: TimelineName::default(),
-            typ: TimeType::Sequence,
-        }
-    }
-}
-


what's the rational there?

we tripped over default timelines being empty string and figured that this is practically never a valid timeline name (for all we know it is practically still valid, but even then we don't want to encourage it as the "default")

abey79 · 2025-01-07T08:57:38Z

crates/viewer/re_view_graph/tests/basic.rs

@@ -0,0 +1,252 @@
+#![expect(clippy::unwrap_used)]


Side thought: wonder is there is a global way to do that for all tests?

Ah this is a leftover from development. We have allow-unwrap-in-tests = true in clippy.toml.

I tried to remove it, but either clippy is not working nicely with the tests/ directory or it's not recognizing .unwrap() in functions that are only used in test cases.

It looks like this only works for functions annotated with #[test].

abey79 · 2025-01-07T09:04:46Z

crates/viewer/re_view_graph/tests/basic.rs

+    // It's important to first register the view class before adding any entities,
+    // otherwise the `VisualizerEntitySubscriber` for our visualizers doesn't exist yet,
+    // and thus will not find anything applicable to the visualizer.
+    test_context
+        .view_class_registry
+        .add_class::<GraphView>()
+        .unwrap();


Moar side thoughts...

In a dream world, we would have some kind of builder pattern on TestContext to provide visibility on the optional bells and whistles we might want to have:

let mut test_context = TextContextBuilder::new().with_class::<GraphView>().with_component_ui().build()

(see get_test_context() in test_all_components_ui.rs why I'd like with_component_ui())

However, this is not possible because TestContext must live low in the hierarchy and all of that stuff is from crates higher up.

I wonder if there is a way to deal with that? (I'm probably trying to over engineer here though)

we could provide extension traits to the test context for this :)

abey79 · 2025-01-07T09:14:49Z

crates/viewer/re_view_graph/tests/basic.rs

+    // TODO(andreas): can we use the `ViewProperty` utilities for this?
+    let view_contents_chunk =
+        Chunk::builder(format!("{}/ViewContents", view_id.as_entity_path()).into())
+            .with_archetype(
+                RowId::new(),
+                timepoint,
+                &re_types::blueprint::archetypes::ViewContents::new(std::iter::once(
+                    re_types::datatypes::Utf8::from("/**"),
+                )),
+            )
+            .build()
+            .unwrap();


I think you could use ViewBlueprint here. It would be cleaner because that's the "official" API (no need to hard-code stuff such as "{}/ViewContents" etc., but you'll need to use TestContext::run() and TestContext::handle_system_commands().

abey79 · 2025-01-07T09:16:55Z

crates/viewer/re_view_graph/tests/basic.rs

+    let view_blueprint = ViewBlueprint::try_from_db(
+        view_id,
+        &test_context.blueprint_store,
+        &test_context.blueprint_query,
+    )


actually, do you really need that ViewBlueprint to be stored in the blueprint store? Can't you just create one here with ViewBlueprint::new()?

you might be right but I'm not sure if there's not other things that depend on things being in the store. Populating the store correctly seems cleaner to me though (well okay ideally we use this as writing interface and process the system commands)

abey79 · 2025-01-07T09:25:57Z

crates/viewer/re_viewer_context/src/test_context.rs

@@ -30,6 +31,9 @@ pub struct TestContext {
    pub selection_state: ApplicationSelectionState,
    pub recording_config: RecordingConfig,

+    // TODO(andreas): Can we just always populate this in `run`?


I'm not sure we can, at least so long as we use view_blueprint.contents.execute_query() to populate that thing.

confirmed, right now we can't. Jochen and I actually spend quite a while trying to do this in here, but that dragged in too many dependencies

Wumpf · 2025-01-07T09:53:14Z

have a dedicated workshop at next meet-up with @Wumpf to see how we can better factor stuff

I expect to touch quite a bit of that boilerplate as part of the many-entities perf effort, so things will hopefully improve way before the meetup :)

crates/viewer/re_view_graph/tests/basic.rs

Wumpf · 2025-01-07T10:01:12Z

crates/viewer/re_view_graph/tests/basic.rs

+    let view_blueprint = ViewBlueprint::try_from_db(
+        view_id,
+        &test_context.blueprint_store,
+        &test_context.blueprint_query,
+    )


you might be right but I'm not sure if there's not other things that depend on things being in the store. Populating the store correctly seems cleaner to me though (well okay ideally we use this as writing interface and process the system commands)

Wumpf · 2025-01-07T10:02:45Z

crates/viewer/re_viewer_context/src/test_context.rs

@@ -30,6 +31,9 @@ pub struct TestContext {
    pub selection_state: ApplicationSelectionState,
    pub recording_config: RecordingConfig,

+    // TODO(andreas): Can we just always populate this in `run`?


confirmed, right now we can't. Jochen and I actually spend quite a while trying to do this in here, but that dragged in too many dependencies

crates/viewer/re_viewport/src/system_execution.rs

grtlr · 2025-01-07T10:54:57Z

@rerun-bot full-check

github-actions · 2025-01-07T10:55:32Z

Started a full build: https://github.com/rerun-io/rerun/actions/runs/12650173463

grtlr and others added 7 commits January 3, 2025 17:58

WIP

7f548ea

WIP

ee844c8

Co-authored-by: Andreas Reich <[email protected]>

wip.

865f14d

add view contents

2c08f38

WIP

7c99c5b

fun with timelines! Oh also, the test now starts working!!

fb268e7

Cleanup

59c2fea

grtlr added 🔨 testing testing and benchmarks exclude from changelog PRs with this won't show up in CHANGELOG.md labels Jan 3, 2025

Wumpf reviewed Jan 3, 2025

View reviewed changes

crates/viewer/re_view_graph/tests/basic.rs Outdated Show resolved Hide resolved

Wumpf reviewed Jan 3, 2025

View reviewed changes

grtlr marked this pull request as draft January 3, 2025 20:27

grtlr added 3 commits January 7, 2025 08:17

Re-enable tests 🙈

552ac5e

Missed Timeline::default

93f63d3

Merge branch 'main' into grtlr/kittest-graph-view

3c811c3

grtlr requested a review from Wumpf January 7, 2025 07:30

grtlr marked this pull request as ready for review January 7, 2025 07:31

grtlr added 2 commits January 7, 2025 08:43

"assign" todos to make lints pass

0d3701c

format

249cf21

grtlr added 2 commits January 7, 2025 09:32

more fixes

7e25bc0

lints

3fb2de2

abey79 approved these changes Jan 7, 2025

View reviewed changes

Wumpf approved these changes Jan 7, 2025

View reviewed changes

Address feedback

db3f97e

Doc

e41b473

Merge branch 'main' into grtlr/kittest-graph-view

26f73e5

grtlr merged commit f294dbe into main Jan 7, 2025
32 checks passed

grtlr deleted the grtlr/kittest-graph-view branch January 7, 2025 13:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `egui_kittest` for the graph view #8578

Use `egui_kittest` for the graph view #8578

grtlr commented Jan 3, 2025

github-actions bot commented Jan 3, 2025 •

edited

Loading

Wumpf Jan 3, 2025

grtlr Jan 7, 2025

grtlr commented Jan 7, 2025

github-actions bot commented Jan 7, 2025

grtlr commented Jan 7, 2025

github-actions bot commented Jan 7, 2025

abey79 left a comment

abey79 Jan 7, 2025

Wumpf Jan 7, 2025

abey79 Jan 7, 2025

grtlr Jan 7, 2025

grtlr Jan 7, 2025

grtlr Jan 7, 2025

abey79 Jan 7, 2025

Wumpf Jan 7, 2025

abey79 Jan 7, 2025

abey79 Jan 7, 2025

Wumpf Jan 7, 2025

abey79 Jan 7, 2025

Wumpf Jan 7, 2025

Wumpf commented Jan 7, 2025

Wumpf Jan 7, 2025

Wumpf Jan 7, 2025

grtlr commented Jan 7, 2025

github-actions bot commented Jan 7, 2025

Use egui_kittest for the graph view #8578

Use egui_kittest for the graph view #8578

Conversation

grtlr commented Jan 3, 2025

Related

What

github-actions bot commented Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grtlr commented Jan 7, 2025

github-actions bot commented Jan 7, 2025

grtlr commented Jan 7, 2025

github-actions bot commented Jan 7, 2025

abey79 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wumpf commented Jan 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grtlr commented Jan 7, 2025

github-actions bot commented Jan 7, 2025

Use `egui_kittest` for the graph view #8578

Use `egui_kittest` for the graph view #8578

github-actions bot commented Jan 3, 2025 •

edited

Loading