Add GNOME screen recording test #505
No reviewers
Labels
No labels
ai-review-please
Closed As
Duplicate
Closed As
Fixed
Closed As
Invalid
easyfix
freeipa
improvetest
iot
newtest
silverblue
tracker
Backlog Status
Needs Review
Backlog Status
Ready
chore
documentation
points
01
points
02
points
03
points
05
points
08
points
13
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Sprint Status
Blocked
Sprint Status
Done
Sprint Status
In Progress
Sprint Status
Review
Sprint Status
To Do
Technical Debt
Work Item
Bug
Work Item
Epic
Work Item
Spike
Work Item
Task
Work Item
User Story
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
quality/os-autoinst-distri-fedora!505
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "newtest/screen_recording"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Introduce a new openQA test module for GNOME screen recording on Wayland.
The test exercises the screenshot overlay workflow, starts and stops a recording,
and verifies that a non-empty screencast file is created in ~/Videos/Screencasts/.
AI Code Review
📋 MR Summary
Adds a new openQA test module for GNOME screen recording on Wayland.
screen_recordingtest intemplates.fif.json.tests/applications/screen_recording/screen_recording.pmto exercise the GNOME screenshot recording workflow.Detailed Code Review
The test implementation is straightforward and clearly models the intended screen recording workflow. However, the bash checks at the end of the test rely on glob expansions (
*.webm) which will fail or behave unexpectedly if there are multiple recordings in the directory. It is crucial to handle cases where previous tests or earlier runs might have left.webmfiles behind. Consider cleaning the directory before starting the recording or making the assertions robust against multiple files.📂 File Reviews
📄 `tests/applications/screen_recording/screen_recording.pm` - Implements the test logic for verifying GNOME screen recordings.
[ -s /home/$user/Videos/Screencasts/*.webm ]will fail with 'too many arguments' if there are multiple.webmfiles in the directory.-scheck,stat -c%s /home/$user/Videos/Screencasts/*.webmwill fail if the glob expands to multiple files.$userin paths, which relies onget_var("USER_LOGIN") // "test". If the user hasn't logged in yet or the directory structure isn't created, the checks could fail. Also, the comment says 'It sets a deterministic keyboard shortcut', but no code actually configuresgsettingsas implied.gsettingsis actually being configured or update the comment. Clear theScreencastsdirectory at the start of the test.Questions:
Screencastsdirectory is empty before this test runs?gsettingsconfiguration mentioned in the comments actually being executed?✅ Summary
.webmfiles to handle multiple files correctly or clear the directory beforehand.gsettingsshortcut configuration mentioned in the script comments.🤖 AI Code Review | Generated with ai-code-review | Model:
gemini-3.1-pro-preview⚠️ AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.
Remark to the AI results:
Screencastsdirectory is always empty, because we run on a clean installation -> if it is not, we will probably hit some weird bug.85208fe3253c680041b43c680041b4ae345039aeThanks, a few comments in line.
@ -0,0 +11,4 @@my $self = shift;my $user = get_var("USER_LOGIN") // "test";# Open a terminal to configure gsettingswe are not in fact doing anything with gsettings here.
@ -0,0 +14,4 @@# Open a terminal to configure gsettingsmenu_launch_type("terminal", checkstart => 1, maximize => 1);# Wait for gsettings to applyor here.
@ -0,0 +50,4 @@sleep(2);type_very_safely("pwd\n");sleep(2);this seems like an awful lot of typing for no good reason. we don't actually check that any of this stuff happened or was recorded. why not cut it down?
Well, I wanted to generate some content for the recorded video without having to use needles. The commands actually do not matter, so I could use shorted commands and less commands. Maybe 2 - 3 would do, too.
@ -0,0 +76,4 @@# Optional: Check file size is reasonable (at least 10KB for a few seconds of recording)assert_script_run("[ \$(stat -c%s /home/$user/Videos/Screencasts/*.webm) -gt 10000 ]");}can we do any better here? can we not play back the video and assert that we see something in the playback?
Dtto. I wanted to avoid creating needles. We can play the video and assert something from it.
1500e8393e9518a489519518a48951a7bb1b59b8a7bb1b59b8c63572c238bbf02f78a22274f9f873Passes:
https://openqa.stg.fedoraproject.org/tests/6186892#step/screen_recording/13
Don't know why
checkwikiis failing. Thebasic_networking_*has been already merged.You didn't rebase the fedora_openqa
newtest/screen_recordingbranch on the basic_networking changes. I'll fix it.AI Code Review
📋 MR Summary
Introduces a new openQA test module to verify GNOME screen recording functionality on Wayland.
screen_recordingtest intemplates.fif.json.screen_recording.pmwhich records terminal activity and verifies playback.Detailed Code Review
The implementation aligns well with the previously discussed requirements. You correctly avoided unnecessary file-system checks, implemented functional verification via video playback, kept the terminal commands concise, and successfully removed the unused
gsettingsimports. The code is clean and the test steps flow logically. I found one minor unused variable that can be cleaned up.📂 File Reviews
📄 `tests/applications/screen_recording/screen_recording.pm` - Implements the core openQA test logic for screen recording and playback verification.
$useris declared and initialized but never used in the script. This can be safely removed to keep the code clean.$uservariable.✅ Summary
$uservariable inscreen_recording.pm.🤖 AI Code Review | Generated with ai-code-review | Model:
gemini-3.1-pro-preview⚠️ AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.
2274f9f87388bd61b782From overnight run on stg, I see an issue on Silverblue - there's no video player installed by default, so the test will need to install one in a prep step.
@adamwill wrote in #505 (comment):
Hmm, I am trying with Fedora-Silverblue-ostree-x86_64-44-20260402.n.0.iso and
Videosare installed by default and it works wonders.Huh. Well, that's not what openQA saw.
a408f1fa8ddeccb2cf62@adamwill wrote in #505 (comment):
I think that this could be an error on the ISO. How do we know what assets should be installed on Silverblue by default? I would expect the same ones as on Workstation. In that case, shouldn't we want this test failed in case Videos is not there?
Anyway, yesterday's ISO did not have that either, so I am adding the pre-installation check on Silverblue that installs the player in case it is not installed.
e2c1ff3af862e269358b62e269358bd8e2316262590f1bddbd5728c459d1https://openqa.stg.fedoraproject.org/tests/6208721#details
I don't think we'd want this test to fail in that case, because this test isn't meant to test "does the image have a video player?" It's meant to test "does screen recording work?" The video player is just an accessory we need to achieve the thing we're actually testing. So I think at worse it would be a soft failure.
Potentially some other test should fail if an expected default app isn't present, like apps_startstop I guess. But I thought it was intended that Silverblue doesn't have a built-in video player ATM. I'll ask the Silverblue folks.
AI Code Review
📋 MR Summary
Adds a new openQA test module for GNOME screen recording on Wayland. The test exercises the screenshot overlay, starts a recording, executes commands, and verifies the generated screencast by opening and playing it.
tests/applications/screen_recording/aaa_setup.pmto conditionally install Showtime on Silverblue variants.tests/applications/screen_recording/screen_recording.pmto perform the actual GNOME screen recording workflow and verification.templates.fif.jsonto register the newscreen_recordingtest in the relevant profile groups.Detailed Code Review
The implementation aligns well with the author's stated intentions and reviewers' past feedback. The reduction in typed commands and the shift to verifying video playback are both present. The
aaa_setup.pmscript correctly handles the Silverblue case where the default video player might be missing.However, there are a couple of logic/robustness issues in the implementation. In
aaa_setup.pm, thescript_runcommand for flatpak info returns a status code, not a boolean/string representation of success, which might invert the expected logic if not careful. Inscreen_recording.pm, the verification step usessleep(14)followed bysend_key("spc")and an assertion. Hardcoded long sleeps in UI tests are brittle and can lead to flaky tests or false negatives if the video loading takes longer than expected or if the video finishes before the sleep completes. Using a mechanism that dynamically waits for the video player window or playback UI elements is generally preferred over absolute sleeps.📂 File Reviews
📄 `tests/applications/screen_recording/aaa_setup.pm` - Sets up the test environment by installing the Showtime video player on Silverblue if it's missing.
script_runis an integer exit code (0 for success, non-zero for failure). The conditionif ($app_missing)evaluates to true if the command fails (returns non-zero). If the command succeeds (returns 0), it evaluates to false. This actually works correctly for checking if the app is missing, but naming the variable$app_missingand relying on the truthiness of a non-zero exit code is confusing and prone to logic errors. It is better to explicitly check the return status.📄 `tests/applications/screen_recording/screen_recording.pm` - Performs the screen recording test workflow and validates the output by playing the video.
sleep(14)is highly susceptible to flakiness. The video being recorded only captures roughly 6 seconds of activity (whoami+sleep(1)+uname -r+sleep(1)+exit+wait_still_screen(3)). Waiting 14 seconds before pressing space might result in the video finishing playback and closing or resetting, leading to the subsequent assertion failing.assert_screento wait for the video player UI to become active before checking playback status.unless (check_screen("screenshot_screencast_recorded", timeout => 1))block executes, it records a soft failure but continues. If the recording actually failed to save, the subsequent steps (Nautilus, double-clicking the video) will fail with a hard timeout, making the soft failure redundant.✅ Summary
sleep(14)inscreen_recording.pmwhich is longer than the actual recorded video, risking test flakiness.script_runinaaa_setup.pmto avoid confusion.🤖 AI Code Review | Generated with ai-code-review | Model:
gemini-3.1-pro-preview⚠️ AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.
Thanks for the revision, but several more notes in line.
@ -0,0 +11,4 @@sub run {my $self = shift;if (get_var("SUBVARIANT") eq "Silverblue") {I would prefer using
get_var("CANNED"), that's our standard check for atomic-ness (I came up with the name long before atomic was the standard. for now.)@ -0,0 +17,4 @@$self->root_console(tty => 3);my $app_missing = script_run("flatpak info org.gnome.Showtime");if ($app_missing) {rather than the AI suggestion, can we just do
if (script_run("flatpak info org.gnome.Showtime")) { ..., not assign a variable? there's no need for the variable as we only use it once, immediately.@ -0,0 +24,4 @@desktop_vt();}else {record_info("Silverblue check", "This test does not run on Silverblue, nothing to do.");is this really necessary?
Well, we once said that we wanted more info on the test result screens for people to get better orientation. So this is why. Necessary? No, it's not.
I meant more "is this really useful information for orienting yourself in execution of this test?" but hey, it doesn't hurt, so if you think it's useful, fine.
@ -0,0 +24,4 @@assert_and_click("screenshot_icon_recordscreen", timeout => 5);# Select "Show Cursor"assert_and_click("screenshot_icon_showcursor", timeout => 5);these timeouts look a little low, there's no real reason to be so aggressive just to save a few seconds on a failure, I don't think.
The selection dialogue stays open, it is stable, it does not move when an icon is clicked, so it never fails if the needles match. I guess I used such a short timeout for some swiftness in creating the needles and when it started working, there were no reasons to put it back.
@ -0,0 +44,4 @@type_very_safely("exit\n");wait_still_screen(3);# Stop the recording using the custom keyboard shortcut we setwe're not actually using a keyboard shortcut, though?
@ -0,0 +47,4 @@# Stop the recording using the custom keyboard shortcut we set# and check if confirmation pop-up jumps out.assert_and_click("panel_icon_stoprecording");unless (check_screen("screenshot_screencast_recorded", timeout => 1)) {why the extremely short timeout?
With a longer one, it was impossible to catch the pop-up to create the needle properly. Now, when the needle exists, the timeout can be prolonged, however no need to make it any long, cause the pop-up disappears after a very short time.
Let's hope that when the needle stops working, the UI will be able to save the proper screen to recreate it easily without having to fiddle with the timeout again. I am setting it to 3 seconds (more does not make any sense to me).
@ -0,0 +61,4 @@# The video should have started. Wait a second, then stop it# and assert the screen.sleep(14);I agree with AI that this is fragile. Ideally, we should just be able to assert_screen and rely on openQA catching when the video is showing the screen we expect, rather than trying to pause at exactly the right time. This would mean we need to make sure the 'correct' state appears in the video for at least, say, 3 seconds, though. Can we do that, and drop the
sleepandsend_key "spc"here and just have theassert_screen?By observing the live logs in the past, I could see that it usually takes one second to perform the needle matching operation and since this is a video stream, I did not want to rely on needles checking a moving video. So I am stopping it to provide a stable screen to the needle matching mechanism. This has already worked well in the other video test we have in Navigation.
AI is wrong to see this as flaky, because it is not important what the video shows at the moment of stopping. The important thing is that the video is shown - and therefore exists. The needle could theoretically match any video content. On Silverblue, although being caught the right time, this required a new needle because the Workstation one did not work. I expect that as we cannot rely on one universal needle anyway and backups will be necessary, it's no big deal if the backup needles matched a different content of the existing video.
The argument to wait so long that the video stream finishes and closes is also weak - the video is like 40 seconds long and I am stopping it after 14 seconds. If the video is playing, it will still have at least 20 seconds to go.
I will do it the suggested way, if you insist, Adam, but I still think that checking a stopped video is better than trying to catch a moment in a playing (and therefore unstable) video stream. I do not trust openQA to be as quick and versatile - I have seen lot's of failed needles that should not have failed because they looked fine and they probably only had some pixels glitched. That risk is higher with a moving content, imho, because there might be some compression involved in the video saving process.
Fair enough. I guess we can go with this and reconsider if it causes any issues.
AI Code Review
📋 MR Summary
Adds a new openQA test module for GNOME screen recording on Wayland.
tests/applications/screen_recording/aaa_setup.pmto conditionally install 'Showtime' on Silverblue if it's missing.tests/applications/screen_recording/screen_recording.pmto exercise the GNOME screenshot/screencast UI.templates.fif.jsonto include thescreen_recordingtest in relevant profile groups.Detailed Code Review
The implementation aligns well with the previously discussed requirements and author responses. The
aaa_setup.pmeffectively usesget_var("CANNED")and avoids redundant variables when runningflatpak info. Thescreen_recording.pmfollows the agreed workflow, including the intentional pausing of the video before asserting the needle to ensure stability. No critical security, logic, or performance issues were identified.✅ Summary
screen_recording.pmto assert that the screencast file is non-empty via the command line (e.g., usingstatorls -l), as the UI check relies heavily on needle matching which might occasionally fail if video playback frames vary.🤖 AI Code Review | Generated with ai-code-review | Model:
gemini-3.1-pro-preview⚠️ AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.
No, we do want to make sure the video is actually correct, even at the cost of occasional flakiness.
One final thing I notice: we don't really need the
aaa_setupstructure here, right? Since there's only one test file anyway. Can't we just roll the two files into one big one and list it as POSTINSTALL in the template? This would also mean we don't waste time / resources making a snapshot at the end ofaaa_setup.Otherwise this looks good now, thanks!
f63fc173eafe44c24c80fe44c24c80dfe93eeebddfe93eeebde04b12b5c4e04b12b5c42923ec94f6I just did a little tweak: we have a precedent for still using
POSTINSTALL_PATHfor a single test file (thetourtest), so let's go with that. Will deploy to staging and give it one final test run, then merge. Thanks.2923ec94f6cedb0c28f1Hmm, the test run had several failures:
Some of them are because maximizing the terminal didn't work - that whole maximize path is annoyingly fragile. Some of them are because the thumbnail showed the desktop background, not the maximized terminal, I'm not sure why.
So...hmm. We can quit trying to match on the thumbnail and just match on the "Screencast from" text, I guess; we don't test thumbnail generation is working, but that's not the primary point of the test. For the terminal maximization, I guess we either don't maximize the terminal (but then maybe the video needles need to be updated for every new desktop background?), or try to make the maximization more robust?
cedb0c28f10e56e2878b0e56e2878b2dfd1c72482dfd1c72487a75c4a977So, I took a look on the maximize path of
menu_launch_type. I spotted one issue and one possible issue:I have fixed the above directly in the main branch as a separate commit. Now, the order is correct. First, application is started, start is confirmed, then maximized. Also, I decided to go with a default check so the
checkstart => 1parameter is no longer needed, but I am keeping the mechanism for compatibility and for a possible off-switch if someone needs it (checkstart => 0).The second issue I have solved with sending two kombos right after another which is quite cheap and the chances for a successful maximization should statistically double. At first, we send
alt-f10which switches between maximized and not maximized in both Gnome and KDE. Then, we wait a bit and sendsuper-uporsuper-pgupto maximize again which does never unmaximize anything, so either it does nothing or it maximizes the application. This will solve all errors when the kombo was not caught for any reason. It will however fail if the application does not have focus (which it should).I restarted the failures from your comment and all worked:
Also, I changed the needle so that it does not catch the thumbnail and it matches on "Screencast from"
I'm not sure about this, because I think some of the needles actually assume / check that the app has been maximized (i.e. they won't match when it's not yet maximized). I'm worried we'll start seeing some tests fail because of this?
Flipping the
checkstartdefault is also dangerous, we should at least have done an inventory of all callers to see if they're 'safe' for this change first.@adamwill wrote in #505 (comment):
It is possible, that an application will fail if
apps_run_$applicationis missing or not matching for the check, but I thought that many applications already have theirapps_run_as I have been consistently using menu_launch_type with checkstart anyway. Also I always tried to match them on application title which does not depend on the size of the window, so I do not expect massive failures. But, I can restart tests on staging and see how many actually do fail.I think that it'll be quicker than to go through all the apps_run_needles trying to match them with their test script counterparts.
UPDATE:
I have restarted KDE x86_64 tests and Workstation x86_64 tests (all of them) and a bunch of Workstation and KDE aarch64 tests (that I could identify as application tests) and as well as Silverblue tests in https://openqa.stg.fedoraproject.org/tests/overview?distri=fedora&version=44&build=Fedora-44-20260410.n.0&groupid=1 and did not spot a single failure because of faulty
menu_launch_type.I am convinced that the changes will not be a disaster and the changes make the function better for sure.
7a75c4a9773b0d50d1b63b0d50d1b6845ec8a9a9@ -0,0 +36,4 @@# Start the recordingsend_key("ctrl-alt-shift-r");assert_screen("screenshot_dashboard");do we actually need to assert the dashboard? can we not just ditch this (and the needle) and rely on matching the things we actually need to click, below?
845ec8a9a98d616a1badAside from the dashboard thing, this looks good. I tried it out in staging and every attempt passed, including when I edited the test to comment out the dashboard assertion. So I took the liberty of editing it out and repushing the PR. I'll merge after tests. Thanks!
AI Code Review
📋 MR Summary
This MR introduces a new openQA test module (
screen_recording) for testing GNOME screen recording on Wayland. It includes installing the default video player (Showtime) if missing, performing a screen recording session, and validating the resulting video file using nautilus and the video player.tests/applications/screen_recording/screen_recording.pm.needles/gnome/apps/for UI matching during the recording and playback process.templates.fif.jsonto include thescreen_recordingtest in the execution pipeline.Detailed Code Review
The implementation aligns well with the stated objectives and correctly incorporates the feedback discussed during the PR review. The use of
get_var("CANNED")for Silverblue/atomic detection and theif (script_run(...))pattern are properly implemented. The logic accurately tests the screenshot overlay functionality, manages the recording, and finally validates the output. The approach of playing the video and stopping it with 'spc' to reliably assert the playback frame matches the design decision to prevent fragile matching on moving video. Overall, the logic is sound and no critical defects were found.✅ Summary
🤖 AI Code Review | Generated with ai-code-review | Model:
gemini-3.1-pro-preview⚠️ AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.