Add element type information and ids to the testing API #5254

tronical · 2024-05-16T16:21:34Z

This requires additional debug information to be generated, which is disabled by default in Rust and in C++ for release builds.

tronical · 2024-05-16T16:36:13Z

Oops, too early to review. There are some issues.

hunger · 2024-05-16T19:21:32Z

internal/compiler/object_tree.rs

+pub struct ElementDebugInfo {
+    // The id qualified with the enclosing component name. Given `foo := Bar {}` this is `EnclosingComponent::foo`
+    pub qualified_id: Option<String>,
+    pub type_name: String,


So you have no access to the AST anymore? Otherwise that info is not too hard to find ;-)

What happens when you import Button as MyButton? What's going to be stored in there?

Exactly because of that this patch does not use the AST. It should be Button, not MyBytton. That’s why the code uses the ElementType’s type_name():

slint/internal/compiler/object_tree.rs

Line 942 in 1069811

let type_name = base_type.type_name().unwrap_or_default().to_string();

ogoffart · 2024-05-31T07:43:26Z

api/rs/build/lib.rs

+    /// Enables the emission of additional debug information in the generated code,
+    /// needed for use with Slint's testing APIs. Disabled by default.
+    #[must_use]
+    pub fn with_debug_info(self, enable: bool) -> Self {


This adds this public API right now, however the "Slint's testing APIs" are not yet stable. Should this be behind an experimental flag? Or maybe just left out at this point?

If we leave it out, the experimental APIs cannot be used - so it's a bit of a chicken and egg problem. So I'd keep this in and in the unlikely event that we totally decide against having a testing API, then we deprecate this function and make it a no-op.

ogoffart · 2024-05-31T07:49:18Z

api/rs/macros/lib.rs

@@ -345,6 +345,11 @@ fn extract_compiler_config(
                            _ => break,
                        }
                    }
+                    Some(TokenTree::Ident(debug_info_ident))
+                        if debug_info_ident.to_string() == "debug_info" =>


the problem with that is that the developer can't make this conditional to a feature in this crate or something like that.
I'm a bit unsure on how best to do that.
Perhaps an env variable set from the build.rs would be better?
Or is there a way to detect if it is building a test? Maybe CARGO_CFG_TEST but i don't know if it's set.

Yes, this is sub-optimal and not quite correct. I did that just so that our own tests pass, but this is not really suitable. I'd keep this like the other options we have there, but they are undocumented.

Since we're not in build.rs but in the crate build itself, maybe CARG_CFG_TEST works - that would be nice. I can try that.

There's one other scenario and that's system tests for a Rust app that uses this macro. I ran into this in the example app, but I guess I need to change that to just not use the macro.

Ah no, I think I got it wrong - we still need a solution for build.rs, and inside build.rs we don't know if later tests will be run or not.

Ok, alternate proposal:

Objective:

Support unit testing via search_api.rs for Rust applications, using build.rs and the slint! macro.

Support unit testing via slint-testing.h for C++ apps

Support system testing via Rust and a slint/system-testing feature

Support system testing via C++ and a SLINT_FEATURE_SYSTEM_TESTING feature

(In theory we have the same for JS and Python, but those use the interpreter and we always have the info available, or can access it if needed)

We remove any debug_info(bool)/--generate-debug-info=true options from the compiler.

We always emit the code to retrieve the element information, but we guard it with #[cfg(test)] and #ifdef SLINT_ENABLE_DEBUG_INFO (or so). This covers both (1) and (2).

For (4), we could add a define to the interface definitions, and it'll be visible during application compilation.

For (3) .... I don't know yet.... :-(

We always emit the code to retrieve the element information, but we guard it with #[cfg(test)] and #ifdef SLINT_ENABLE_DEBUG_INFO (or so). This covers both (1) and (2).

Good idea. Although #[cfg(test)] won't work with rust system-testing i think.

For the macro, we could forward the slint/system-testing feature to the macro.
For the build, we could either have a feature, or an api function, or the build.rs from slint crate (that doesn't exist) could print something based on the enabled feature so that the slint-build can access env variables.

I think for now we could simply use an env variable since it is internal only.

internal/backends/testing/search_api.rs

ogoffart · 2024-05-31T08:09:21Z

internal/backends/testing/search_api.rs

+    ///            ("".into(), "ButtonBase::root".into())
+    ///           ]);
+    /// ```
+    pub fn element_type_names_and_ids(


If it was me, I'd only give the "App::mybutton" and not the root for everything.
I'd have element_id(&self) -> Option<SharedString> and element_type(&self) -> Option<SharedString> as single function.

This means you cannot match based on the root.
Maybe we could do element_bases(&self) -> Option<impl Iterator<Item = SharedString>> so we could still match on the root.

Good idea, that makes the API easier to use for the common case.

This is implemented now (also for C++).

tests/cases/testing/find_by_element_id_or_type.slint

api/cpp/tests/testing.cpp

tests/cases/testing/find_by_element_id_or_type.slint

tronical · 2024-05-31T14:07:07Z

I need to rebase this as the CI merges this with master and then it doesn't compile anymore due to compiler API changes.

also compile the LSP

Simplify a bit the llr generation so that there is less code duplication between Rust and C++

By default with C++ it's enabled if debug info is on CONFIG. For Rust it's in the CompilerConfiguration API. For the interpreter it's always enabled, since ... we have it.

Replace dummy accessible-label use with the new element id lookup mechanism for testing.

Keep merging elements, but remember the boundaries in the debug info, separated by a slash. Also fixed tests that rely on accessible-label being set only once. For example ``` Button { text: "foo"; } ``` will certainly have "foo" as accessible-label on `Button`, but its internal `Text` element has an implicit "accessible-label" set to the same value. So don't rely on that for now but search by id instead.

Describe what an id is and provide an example.

De-duplicate the example and the same code in the test.

…, type_name() and bases() functions This API may be easier to use.

ogoffart

I think this is almost good to go. My only slight concern at this point is that the API in slint-build and slint-macro is being available even though this is still an experimental feature.

but since this cannot be used anyway i think it's probably fine to add it and to change later.

ogoffart · 2024-06-01T07:52:39Z

api/rs/macros/lib.rs

@@ -345,6 +345,11 @@ fn extract_compiler_config(
                            _ => break,
                        }
                    }
+                    Some(TokenTree::Ident(debug_info_ident))
+                        if debug_info_ident.to_string() == "debug_info" =>


We always emit the code to retrieve the element information, but we guard it with #[cfg(test)] and #ifdef SLINT_ENABLE_DEBUG_INFO (or so). This covers both (1) and (2).

Good idea. Although #[cfg(test)] won't work with rust system-testing i think.

For the macro, we could forward the slint/system-testing feature to the macro.
For the build, we could either have a feature, or an api function, or the build.rs from slint crate (that doesn't exist) could print something based on the enabled feature so that the slint-build can access env variables.

I think for now we could simply use an env variable since it is internal only.

ogoffart · 2024-06-01T07:59:14Z

internal/compiler/passes/optimize_useless_rectangles.rs

@@ -72,5 +75,5 @@ fn can_optimize(elem: &ElementRc) -> bool {
    !e.bindings.keys().chain(analysis.iter().filter(|(_, v)| v.is_set).map(|(k, _)| k)).any(|k| {
        !e.property_declarations.contains_key(k.as_str())
            && base_type.properties.contains_key(k.as_str())
-    })
+    }) && e.accessibility_props.0.is_empty()


i'm thinking we should still be able to optimize that but make sure to keep the accessibility prop. Anyway, this is simpler for this PR so let's go with that.

tronical requested a review from ogoffart May 16, 2024 16:21

tronical force-pushed the simon/item-debug-id branch from 243f773 to 1069811 Compare May 16, 2024 16:25

tronical removed the request for review from ogoffart May 16, 2024 16:35

tronical marked this pull request as draft May 16, 2024 16:36

hunger reviewed May 16, 2024

View reviewed changes

tronical force-pushed the simon/item-debug-id branch 4 times, most recently from 20d9131 to c918cda Compare May 17, 2024 15:36

tronical marked this pull request as ready for review May 17, 2024 17:30

tronical requested a review from ogoffart May 17, 2024 17:31

tronical force-pushed the simon/item-debug-id branch 2 times, most recently from 4ed68de to 6058260 Compare May 21, 2024 07:38

tronical mentioned this pull request May 27, 2024

Fix text update of SpinBox #5243

Merged

tronical force-pushed the simon/item-debug-id branch from 7977853 to 330ba4f Compare May 27, 2024 16:55

ogoffart reviewed May 31, 2024

View reviewed changes

github-actions bot reviewed May 31, 2024

View reviewed changes

tests/cases/testing/find_by_element_id_or_type.slint Outdated Show resolved Hide resolved

github-actions bot reviewed May 31, 2024

View reviewed changes

api/cpp/tests/testing.cpp Outdated Show resolved Hide resolved

tests/cases/testing/find_by_element_id_or_type.slint Outdated Show resolved Hide resolved

Don't optimized away rectangles that have dedicated accessibility roles

d40d34d

tronical force-pushed the simon/item-debug-id branch 4 times, most recently from 501eab5 to f9c2e2d Compare May 31, 2024 14:23

tronical and others added 5 commits May 31, 2024 16:25

WIP: Search by qualified element id

7c2166c

element_ids: Can't return a SharedString by value from the vtable

73da66b

also compile the LSP

C++ part of element_ids

c6088c5

Simplify a bit the llr generation so that there is less code duplication between Rust and C++

Make element debug information configurable

a17ae9b

By default with C++ it's enabled if debug info is on CONFIG. For Rust it's in the CompilerConfiguration API. For the interpreter it's always enabled, since ... we have it.

WIP: Add find_by_element_type_name

ba27be9

tronical added 7 commits May 31, 2024 16:25

Expose element type names and ids in Rust and C++

aef7b98

Simplify spinbox test

9751568

Replace dummy accessible-label use with the new element id lookup mechanism for testing.

Add C++ API for find_by_element_type_name

3893032

Fix ElementHandle::find_by_element_id docs

aef9ec2

Describe what an id is and provide an example.

Simplify docs for element_type_names_and_ids

dbafe6d

De-duplicate the example and the same code in the test.

Replace element_type_names_and_ids in the ElementHandle API with id()…

b0364ec

…, type_name() and bases() functions This API may be easier to use.

tronical force-pushed the simon/item-debug-id branch from f9c2e2d to b0364ec Compare May 31, 2024 14:26

ogoffart reviewed Jun 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add element type information and ids to the testing API #5254

Add element type information and ids to the testing API #5254

tronical commented May 16, 2024

tronical commented May 16, 2024

hunger May 16, 2024

tronical May 16, 2024

ogoffart May 31, 2024

tronical May 31, 2024

ogoffart May 31, 2024

tronical May 31, 2024

tronical May 31, 2024

ogoffart Jun 1, 2024

ogoffart May 31, 2024

tronical May 31, 2024

tronical May 31, 2024

tronical commented May 31, 2024

ogoffart left a comment

ogoffart Jun 1, 2024

ogoffart Jun 1, 2024

Add element type information and ids to the testing API #5254

Are you sure you want to change the base?

Add element type information and ids to the testing API #5254

Conversation

tronical commented May 16, 2024

tronical commented May 16, 2024

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

tronical commented May 31, 2024

ogoffart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment