Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a snapshot test for list items #8248

Merged
merged 2 commits into from
Nov 29, 2024
Merged

Conversation

abey79
Copy link
Member

@abey79 abey79 commented Nov 29, 2024

What

snapshot:

@abey79 abey79 added 🔨 testing testing and benchmarks ui concerns graphical user interface exclude from changelog PRs with this won't show up in CHANGELOG.md labels Nov 29, 2024
Copy link

github-actions bot commented Nov 29, 2024

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

  • I have tested the web viewer
Result Commit Link
dd57d44 https://rerun.io/viewer/pr/8248

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

re_ui::apply_style_and_install_loaders(ui.ctx());
list_item::list_item_scope(ui, "list_item_scope", &mut test_code);
});
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Food for thought: it would be nice if we could set things up so that some screenshots tests (like this one) would also be actually runnable (for additional manual testing).

I don't know how (maybe @lucasmerlin has a good idea?), but it would be cool.
In this case I guess the test could actually call some piece of code shared with re_ui_example.

Anyways, not necessary for this PR obviously, just an interesting idea imho

Copy link
Contributor

@lucasmerlin lucasmerlin Nov 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already thought about something like this, I think we could allow this for all tests, just running the app closure via eframe before the test is executed. This could be enabled via a feature and an environment variable like DEBUG_TESTS or as a method on harness.

Edit: The weird thing is that now there would be two different egui contexts, which will probably lead to unexpected results in complex uis. Not sure if we could pass an existing egui Context to eframe and tell it to use that somehow

Copy link
Contributor

@lucasmerlin lucasmerlin Nov 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gave this a try, I don't think this will be easily possible 🥲

on macOS, `EventLoop` must be created on the main thread!

But maybe we could use https://github.com/emilk/eterm? 😄 Perfect usecase for that. That might also be able to solve the two context problem

@abey79 abey79 merged commit 3e6925d into main Nov 29, 2024
31 checks passed
@abey79 abey79 deleted the antoine/list-item-snapshot-test branch November 29, 2024 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exclude from changelog PRs with this won't show up in CHANGELOG.md 🔨 testing testing and benchmarks ui concerns graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants