Add GNOME screen recording test #505

Merged
adamwill merged 1 commit from newtest/screen_recording into main 2026-04-17 20:21:33 +00:00
Owner

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/.

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.

  • Key Changes:
    • Added needles for GNOME screenshot/recording UI elements.
    • Added screen_recording test in templates.fif.json.
    • Created tests/applications/screen_recording/screen_recording.pm to exercise the GNOME screenshot recording workflow.
  • Impact: templates.fif.json, tests/applications/screen_recording, needles/gnome/apps/screen_recording
  • Risk Level: 🟡 Medium - While a new test is low risk to the application itself, shell scripting errors in assertions might cause the test to fail or behave unpredictably if multiple screencast files exist.

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 .webm files 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.
  • Major [Bug]: The shell test [ -s /home/$user/Videos/Screencasts/*.webm ] will fail with 'too many arguments' if there are multiple .webm files in the directory.
    • Suggestion: Either clean up the directory before recording, or use a loop/find command to check the files.
assert_script_run("find /home/$user/Videos/Screencasts/ -name '*.webm' -type f -size +0c | grep -q .");
  • Major [Bug]: Similar to the -s check, stat -c%s /home/$user/Videos/Screencasts/*.webm will fail if the glob expands to multiple files.
    • Suggestion: Find the most recent file or check all files, or ensure the directory is clean prior to recording.
assert_script_run("find /home/$user/Videos/Screencasts/ -name '*.webm' -type f -size +10000c | grep -q .");
  • Minor [Logic]: The script uses $user in paths, which relies on get_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 configures gsettings as implied.
    • Suggestion: Verify that gsettings is actually being configured or update the comment. Clear the Screencasts directory at the start of the test.

Questions:

  • Are we guaranteed that the Screencasts directory is empty before this test runs?
  • Where is the gsettings configuration mentioned in the comments actually being executed?

Summary

  • Overall Assessment: The test logic is sound, but the final file size assertions are prone to shell globbing errors if multiple screencasts exist. Fixing these assertions is required for test stability.
  • Priority Issues:
    • Fix shell assertions for .webm files to handle multiple files correctly or clear the directory beforehand.
  • Minor Suggestions:
    • Clarify or implement the missing gsettings shortcut configuration mentioned in the script comments.
    • Add cleanup logic to remove the generated screencast after the test completes.

🤖 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.

## AI Code Review ### 📋 MR Summary Adds a new openQA test module for GNOME screen recording on Wayland. - **Key Changes:** - Added needles for GNOME screenshot/recording UI elements. - Added `screen_recording` test in `templates.fif.json`. - Created `tests/applications/screen_recording/screen_recording.pm` to exercise the GNOME screenshot recording workflow. - **Impact:** templates.fif.json, tests/applications/screen_recording, needles/gnome/apps/screen_recording - **Risk Level:** 🟡 Medium - While a new test is low risk to the application itself, shell scripting errors in assertions might cause the test to fail or behave unpredictably if multiple screencast files exist. ### 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 `.webm` files behind. Consider cleaning the directory before starting the recording or making the assertions robust against multiple files. #### 📂 File Reviews <details> <summary><strong>📄 `tests/applications/screen_recording/screen_recording.pm`</strong> - Implements the test logic for verifying GNOME screen recordings.</summary> - **Major** [Bug]: The shell test `[ -s /home/$user/Videos/Screencasts/*.webm ]` will fail with 'too many arguments' if there are multiple `.webm` files in the directory. - *Suggestion:* Either clean up the directory before recording, or use a loop/find command to check the files. ```` assert_script_run("find /home/$user/Videos/Screencasts/ -name '*.webm' -type f -size +0c | grep -q ."); ```` - **Major** [Bug]: Similar to the `-s` check, `stat -c%s /home/$user/Videos/Screencasts/*.webm` will fail if the glob expands to multiple files. - *Suggestion:* Find the most recent file or check all files, or ensure the directory is clean prior to recording. ```` assert_script_run("find /home/$user/Videos/Screencasts/ -name '*.webm' -type f -size +10000c | grep -q ."); ```` - **Minor** [Logic]: The script uses `$user` in paths, which relies on `get_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 configures `gsettings` as implied. - *Suggestion:* Verify that `gsettings` is actually being configured or update the comment. Clear the `Screencasts` directory at the start of the test. **Questions:** - ❓ Are we guaranteed that the `Screencasts` directory is empty before this test runs? - ❓ Where is the `gsettings` configuration mentioned in the comments actually being executed? </details> ### ✅ Summary - **Overall Assessment:** The test logic is sound, but the final file size assertions are prone to shell globbing errors if multiple screencasts exist. Fixing these assertions is required for test stability. - **Priority Issues:** - Fix shell assertions for `.webm` files to handle multiple files correctly or clear the directory beforehand. - **Minor Suggestions:** - Clarify or implement the missing `gsettings` shortcut configuration mentioned in the script comments. - Add cleanup logic to remove the generated screencast after the test completes. --- 🤖 **AI Code Review** | Generated with [ai-code-review](https://gitlab.com/redhat/edge/ci-cd/ai-code-review) | **Model:** `gemini-3.1-pro-preview` ⚠️ *AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.*
Author
Owner

Remark to the AI results:

  1. We expect that the Screencasts directory is always empty, because we run on a clean installation -> if it is not, we will probably hit some weird bug.
  2. We expect that the user directory structure has been fully created during the installation -> if not, we certainly want to know
  3. I removed the deterministic key as any key combo is as good as the original one and we want to know that the original one works.
Remark to the AI results: 1. We expect that the `Screencasts` directory is always empty, because we run on a clean installation -> if it is not, we will probably hit some weird bug. 2. We expect that the user directory structure has been fully created during the installation -> if not, we certainly want to know 3. I removed the deterministic key as any key combo is as good as the original one and we want to know that the original one works.
lruzicka force-pushed newtest/screen_recording from 85208fe325
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m29s
CI via Tox and perl / perl (pull_request) Successful in 3m36s
CI via Tox and perl / checkwiki (pull_request) Failing after 33s
AI Code Review / ai-review (pull_request_target) Successful in 27s
to 3c680041b4
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m43s
CI via Tox and perl / perl (pull_request) Successful in 3m36s
CI via Tox and perl / checkwiki (pull_request) Successful in 44s
2026-03-23 09:01:24 +00:00
Compare
adamwill force-pushed newtest/screen_recording from 3c680041b4
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m43s
CI via Tox and perl / perl (pull_request) Successful in 3m36s
CI via Tox and perl / checkwiki (pull_request) Successful in 44s
to ae345039ae
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m45s
CI via Tox and perl / perl (pull_request) Successful in 3m42s
CI via Tox and perl / checkwiki (pull_request) Failing after 34s
2026-03-31 21:12:31 +00:00
Compare
adamwill left a comment
Owner

Thanks, a few comments in line.

Thanks, a few comments in line.
@ -0,0 +11,4 @@
my $self = shift;
my $user = get_var("USER_LOGIN") // "test";
# Open a terminal to configure gsettings
Owner

we are not in fact doing anything with gsettings here.

we are not in fact doing anything with gsettings here.
@ -0,0 +14,4 @@
# Open a terminal to configure gsettings
menu_launch_type("terminal", checkstart => 1, maximize => 1);
# Wait for gsettings to apply
Owner

or here.

or here.
@ -0,0 +50,4 @@
sleep(2);
type_very_safely("pwd\n");
sleep(2);
Owner

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?

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?
Author
Owner

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.

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 ]");
}
Owner

can we do any better here? can we not play back the video and assert that we see something in the playback?

can we do any better here? can we not play back the video and assert that we see something in the playback?
Author
Owner

Dtto. I wanted to avoid creating needles. We can play the video and assert something from it.

Dtto. I wanted to avoid creating needles. We can play the video and assert something from it.
lruzicka force-pushed newtest/screen_recording from 1500e8393e
Some checks failed
CI via Tox and perl / tox (pull_request) Failing after 1m29s
CI via Tox and perl / perl (pull_request) Successful in 3m47s
CI via Tox and perl / checkwiki (pull_request) Failing after 33s
to 9518a48951
Some checks failed
CI via Tox and perl / tox (pull_request) Failing after 1m26s
CI via Tox and perl / perl (pull_request) Successful in 3m33s
CI via Tox and perl / checkwiki (pull_request) Failing after 33s
2026-04-01 11:47:52 +00:00
Compare
lruzicka force-pushed newtest/screen_recording from 9518a48951
Some checks failed
CI via Tox and perl / tox (pull_request) Failing after 1m26s
CI via Tox and perl / checkwiki (pull_request) Failing after 33s
CI via Tox and perl / perl (pull_request) Successful in 3m33s
to a7bb1b59b8
Some checks failed
CI via Tox and perl / tox (pull_request) Failing after 1m27s
CI via Tox and perl / perl (pull_request) Successful in 3m53s
CI via Tox and perl / checkwiki (pull_request) Failing after 35s
2026-04-01 12:15:29 +00:00
Compare
lruzicka force-pushed newtest/screen_recording from a7bb1b59b8
Some checks failed
CI via Tox and perl / tox (pull_request) Failing after 1m27s
CI via Tox and perl / perl (pull_request) Successful in 3m53s
CI via Tox and perl / checkwiki (pull_request) Failing after 35s
to c63572c238
Some checks failed
CI via Tox and perl / tox (pull_request) Failing after 1m26s
CI via Tox and perl / perl (pull_request) Has been cancelled
CI via Tox and perl / checkwiki (pull_request) Has been cancelled
2026-04-01 15:32:45 +00:00
Compare
lruzicka force-pushed newtest/screen_recording from bbf02f78a2
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m27s
CI via Tox and perl / perl (pull_request) Successful in 3m35s
CI via Tox and perl / checkwiki (pull_request) Failing after 31s
to 2274f9f873
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m24s
CI via Tox and perl / perl (pull_request) Successful in 3m27s
CI via Tox and perl / checkwiki (pull_request) Successful in 33s
AI Code Review / ai-review (pull_request_target) Successful in 34s
2026-04-01 15:49:58 +00:00
Compare
Author
Owner
  • Fixed the comments
  • Deleted majority of the commands, left two short ones, the recorded video is long enough to test.
  • I am now playing the Video instead.

Passes:
https://openqa.stg.fedoraproject.org/tests/6186892#step/screen_recording/13

* Fixed the comments * Deleted majority of the commands, left two short ones, the recorded video is long enough to test. * I am now playing the Video instead. Passes: https://openqa.stg.fedoraproject.org/tests/6186892#step/screen_recording/13
Author
Owner

Don't know why checkwiki is failing. The basic_networking_* has been already merged.

Don't know why `checkwiki` is failing. The `basic_networking_*` has been already merged.
Owner

You didn't rebase the fedora_openqa newtest/screen_recording branch on the basic_networking changes. I'll fix it.

You didn't rebase the fedora_openqa `newtest/screen_recording` branch 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.

  • Key Changes:
    • Adds visual match JSON files (needles) for screen recording UI elements.
    • Registers the new screen_recording test in templates.fif.json.
    • Implements the test logic in screen_recording.pm which records terminal activity and verifies playback.
  • Impact: tests/applications/screen_recording, needles/gnome, templates.fif.json
  • Risk Level: 🟢 Low - This PR adds a new standalone UI test without modifying existing core functionality or production code.

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 gsettings imports. 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.
  • Minor [Style]: The variable $user is declared and initialized but never used in the script. This can be safely removed to keep the code clean.
    • Suggestion: Remove the unused $user variable.
sub run {
    my $self = shift;

    # Open a terminal to have some content to record
    menu_launch_type("terminal", checkstart => 1, maximize => 1);
    wait_still_screen(2);

Summary

  • Overall Assessment: The changes successfully implement the agreed-upon test strategy with playback verification. Only minor code cleanup is suggested.
  • Minor Suggestions:
    • Remove the unused $user variable in screen_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.

## AI Code Review ### 📋 MR Summary Introduces a new openQA test module to verify GNOME screen recording functionality on Wayland. - **Key Changes:** - Adds visual match JSON files (needles) for screen recording UI elements. - Registers the new `screen_recording` test in `templates.fif.json`. - Implements the test logic in `screen_recording.pm` which records terminal activity and verifies playback. - **Impact:** tests/applications/screen_recording, needles/gnome, templates.fif.json - **Risk Level:** 🟢 Low - This PR adds a new standalone UI test without modifying existing core functionality or production code. ### 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 `gsettings` imports. The code is clean and the test steps flow logically. I found one minor unused variable that can be cleaned up. #### 📂 File Reviews <details> <summary><strong>📄 `tests/applications/screen_recording/screen_recording.pm`</strong> - Implements the core openQA test logic for screen recording and playback verification.</summary> - **Minor** [Style]: The variable `$user` is declared and initialized but never used in the script. This can be safely removed to keep the code clean. - *Suggestion:* Remove the unused `$user` variable. ```` sub run { my $self = shift; # Open a terminal to have some content to record menu_launch_type("terminal", checkstart => 1, maximize => 1); wait_still_screen(2); ```` </details> ### ✅ Summary - **Overall Assessment:** The changes successfully implement the agreed-upon test strategy with playback verification. Only minor code cleanup is suggested. - **Minor Suggestions:** - Remove the unused `$user` variable in `screen_recording.pm`. --- 🤖 **AI Code Review** | Generated with [ai-code-review](https://gitlab.com/redhat/edge/ci-cd/ai-code-review) | **Model:** `gemini-3.1-pro-preview` ⚠️ *AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.*
adamwill force-pushed newtest/screen_recording from 2274f9f873
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m24s
CI via Tox and perl / perl (pull_request) Successful in 3m27s
CI via Tox and perl / checkwiki (pull_request) Successful in 33s
AI Code Review / ai-review (pull_request_target) Successful in 34s
to 88bd61b782
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m36s
CI via Tox and perl / perl (pull_request) Successful in 3m42s
CI via Tox and perl / checkwiki (pull_request) Successful in 31s
2026-04-02 04:44:48 +00:00
Compare
Owner

From 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.

From 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.
Author
Owner

@adamwill wrote in #505 (comment):

From 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.

Hmm, I am trying with Fedora-Silverblue-ostree-x86_64-44-20260402.n.0.iso and Videos are installed by default and it works wonders.

@adamwill wrote in https://forge.fedoraproject.org/quality/os-autoinst-distri-fedora/pulls/505#issuecomment-606079: > From 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. Hmm, I am trying with Fedora-Silverblue-ostree-x86_64-44-20260402.n.0.iso and `Videos` are installed by default and it works wonders.
Owner

Huh. Well, that's not what openQA saw.

Huh. Well, that's [not what openQA saw](https://openqa.stg.fedoraproject.org/tests/6191497#step/screen_recording/13).
adamwill force-pushed newtest/screen_recording from a408f1fa8d
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m36s
CI via Tox and perl / perl (pull_request) Successful in 3m33s
CI via Tox and perl / checkwiki (pull_request) Successful in 36s
to deccb2cf62
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m37s
CI via Tox and perl / perl (pull_request) Successful in 3m31s
CI via Tox and perl / checkwiki (pull_request) Successful in 32s
2026-04-02 19:09:53 +00:00
Compare
Author
Owner

@adamwill wrote in #505 (comment):

Huh. Well, that's not what openQA saw.

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.

@adamwill wrote in https://forge.fedoraproject.org/quality/os-autoinst-distri-fedora/pulls/505#issuecomment-606134: > Huh. Well, that's [not what openQA saw](https://openqa.stg.fedoraproject.org/tests/6191497#step/screen_recording/13). 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.
lruzicka force-pushed newtest/screen_recording from e2c1ff3af8
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m23s
CI via Tox and perl / perl (pull_request) Failing after 3m31s
CI via Tox and perl / checkwiki (pull_request) Successful in 34s
to 62e269358b
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m25s
CI via Tox and perl / perl (pull_request) Successful in 3m34s
CI via Tox and perl / checkwiki (pull_request) Successful in 34s
2026-04-07 14:14:19 +00:00
Compare
lruzicka force-pushed newtest/screen_recording from 62e269358b
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m25s
CI via Tox and perl / perl (pull_request) Successful in 3m34s
CI via Tox and perl / checkwiki (pull_request) Successful in 34s
to d8e2316262
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m24s
CI via Tox and perl / perl (pull_request) Successful in 3m28s
CI via Tox and perl / checkwiki (pull_request) Successful in 31s
2026-04-07 14:28:25 +00:00
Compare
lruzicka force-pushed newtest/screen_recording from 590f1bddbd
Some checks failed
CI via Tox and perl / tox (pull_request) Has been cancelled
CI via Tox and perl / perl (pull_request) Has been cancelled
CI via Tox and perl / checkwiki (pull_request) Has been cancelled
to 5728c459d1
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m24s
CI via Tox and perl / perl (pull_request) Successful in 3m29s
CI via Tox and perl / checkwiki (pull_request) Successful in 33s
AI Code Review / ai-review (pull_request_target) Successful in 33s
2026-04-07 16:11:52 +00:00
Compare
Author
Owner
https://openqa.stg.fedoraproject.org/tests/6208721#details
Owner

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?

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.

> 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? 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.

  • Key Changes:
    • Added tests/applications/screen_recording/aaa_setup.pm to conditionally install Showtime on Silverblue variants.
    • Added tests/applications/screen_recording/screen_recording.pm to perform the actual GNOME screen recording workflow and verification.
    • Added necessary needle JSON files to support GUI assertions during the test.
    • Updated templates.fif.json to register the new screen_recording test in the relevant profile groups.
  • Impact: tests/applications/screen_recording, templates.fif.json, needles/gnome
  • Risk Level: 🟡 Medium - The test interacts directly with GUI elements which are notoriously flaky in UI testing, and it handles environment-specific setup (Silverblue flatpak installation) which requires solid error handling. Overall safe for the application, but could lead to unstable tests if not robust.

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.pm script 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, the script_run command for flatpak info returns a status code, not a boolean/string representation of success, which might invert the expected logic if not careful. In screen_recording.pm, the verification step uses sleep(14) followed by send_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.
  • Major [Logic]: The return value of script_run is an integer exit code (0 for success, non-zero for failure). The condition if ($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_missing and 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.
    • Suggestion: Explicitly check against the failure exit code or use a more descriptive approach to ensure clarity.
my $ret = script_run("flatpak info org.gnome.Showtime");
if ($ret != 0) {
    record_soft_failure("Showtime not installed by default.");
    assert_script_run("flatpak install -y org.gnome.Showtime", timeout => 600);
}
📄 `tests/applications/screen_recording/screen_recording.pm` - Performs the screen recording test workflow and validates the output by playing the video.
  • Major [Testing]: Hardcoded 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.
    • Suggestion: Reduce the sleep duration to a value smaller than the recording length, or better yet, use assert_screen to wait for the video player UI to become active before checking playback status.
assert_screen("screenshot_video_playing", timeout => 10);
# If you need to stop it to check a specific frame:
send_key("spc");
assert_screen("screenshot_video_paused");
  • Minor [Testing]: If the 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.
    • Suggestion: Ensure the test gracefully aborts or handles the missing file scenario if the recording fails, rather than letting a subsequent click assertion crash the test blindly.

Summary

  • Overall Assessment: The logic is mostly solid and handles the requested scenarios, but relies on brittle hardcoded sleep values for test synchronization and has a confusing exit code check in the setup script. These should be addressed to ensure test stability.
  • Priority Issues:
    • Fix the hardcoded sleep(14) in screen_recording.pm which is longer than the actual recorded video, risking test flakiness.
    • Clarify the exit code logic for script_run in aaa_setup.pm to avoid confusion.
  • Minor Suggestions:
    • Add explicit handling/assertion for the expected video file actually existing in the directory before attempting to double click it.

🤖 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.

## 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. - **Key Changes:** - Added `tests/applications/screen_recording/aaa_setup.pm` to conditionally install Showtime on Silverblue variants. - Added `tests/applications/screen_recording/screen_recording.pm` to perform the actual GNOME screen recording workflow and verification. - Added necessary needle JSON files to support GUI assertions during the test. - Updated `templates.fif.json` to register the new `screen_recording` test in the relevant profile groups. - **Impact:** tests/applications/screen_recording, templates.fif.json, needles/gnome - **Risk Level:** 🟡 Medium - The test interacts directly with GUI elements which are notoriously flaky in UI testing, and it handles environment-specific setup (Silverblue flatpak installation) which requires solid error handling. Overall safe for the application, but could lead to unstable tests if not robust. ### 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.pm` script 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`, the `script_run` command for flatpak info returns a status code, not a boolean/string representation of success, which might invert the expected logic if not careful. In `screen_recording.pm`, the verification step uses `sleep(14)` followed by `send_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 <details> <summary><strong>📄 `tests/applications/screen_recording/aaa_setup.pm`</strong> - Sets up the test environment by installing the Showtime video player on Silverblue if it's missing.</summary> - **Major** [Logic]: The return value of `script_run` is an integer exit code (0 for success, non-zero for failure). The condition `if ($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_missing` and 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. - *Suggestion:* Explicitly check against the failure exit code or use a more descriptive approach to ensure clarity. ```` my $ret = script_run("flatpak info org.gnome.Showtime"); if ($ret != 0) { record_soft_failure("Showtime not installed by default."); assert_script_run("flatpak install -y org.gnome.Showtime", timeout => 600); } ```` </details> <details> <summary><strong>📄 `tests/applications/screen_recording/screen_recording.pm`</strong> - Performs the screen recording test workflow and validates the output by playing the video.</summary> - **Major** [Testing]: Hardcoded `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. - *Suggestion:* Reduce the sleep duration to a value smaller than the recording length, or better yet, use `assert_screen` to wait for the video player UI to become active before checking playback status. ```` assert_screen("screenshot_video_playing", timeout => 10); # If you need to stop it to check a specific frame: send_key("spc"); assert_screen("screenshot_video_paused"); ```` - **Minor** [Testing]: If the `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. - *Suggestion:* Ensure the test gracefully aborts or handles the missing file scenario if the recording fails, rather than letting a subsequent click assertion crash the test blindly. </details> ### ✅ Summary - **Overall Assessment:** The logic is mostly solid and handles the requested scenarios, but relies on brittle hardcoded sleep values for test synchronization and has a confusing exit code check in the setup script. These should be addressed to ensure test stability. - **Priority Issues:** - Fix the hardcoded `sleep(14)` in `screen_recording.pm` which is longer than the actual recorded video, risking test flakiness. - Clarify the exit code logic for `script_run` in `aaa_setup.pm` to avoid confusion. - **Minor Suggestions:** - Add explicit handling/assertion for the expected video file actually existing in the directory before attempting to double click it. --- 🤖 **AI Code Review** | Generated with [ai-code-review](https://gitlab.com/redhat/edge/ci-cd/ai-code-review) | **Model:** `gemini-3.1-pro-preview` ⚠️ *AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.*
adamwill left a comment
Owner

Thanks for the revision, but several more notes in line.

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") {
Owner

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.)

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.)
lruzicka marked this conversation as resolved
@ -0,0 +17,4 @@
$self->root_console(tty => 3);
my $app_missing = script_run("flatpak info org.gnome.Showtime");
if ($app_missing) {
Owner

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.

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.
lruzicka marked this conversation as resolved
@ -0,0 +24,4 @@
desktop_vt();
}
else {
record_info("Silverblue check", "This test does not run on Silverblue, nothing to do.");
Owner

is this really necessary?

is this really necessary?
Author
Owner

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.

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.
Owner

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.

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.
lruzicka marked this conversation as resolved
@ -0,0 +24,4 @@
assert_and_click("screenshot_icon_recordscreen", timeout => 5);
# Select "Show Cursor"
assert_and_click("screenshot_icon_showcursor", timeout => 5);
Owner

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.

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.
Author
Owner

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.

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.
lruzicka marked this conversation as resolved
@ -0,0 +44,4 @@
type_very_safely("exit\n");
wait_still_screen(3);
# Stop the recording using the custom keyboard shortcut we set
Owner

we're not actually using a keyboard shortcut, though?

we're not actually using a keyboard shortcut, though?
lruzicka marked this conversation as resolved
@ -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)) {
Owner

why the extremely short timeout?

why the extremely short timeout?
Author
Owner

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).

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).
lruzicka marked this conversation as resolved
@ -0,0 +61,4 @@
# The video should have started. Wait a second, then stop it
# and assert the screen.
sleep(14);
Owner

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 sleep and send_key "spc" here and just have the assert_screen?

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 `sleep` and `send_key "spc"` here and just have the `assert_screen`?
Author
Owner

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.

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.
Owner

Fair enough. I guess we can go with this and reconsider if it causes any issues.

Fair enough. I guess we can go with this and reconsider if it causes any issues.
adamwill marked this conversation as resolved

AI Code Review

📋 MR Summary

Adds a new openQA test module for GNOME screen recording on Wayland.

  • Key Changes:
    • Created tests/applications/screen_recording/aaa_setup.pm to conditionally install 'Showtime' on Silverblue if it's missing.
    • Created tests/applications/screen_recording/screen_recording.pm to exercise the GNOME screenshot/screencast UI.
    • Added multiple new needle JSON files to support the UI workflow assertions.
    • Configured templates.fif.json to include the screen_recording test in relevant profile groups.
  • Impact: tests/applications/screen_recording, needles/gnome, templates.fif.json
  • Risk Level: 🟢 Low - These changes introduce test automation and setup scripts without affecting core project logic or product code. The implementations follow recent review agreements.

Detailed Code Review

The implementation aligns well with the previously discussed requirements and author responses. The aaa_setup.pm effectively uses get_var("CANNED") and avoids redundant variables when running flatpak info. The screen_recording.pm follows 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

  • Overall Assessment: The changes look solid and address previous review feedback. No critical issues found.
  • Minor Suggestions:
    • Consider adding a fallback validation in screen_recording.pm to assert that the screencast file is non-empty via the command line (e.g., using stat or ls -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.

## AI Code Review ### 📋 MR Summary Adds a new openQA test module for GNOME screen recording on Wayland. - **Key Changes:** - Created `tests/applications/screen_recording/aaa_setup.pm` to conditionally install 'Showtime' on Silverblue if it's missing. - Created `tests/applications/screen_recording/screen_recording.pm` to exercise the GNOME screenshot/screencast UI. - Added multiple new needle JSON files to support the UI workflow assertions. - Configured `templates.fif.json` to include the `screen_recording` test in relevant profile groups. - **Impact:** tests/applications/screen_recording, needles/gnome, templates.fif.json - **Risk Level:** 🟢 Low - These changes introduce test automation and setup scripts without affecting core project logic or product code. The implementations follow recent review agreements. ### Detailed Code Review The implementation aligns well with the previously discussed requirements and author responses. The `aaa_setup.pm` effectively uses `get_var("CANNED")` and avoids redundant variables when running `flatpak info`. The `screen_recording.pm` follows 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 - **Overall Assessment:** The changes look solid and address previous review feedback. No critical issues found. - **Minor Suggestions:** - Consider adding a fallback validation in `screen_recording.pm` to assert that the screencast file is non-empty via the command line (e.g., using `stat` or `ls -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](https://gitlab.com/redhat/edge/ci-cd/ai-code-review) | **Model:** `gemini-3.1-pro-preview` ⚠️ *AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.*
Owner

Consider adding a fallback validation in screen_recording.pm to assert that the screencast file is non-empty via the command line (e.g., using stat or ls -l), as the UI check relies heavily on needle matching which might occasionally fail if video playback frames vary.

No, we do want to make sure the video is actually correct, even at the cost of occasional flakiness.

> Consider adding a fallback validation in screen_recording.pm to assert that the screencast file is non-empty via the command line (e.g., using stat or ls -l), as the UI check relies heavily on needle matching which might occasionally fail if video playback frames vary. No, we do want to make sure the video is actually correct, even at the cost of occasional flakiness.
Owner

One final thing I notice: we don't really need the aaa_setup structure 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 of aaa_setup.

Otherwise this looks good now, thanks!

One final thing I notice: we don't really need the `aaa_setup` structure 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 of `aaa_setup`. Otherwise this looks good now, thanks!
lruzicka force-pushed newtest/screen_recording from f63fc173ea
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m24s
CI via Tox and perl / perl (pull_request) Has been cancelled
CI via Tox and perl / checkwiki (pull_request) Has been cancelled
to fe44c24c80
All checks were successful
CI via Tox and perl / perl (pull_request) Successful in 3m30s
CI via Tox and perl / checkwiki (pull_request) Successful in 33s
CI via Tox and perl / tox (pull_request) Successful in 1m25s
2026-04-09 08:33:11 +00:00
Compare
lruzicka force-pushed newtest/screen_recording from fe44c24c80
All checks were successful
CI via Tox and perl / perl (pull_request) Successful in 3m30s
CI via Tox and perl / checkwiki (pull_request) Successful in 33s
CI via Tox and perl / tox (pull_request) Successful in 1m25s
to dfe93eeebd
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m26s
CI via Tox and perl / perl (pull_request) Successful in 3m30s
CI via Tox and perl / checkwiki (pull_request) Successful in 31s
2026-04-09 10:24:32 +00:00
Compare
lruzicka force-pushed newtest/screen_recording from dfe93eeebd
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m26s
CI via Tox and perl / perl (pull_request) Successful in 3m30s
CI via Tox and perl / checkwiki (pull_request) Successful in 31s
to e04b12b5c4
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m23s
CI via Tox and perl / perl (pull_request) Successful in 3m28s
CI via Tox and perl / checkwiki (pull_request) Successful in 31s
2026-04-09 10:52:53 +00:00
Compare
adamwill force-pushed newtest/screen_recording from e04b12b5c4
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m23s
CI via Tox and perl / perl (pull_request) Successful in 3m28s
CI via Tox and perl / checkwiki (pull_request) Successful in 31s
to 2923ec94f6
All checks were successful
CI via Tox and perl / perl (pull_request) Successful in 3m33s
CI via Tox and perl / checkwiki (pull_request) Successful in 33s
CI via Tox and perl / tox (pull_request) Successful in 1m23s
2026-04-09 18:14:44 +00:00
Compare
Owner

I just did a little tweak: we have a precedent for still using POSTINSTALL_PATH for a single test file (the tour test), so let's go with that. Will deploy to staging and give it one final test run, then merge. Thanks.

I just did a little tweak: we have a precedent for still using `POSTINSTALL_PATH` for a single test file (the `tour` test), so let's go with that. Will deploy to staging and give it one final test run, then merge. Thanks.
adamwill force-pushed newtest/screen_recording from 2923ec94f6
All checks were successful
CI via Tox and perl / perl (pull_request) Successful in 3m33s
CI via Tox and perl / checkwiki (pull_request) Successful in 33s
CI via Tox and perl / tox (pull_request) Successful in 1m23s
to cedb0c28f1
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m24s
CI via Tox and perl / checkwiki (pull_request) Successful in 32s
CI via Tox and perl / perl (pull_request) Successful in 3m33s
2026-04-09 18:34:53 +00:00
Compare
Owner

Hmm, 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?

Hmm, the test run had several failures: * https://openqa.stg.fedoraproject.org/tests/6222893 * https://openqa.stg.fedoraproject.org/tests/6222472 * https://openqa.stg.fedoraproject.org/tests/6222572 * https://openqa.stg.fedoraproject.org/tests/6222234 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?
lruzicka force-pushed newtest/screen_recording from cedb0c28f1
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m24s
CI via Tox and perl / checkwiki (pull_request) Successful in 32s
CI via Tox and perl / perl (pull_request) Successful in 3m33s
to 0e56e2878b
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m38s
CI via Tox and perl / perl (pull_request) Has been cancelled
CI via Tox and perl / checkwiki (pull_request) Has been cancelled
2026-04-10 10:55:38 +00:00
Compare
lruzicka force-pushed newtest/screen_recording from 0e56e2878b
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m38s
CI via Tox and perl / perl (pull_request) Has been cancelled
CI via Tox and perl / checkwiki (pull_request) Has been cancelled
to 2dfd1c7248
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m33s
CI via Tox and perl / checkwiki (pull_request) Successful in 34s
CI via Tox and perl / perl (pull_request) Successful in 3m30s
2026-04-10 10:59:40 +00:00
Compare
lruzicka force-pushed newtest/screen_recording from 2dfd1c7248
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m33s
CI via Tox and perl / checkwiki (pull_request) Successful in 34s
CI via Tox and perl / perl (pull_request) Successful in 3m30s
to 7a75c4a977
All checks were successful
CI via Tox and perl / perl (pull_request) Successful in 3m30s
CI via Tox and perl / tox (pull_request) Successful in 1m28s
CI via Tox and perl / checkwiki (pull_request) Successful in 34s
2026-04-10 11:15:44 +00:00
Compare
Author
Owner

So, I took a look on the maximize path of menu_launch_type. I spotted one issue and one possible issue:

  1. Issue: We maximized the application and then checked that it successfully started. That order is wrong. First, we must check if the application starts and when it does, we can maximize it and it should work per se.
  2. Possible issue: We do not needlecheck if the application has been maximized because that would require at least two to a number of new needles and we do not want it.

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 => 1 parameter 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-f10 which switches between maximized and not maximized in both Gnome and KDE. Then, we wait a bit and send super-up or super-pgup to 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"

So, I took a look on the _maximize path_ of `menu_launch_type`. I spotted one issue and one possible issue: 1. Issue: We maximized the application and then checked that it successfully started. That order is wrong. First, we must check if the application starts and when it does, we can maximize it and it should work per se. 2. Possible issue: We do not needlecheck if the application has been maximized because that would require at least two to a number of new needles and we do not want it. 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 => 1` parameter 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-f10` which switches between maximized and not maximized in both Gnome and KDE. Then, we wait a bit and send `super-up` or `super-pgup` to 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: * https://openqa.stg.fedoraproject.org/tests/6227170 * https://openqa.stg.fedoraproject.org/tests/6227171 * https://openqa.stg.fedoraproject.org/tests/6227176 * https://openqa.stg.fedoraproject.org/tests/6227175 Also, I changed the needle so that it does not catch the thumbnail and it matches on "Screencast from"
Owner
  • Issue: We maximized the application and then checked that it successfully started. That order is wrong. First, we must check if the application starts and when it does, we can maximize it and it should work per se.

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 checkstart default is also dangerous, we should at least have done an inventory of all callers to see if they're 'safe' for this change first.

> * Issue: We maximized the application and then checked that it successfully started. That order is wrong. First, we must check if the application starts and when it does, we can maximize it and it should work per se. 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 `checkstart` default is also dangerous, we should at least have done an inventory of all callers to see if they're 'safe' for this change first.
Author
Owner

@adamwill wrote in #505 (comment):

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 checkstart default is also dangerous, we should at least have done an inventory of all callers to see if they're 'safe' for this change first.

It is possible, that an application will fail if apps_run_$application is missing or not matching for the check, but I thought that many applications already have their apps_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.

@adamwill wrote in https://forge.fedoraproject.org/quality/os-autoinst-distri-fedora/pulls/505#issuecomment-613334: > 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 `checkstart` default is also dangerous, we should at least have done an inventory of all callers to see if they're 'safe' for this change first. It is possible, that an application will fail if `apps_run_$application` is missing or not matching for the check, but I thought that many applications already have their `apps_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.
lruzicka force-pushed newtest/screen_recording from 7a75c4a977
All checks were successful
CI via Tox and perl / perl (pull_request) Successful in 3m30s
CI via Tox and perl / tox (pull_request) Successful in 1m28s
CI via Tox and perl / checkwiki (pull_request) Successful in 34s
to 3b0d50d1b6
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m23s
CI via Tox and perl / perl (pull_request) Successful in 3m30s
CI via Tox and perl / checkwiki (pull_request) Failing after 36s
2026-04-14 12:12:46 +00:00
Compare
adamwill force-pushed newtest/screen_recording from 3b0d50d1b6
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m23s
CI via Tox and perl / perl (pull_request) Successful in 3m30s
CI via Tox and perl / checkwiki (pull_request) Failing after 36s
to 845ec8a9a9
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m23s
CI via Tox and perl / perl (pull_request) Successful in 3m35s
CI via Tox and perl / checkwiki (pull_request) Successful in 34s
2026-04-17 19:17:31 +00:00
Compare
@ -0,0 +36,4 @@
# Start the recording
send_key("ctrl-alt-shift-r");
assert_screen("screenshot_dashboard");
Owner

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?

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?
adamwill force-pushed newtest/screen_recording from 845ec8a9a9
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m23s
CI via Tox and perl / perl (pull_request) Successful in 3m35s
CI via Tox and perl / checkwiki (pull_request) Successful in 34s
to 8d616a1bad
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m28s
CI via Tox and perl / perl (pull_request) Successful in 3m41s
CI via Tox and perl / checkwiki (pull_request) Successful in 32s
AI Code Review / ai-review (pull_request_target) Successful in 25s
2026-04-17 20:11:01 +00:00
Compare
Owner

Aside 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!

Aside 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.

  • Key Changes:
    • Added a new openQA test file tests/applications/screen_recording/screen_recording.pm.
    • Added multiple needle files in needles/gnome/apps/ for UI matching during the recording and playback process.
    • Updated templates.fif.json to include the screen_recording test in the execution pipeline.
  • Impact: tests/applications/screen_recording, needles/gnome, templates.fif.json
  • Risk Level: 🟢 Low - The changes only add a test module and associated test assets (needles) without modifying any application or system source code. Impact is strictly isolated to test execution.

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 the if (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

  • Overall Assessment: The code review is complete and no critical bugs, security vulnerabilities, or logic errors were identified. The implementation follows the agreed-upon project patterns.

🤖 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.

## 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. - **Key Changes:** - Added a new openQA test file `tests/applications/screen_recording/screen_recording.pm`. - Added multiple needle files in `needles/gnome/apps/` for UI matching during the recording and playback process. - Updated `templates.fif.json` to include the `screen_recording` test in the execution pipeline. - **Impact:** tests/applications/screen_recording, needles/gnome, templates.fif.json - **Risk Level:** 🟢 Low - The changes only add a test module and associated test assets (needles) without modifying any application or system source code. Impact is strictly isolated to test execution. ### 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 the `if (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 - **Overall Assessment:** The code review is complete and no critical bugs, security vulnerabilities, or logic errors were identified. The implementation follows the agreed-upon project patterns. --- 🤖 **AI Code Review** | Generated with [ai-code-review](https://gitlab.com/redhat/edge/ci-cd/ai-code-review) | **Model:** `gemini-3.1-pro-preview` ⚠️ *AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.*
adamwill deleted branch newtest/screen_recording 2026-04-17 20:21:33 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
quality/os-autoinst-distri-fedora!505
No description provided.