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

masonry: Disable render tests by default #292

Conversation

waywardmonkeys
Copy link
Contributor

Set both ENABLE_RENDER_TESTS=1 and ENABLE_RENDER_SNAPSHOTS=1 in your environment to run the tests.

Set both `ENABLE_RENDER_TESTS=1` and `ENABLE_RENDER_SNAPSHOTS=1`
in your environment to run the tests.
@waywardmonkeys waywardmonkeys requested a review from DJMcNab May 8, 2024 10:50
@waywardmonkeys
Copy link
Contributor Author

My concerns with this PR:

  1. There's no documentation for how to run the tests yet, so nowhere to mention how to re-enable them.
  2. I'm not sure we need 2 separate environment variables for now.

@@ -238,7 +243,7 @@ impl TestHarness {
/// Create a bitmap (an array of pixels), paint the window and return the bitmap as an 8-bits-per-channel RGB image.
pub fn render(&mut self) -> RgbaImage {
let (scene, _tree_update) = self.render_root.redraw();
if std::env::var("SKIP_RENDER_TESTS").is_ok_and(|it| !it.is_empty()) {
if !check_env_enabled("ENABLE_RENDER_TESTS") {
Copy link
Member

Choose a reason for hiding this comment

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

So as it happens, the render tests mostly work fine

So I'd probably revert this half of the change

@waywardmonkeys
Copy link
Contributor Author

One other difference is that one part of this was reading the env variable at runtime via std::env::var while the other part was reading it at compile time via option_env!.

@DJMcNab
Copy link
Member

DJMcNab commented May 22, 2024

So, my expected updates for this PR would be.

  1. Keep the change to read at runtime for both
  2. Re-invert the SKIP_RENDER_TESTS flag, as that is only useful for Windows CI

Alternatively, we could close this pending the fixed snapshot tests (i.e. #233), but that is at least another couple of weeks away.
I can make these changes if needed.

@flosse flosse added the masonry Issues relating to the Masonry widget layer label Aug 13, 2024
@PoignardAzur
Copy link
Contributor

I think we can close this now that render tests work?

@waywardmonkeys waywardmonkeys deleted the disable-render-tests-by-default branch August 16, 2024 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
masonry Issues relating to the Masonry widget layer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants