Skip to content

chore: fix screenshot workflow WD-33136#1815

Open
omarelkashef wants to merge 1 commit intocanonical:mainfrom
omarelkashef:WD-33136-revive-screenshot-workflow
Open

chore: fix screenshot workflow WD-33136#1815
omarelkashef wants to merge 1 commit intocanonical:mainfrom
omarelkashef:WD-33136-revive-screenshot-workflow

Conversation

@omarelkashef
Copy link
Contributor

@omarelkashef omarelkashef commented Feb 23, 2026

Done

  • fix screenshot workflow

QA

  1. Run the LXD-UI:
  2. Perform the following QA steps:

@webteam-app
Copy link

@omarelkashef omarelkashef marked this pull request as draft February 23, 2026 12:09
@omarelkashef omarelkashef force-pushed the WD-33136-revive-screenshot-workflow branch 3 times, most recently from df546cd to f22b2ca Compare February 23, 2026 16:47
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

Some thoughts on the open PR below.

@omarelkashef omarelkashef force-pushed the WD-33136-revive-screenshot-workflow branch 19 times, most recently from 0fa910a to 9f60765 Compare February 25, 2026 15:52
@omarelkashef omarelkashef marked this pull request as ready for review February 25, 2026 16:24
@omarelkashef omarelkashef self-assigned this Feb 25, 2026
@omarelkashef omarelkashef requested a review from edlerd February 26, 2026 08:25
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

Thanks for fixing it. Do you have a link to a local run of the screenshot tests on your branch?

Some observations on the helper changes are below.

@omarelkashef
Copy link
Contributor Author

Thanks for fixing it. Do you have a link to a local run of the screenshot tests on your branch?

Some observations on the helper changes are below.

Yes, it is included in the PR description.

@omarelkashef omarelkashef force-pushed the WD-33136-revive-screenshot-workflow branch from 9f60765 to d801998 Compare February 26, 2026 11:37
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

Good progress! Some thoughts and ideas on how to stailize the test suite below.

@edlerd
Copy link
Collaborator

edlerd commented Feb 26, 2026

Yes, it is included in the PR description.

Thanks!, I missed that. One thing I noticed when checking the screenshot from that run:

  • The tutorial/desktop_console.png file is showing a booting instance. I suspect the timeout before needs to be higher
  • The tutorial/hello_world_desktop.png file similarly is showing a booting instance.

@omarelkashef omarelkashef force-pushed the WD-33136-revive-screenshot-workflow branch from d801998 to a8184f8 Compare February 27, 2026 09:15
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

Good progress. It is passing now here and producing right screenshots. 👍

A few nitpicks on the code structure below. Then this should be good to merge from my side.

@omarelkashef omarelkashef force-pushed the WD-33136-revive-screenshot-workflow branch 2 times, most recently from 6de441f to 011ed4c Compare February 27, 2026 13:16
@omarelkashef omarelkashef requested a review from edlerd February 27, 2026 13:30
Signed-off-by: Omar Elkashef <omarelkashef01@gmail.com>
@omarelkashef omarelkashef force-pushed the WD-33136-revive-screenshot-workflow branch from 011ed4c to 83fe1d3 Compare February 27, 2026 13:37
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for fixing the screenshots suite :)

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.

3 participants