Add GNOME Initial Setup post-install test #517

Merged
adamwill merged 25 commits from newtest/gnome-initial-setup into main 2026-05-11 23:55:07 +00:00
Owner

Introduce a new GNOME Initial Setup test suite that resets the existing
user state, runs through the GIS wizard, and verifies that the selected
settings were applied correctly after first login.

The new test covers:

  • language selection
  • keyboard layout selection
  • privacy defaults and toggles
  • timezone selection
  • third-party repository enablement
  • user creation and password setup

Add the required GIS needles, define a new gnome_initial_setup test
scenario in templates.fif.json

Fixes: #506

Introduce a new GNOME Initial Setup test suite that resets the existing user state, runs through the GIS wizard, and verifies that the selected settings were applied correctly after first login. The new test covers: - language selection - keyboard layout selection - privacy defaults and toggles - timezone selection - third-party repository enablement - user creation and password setup Add the required GIS needles, define a new gnome_initial_setup test scenario in templates.fif.json Fixes: https://forge.fedoraproject.org/quality/os-autoinst-distri-fedora/issues/506
Add GNOME Initial Setup post-install test
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m25s
CI via Tox and perl / perl (pull_request) Successful in 3m27s
CI via Tox and perl / checkwiki (pull_request) Failing after 38s
AI Code Review / ai-review (pull_request_target) Successful in 28s
1bba2ea808
Introduce a new GNOME Initial Setup test suite that resets the existing
user state, runs through the GIS wizard, and verifies that the selected
settings were applied correctly after first login.

The new test covers:
- language selection
- keyboard layout selection
- privacy defaults and toggles
- timezone selection
- third-party repository enablement
- user creation and password setup

Add the required GIS needles, define a new gnome_initial_setup test
scenario in templates.fif.json

Fixes: #506

AI Code Review

📋 MR Summary

This MR introduces a new GNOME Initial Setup test suite that resets the existing user state, runs through the GIS wizard, and verifies that the selected settings were applied correctly after the first login.

  • Key Changes:
    • Adds check-needles.py entries and multiple JSON needles for GNOME Initial Setup UI interactions.
    • Introduces templates.fif.json entries for gnome_initial_setup and screen_recording tests, and alters QEMURAM.
    • Creates aaa_setup.pm to clear current user state and reboot the system to trigger the setup wizard.
    • Creates gnome_initial_setup.pm to drive the initial setup wizard interaction programmatically.
    • Creates post_scriptum.pm to assert that settings (locale, timezone, privacy, repos, user) were successfully applied.
  • Impact: needles/gnome/apps/gnome-initial-setup, tests/applications/gnome-initial-setup, check-needles.py, templates.fif.json
  • Risk Level: 🟡 Medium - While mostly adding new tests, it drops the default QEMURAM from 5120 to 4096 in templates.fif.json, which could have downstream effects on performance or stability of other test suites relying on this configuration.

Detailed Code Review

The MR introduces a comprehensive set of tests to cover the GNOME Initial Setup. The code is well-structured and broken down into preparation, interaction, and validation stages. However, modifying QEMURAM in templates.fif.json might unintentionally impact other tests and its rationale is not explained in the commit message. There is also a missing input validation around the serial console setup, and hardcoding layout adjustments for typing loadkeys us could be flaky if the previous layout state isn't perfectly predictable.

📂 File Reviews

📄 `templates.fif.json` - Adds new test profiles and reduces QEMURAM.
  • Major [Performance]: QEMURAM is reduced from 5120 to 4096. This change is not mentioned in the commit message and could impact the performance or stability of other test runs.
    • Suggestion: Revert this change if it was unintentional, or document the rationale in the commit message.
-"QEMURAM": 4096,
+"QEMURAM": 5120,

Questions:

  • Was the QEMURAM change intentional? If so, does it need to be tested against other memory-heavy test suites?
📄 `tests/applications/gnome-initial-setup/aaa_setup.pm` - Sets up the environment by deleting the current user and rebooting.
  • Minor [Logic]: Deleting a user while processes might still be running in the background could lead to incomplete removal.
    • Suggestion: Ensure all user processes are killed before removing the user, e.g., using loginctl kill-user.
assert_script_run("loginctl kill-user $username");
assert_script_run("userdel -r $username");
📄 `tests/applications/gnome-initial-setup/post_scriptum.pm` - Validates system settings after the initial setup completes.
  • Major [Logic]: Entering loadkezs us assumes the exact layout mapped 'y' to 'z'. If the keyboard layout is not exactly what is expected (e.g., QWERTY instead of QWERTZ), this command will fail to execute correctly.
    • Suggestion: Use testapi functions that do not rely on raw typing strings that are dependent on layout assumptions, or guarantee layout state before typing.
null
  • Major [Security]: Passing user variables via bash evaluation in run_as_user can lead to command injection if $user or $cmd contains malicious characters. While it's a test environment, proper escaping should be used.
    • Suggestion: Ensure $cmd and $user are properly sanitized or passed securely without shell evaluation risks.
null

Summary

  • Overall Assessment: The test additions look robust, but a potentially breaking change to QEMURAM and some brittle keyboard layout workarounds require attention before merging.
  • Priority Issues:
    • Revert or justify the change to QEMURAM in templates.fif.json.
    • Fix the brittle layout assumption (loadkezs us) in post_scriptum.pm to prevent test flakiness.
  • Minor Suggestions:
    • Sanitize inputs in run_as_user wrapper to prevent command injection risks.
    • Kill user processes explicitly before running userdel -r to avoid failures due to locked files.

🤖 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 GNOME Initial Setup test suite that resets the existing user state, runs through the GIS wizard, and verifies that the selected settings were applied correctly after the first login. - **Key Changes:** - Adds `check-needles.py` entries and multiple JSON needles for GNOME Initial Setup UI interactions. - Introduces `templates.fif.json` entries for `gnome_initial_setup` and `screen_recording` tests, and alters QEMURAM. - Creates `aaa_setup.pm` to clear current user state and reboot the system to trigger the setup wizard. - Creates `gnome_initial_setup.pm` to drive the initial setup wizard interaction programmatically. - Creates `post_scriptum.pm` to assert that settings (locale, timezone, privacy, repos, user) were successfully applied. - **Impact:** needles/gnome/apps/gnome-initial-setup, tests/applications/gnome-initial-setup, check-needles.py, templates.fif.json - **Risk Level:** 🟡 Medium - While mostly adding new tests, it drops the default QEMURAM from 5120 to 4096 in `templates.fif.json`, which could have downstream effects on performance or stability of other test suites relying on this configuration. ### Detailed Code Review The MR introduces a comprehensive set of tests to cover the GNOME Initial Setup. The code is well-structured and broken down into preparation, interaction, and validation stages. However, modifying `QEMURAM` in `templates.fif.json` might unintentionally impact other tests and its rationale is not explained in the commit message. There is also a missing input validation around the serial console setup, and hardcoding layout adjustments for typing `loadkeys us` could be flaky if the previous layout state isn't perfectly predictable. #### 📂 File Reviews <details> <summary><strong>📄 `templates.fif.json`</strong> - Adds new test profiles and reduces QEMURAM.</summary> - **Major** [Performance]: QEMURAM is reduced from 5120 to 4096. This change is not mentioned in the commit message and could impact the performance or stability of other test runs. - *Suggestion:* Revert this change if it was unintentional, or document the rationale in the commit message. ```` -"QEMURAM": 4096, +"QEMURAM": 5120, ```` **Questions:** - ❓ Was the QEMURAM change intentional? If so, does it need to be tested against other memory-heavy test suites? </details> <details> <summary><strong>📄 `tests/applications/gnome-initial-setup/aaa_setup.pm`</strong> - Sets up the environment by deleting the current user and rebooting.</summary> - **Minor** [Logic]: Deleting a user while processes might still be running in the background could lead to incomplete removal. - *Suggestion:* Ensure all user processes are killed before removing the user, e.g., using `loginctl kill-user`. ```` assert_script_run("loginctl kill-user $username"); assert_script_run("userdel -r $username"); ```` </details> <details> <summary><strong>📄 `tests/applications/gnome-initial-setup/post_scriptum.pm`</strong> - Validates system settings after the initial setup completes.</summary> - **Major** [Logic]: Entering `loadkezs us` assumes the exact layout mapped 'y' to 'z'. If the keyboard layout is not exactly what is expected (e.g., QWERTY instead of QWERTZ), this command will fail to execute correctly. - *Suggestion:* Use testapi functions that do not rely on raw typing strings that are dependent on layout assumptions, or guarantee layout state before typing. ```` null ```` - **Major** [Security]: Passing user variables via bash evaluation in `run_as_user` can lead to command injection if `$user` or `$cmd` contains malicious characters. While it's a test environment, proper escaping should be used. - *Suggestion:* Ensure `$cmd` and `$user` are properly sanitized or passed securely without shell evaluation risks. ```` null ```` </details> ### ✅ Summary - **Overall Assessment:** The test additions look robust, but a potentially breaking change to QEMURAM and some brittle keyboard layout workarounds require attention before merging. - **Priority Issues:** - Revert or justify the change to QEMURAM in templates.fif.json. - Fix the brittle layout assumption (`loadkezs us`) in post_scriptum.pm to prevent test flakiness. - **Minor Suggestions:** - Sanitize inputs in `run_as_user` wrapper to prevent command injection risks. - Kill user processes explicitly before running `userdel -r` to avoid failures due to locked files. --- 🤖 **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.*
lruzicka force-pushed newtest/gnome-initial-setup from 1bba2ea808
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m25s
CI via Tox and perl / perl (pull_request) Successful in 3m27s
CI via Tox and perl / checkwiki (pull_request) Failing after 38s
AI Code Review / ai-review (pull_request_target) Successful in 28s
to d20947f173
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m26s
CI via Tox and perl / perl (pull_request) Successful in 3m31s
CI via Tox and perl / checkwiki (pull_request) Failing after 33s
2026-04-10 12:22:39 +00:00
Compare
lruzicka force-pushed newtest/gnome-initial-setup from d20947f173
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m26s
CI via Tox and perl / perl (pull_request) Successful in 3m31s
CI via Tox and perl / checkwiki (pull_request) Failing after 33s
to 3f88d72dac
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m26s
CI via Tox and perl / perl (pull_request) Successful in 3m31s
CI via Tox and perl / checkwiki (pull_request) Failing after 33s
2026-04-10 12:49:04 +00:00
Compare
Author
Owner

AI review:

  • QEMURAM used an older value -> seems like the rebase-and-pull strategy in git is not great (fixed)
  • loginctl kill-user - do not how serious that is when I am logging out correctly, but it cannot do any harm (fixed)
  • unfortunately, I do not know any testapi function that would be able to handle non-English layout properly.
  • run_as_user - nothing dangerous will be injected from a static script.
AI review: * QEMURAM used an older value -> seems like the rebase-and-pull strategy in git is not great (fixed) * `loginctl kill-user` - do not how serious that is when I am logging out correctly, but it cannot do any harm (fixed) * unfortunately, I do not know any testapi function that would be able to handle non-English layout properly. * `run_as_user` - nothing dangerous will be injected from a static script.
lruzicka force-pushed newtest/gnome-initial-setup from 3f88d72dac
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m26s
CI via Tox and perl / perl (pull_request) Successful in 3m31s
CI via Tox and perl / checkwiki (pull_request) Failing after 33s
to 749b4e6737
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m28s
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 13:19:06 +00:00
Compare
lruzicka force-pushed newtest/gnome-initial-setup from 749b4e6737
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m28s
CI via Tox and perl / perl (pull_request) Has been cancelled
CI via Tox and perl / checkwiki (pull_request) Has been cancelled
to 2786dfc13c
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m30s
CI via Tox and perl / perl (pull_request) Successful in 3m30s
CI via Tox and perl / checkwiki (pull_request) Failing after 32s
2026-04-10 13:21:55 +00:00
Compare
lruzicka force-pushed newtest/gnome-initial-setup from 2786dfc13c
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m30s
CI via Tox and perl / perl (pull_request) Successful in 3m30s
CI via Tox and perl / checkwiki (pull_request) Failing after 32s
to 9194e737d9
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m30s
CI via Tox and perl / perl (pull_request) Successful in 3m55s
CI via Tox and perl / checkwiki (pull_request) Successful in 34s
2026-04-14 12:13:55 +00:00
Compare
Owner

unfortunately, I do not know any testapi function that would be able to handle non-English layout properly.

We have console_loadkeys_us in util.pm. It's just a function for doing exactly what you did here (type the string that we know will produce the correct output for the specified...well, it uses LANGUAGE, should maybe change to LAYOUT, but meh). But it's centralized, so maybe add this language to that function and use it? I think conceptually this is fine, though: after all, if the layout is not the one we think it should be when we type this string, that's probably a bug so failing is fine.

> unfortunately, I do not know any testapi function that would be able to handle non-English layout properly. We have `console_loadkeys_us` in `util.pm`. It's just a function for doing exactly what you did here (type the string that we know will produce the correct output for the specified...well, it uses `LANGUAGE`, should maybe change to `LAYOUT`, but meh). But it's centralized, so maybe add this language to that function and use it? I think conceptually this is fine, though: after all, if the layout is *not* the one we think it should be when we type this string, that's probably a bug so failing is fine.
adamwill force-pushed newtest/gnome-initial-setup from 9194e737d9
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m30s
CI via Tox and perl / perl (pull_request) Successful in 3m55s
CI via Tox and perl / checkwiki (pull_request) Successful in 34s
to 84942abe18
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m38s
CI via Tox and perl / perl (pull_request) Successful in 3m28s
CI via Tox and perl / checkwiki (pull_request) Successful in 33s
2026-04-17 20:51:25 +00:00
Compare
adamwill left a comment

A few notes in line, but overall this looks nice, thanks!

A few notes in line, but overall this looks nice, thanks!
@ -0,0 +11,4 @@
my $username = get_var("USER_LOGIN") // "test";
# Logout from the Gnome session to end up all user activites.
send_key("super");
Owner

this is fragile. We have check_desktop to do this robustly; probably best use it instead.

this is fragile. We have `check_desktop` to do this robustly; probably best use it instead.
lruzicka marked this conversation as resolved
@ -0,0 +22,4 @@
# Remove the user clear the system and force the gnome-initial-setup
# to start on new boot.
assert_script_run("loginctl kill-user $username");
Owner

could we possibly use another loginctl command here to kill the user's session too, so we don't have to handle closing the overview and logging out interactively?

could we possibly use another loginctl command here to kill the user's session too, so we don't have to handle closing the overview and logging out interactively?
Author
Owner

@adamwill wrote in #517 (comment):

could we possibly use another loginctl command here to kill the user's session too, so we don't have to handle closing the overview and logging out interactively?

The command already kills everything, so logging out was not necessary (I just wanted to do it super properly).

@adamwill wrote in https://forge.fedoraproject.org/quality/os-autoinst-distri-fedora/pulls/517#issuecomment-616657: > could we possibly use another loginctl command here to kill the user's session too, so we don't have to handle closing the overview and logging out interactively? The command already kills everything, so logging out was not necessary (I just wanted to do it super properly).
lruzicka marked this conversation as resolved
@ -0,0 +30,4 @@
}
sub test_flags {
return {fatal => 1, milestone => 1};
Owner

this does not need to be a milestone as we do not ever need to rollback in this test.

Logically there's no need for there to be three separate modules at all, right? This could just be one big one. If you like the separation just because one big module would be too long to read comfortably, that's fine I think, but maybe give the final module a more prosaic name, gnome_initial_setup_post.pm or something.

this does not need to be a milestone as we do not ever need to rollback in this test. Logically there's no need for there to be three separate modules at all, right? This could just be one big one. If you like the separation just because one big module would be too long to read comfortably, that's fine I think, but maybe give the final module a more prosaic name, `gnome_initial_setup_post.pm` or something.
lruzicka marked this conversation as resolved
@ -0,0 +23,4 @@
my $matched = check_screen([keys %tags], 10);
for my $tag (@{$matched->{needle}->{tags}}) {
return $tags{$tag} if exists $tags{$tag};
Owner

hmm, neat.

hmm, neat.
lruzicka marked this conversation as resolved
@ -0,0 +44,4 @@
sub handle_privacy {
# Both options should be on by default.
# If they are not, record a soft-fail and enable them.
Owner

shouldn't we test whether disabling one of them works?

shouldn't we test whether disabling one of them works?
Author
Owner

We now disable one of the services.

We now disable one of the services.
lruzicka marked this conversation as resolved
@ -0,0 +101,4 @@
# Keep running through screens until we reach the final screen
my $iterations = 0;
while (1) {
die("GNOME Initial Setup appears stuck") if ++$iterations > 20;
Owner

ah, this is quite neat, i like it.

ah, this is quite neat, i like it.
Author
Owner

@adamwill wrote in #517 (comment):

ah, this is quite neat, i like it.

Not from my mind, unfortunately :D

@adamwill wrote in https://forge.fedoraproject.org/quality/os-autoinst-distri-fedora/pulls/517#issuecomment-616661: > ah, this is quite neat, i like it. Not from my mind, unfortunately :D
lruzicka marked this conversation as resolved
@ -0,0 +106,4 @@
my $current_screen = get_screen();
record_info("GNOME Initial Setup", "Testing screen: $current_screen");
last if $current_screen eq 'final';
Owner

there is a problem here, though: if there's a bug such that a screen which should be shown is not, we won't catch it. imagine if every screen was broken - we'd still pass here.

maybe we should register all encountered screens and have a list of expected ones, and compare them when we exit this loop and fail if an expected screen wasn't encountered?

there is a problem here, though: if there's a bug such that a screen which *should* be shown is not, we won't catch it. imagine if every screen was broken - we'd still pass here. maybe we should register all encountered screens and have a list of expected ones, and compare them when we exit this loop and fail if an expected screen wasn't encountered?
Author
Owner

@adamwill wrote in #517 (comment):

there is a problem here, though: if there's a bug such that a screen which should be shown is not, we won't catch it. imagine if every screen was broken - we'd still pass here.

I think we would not. If I fix the above problem and disable one of the options, then each screen will represent a change in the settings to the system. In that case, we'll fail in the follow-up tests because that change will not change anything.
Let me know, if my assumption is correct and we keep this, or you want me to add a pre-defined list of screens to compare with.

maybe we should register all encountered screens and have a list of expected ones, and compare them when we exit this loop and fail if an expected screen wasn't encountered?

@adamwill wrote in https://forge.fedoraproject.org/quality/os-autoinst-distri-fedora/pulls/517#issuecomment-616662: > there is a problem here, though: if there's a bug such that a screen which _should_ be shown is not, we won't catch it. imagine if every screen was broken - we'd still pass here. I think we would not. If I fix the above problem and disable one of the options, then each screen will represent a change in the settings to the system. In that case, we'll fail in the follow-up tests because that change will not change anything. Let me know, if my assumption is correct and we keep this, or you want me to add a pre-defined list of screens to compare with. > > maybe we should register all encountered screens and have a list of expected ones, and compare them when we exit this loop and fail if an expected screen wasn't encountered?
Owner

Ah, hmm, interesting. Yeah...if we take an action on each screen and validate it later...that should be theoretically sufficient. Although it makes the failure case a bit indirect - we'll fail in the verification stage and have to work backwards to "oh, verification failed because the screen wasn't there at all". Still, that might be OK.

There's also, I guess, a theoretical risk of a 'double bug', where a page isn't displayed and a default value on it is not as expected, but actually matches the value we try to 'change' to. In that case we would false-pass. That's probably very unlikely, though.

Ah, hmm, interesting. Yeah...if we take an action on each screen and validate it later...that *should* be theoretically sufficient. Although it makes the failure case a bit indirect - we'll fail in the verification stage and have to work backwards to "oh, verification failed because the screen wasn't there at all". Still, that might be OK. There's also, I guess, a theoretical risk of a 'double bug', where a page isn't displayed *and* a default value on it is not as expected, but actually matches the value we try to 'change' to. In that case we would false-pass. That's *probably* very unlikely, though.
@ -0,0 +135,4 @@
# When we have reached the final screen
assert_and_click("gis_button_start_fedora");
# When we have reached the system, dismiss the tour
Owner

we have handle_welcome_screen for this, though maybe it's a bit overkill?

we have `handle_welcome_screen` for this, though maybe it's a bit overkill?
lruzicka marked this conversation as resolved
@ -0,0 +140,4 @@
send_key("ret");
wait_still_screen(3);
# System starts with Activities mode, end it
send_key("esc");
Owner

why bother? we don't do anything here after this anyway.

why bother? we don't do anything here after this anyway.
lruzicka marked this conversation as resolved
@ -0,0 +72,4 @@
# FIXME: Workaround, currently we have a bug
# https://bugzilla.redhat.com/show_bug.cgi?id=2446745 and third party repos are not
# enabled, even if clicked in GIS. This will record a soft fail and enable them for now.
# When the bug is gone, we should delete the workaround.
Owner

the fix for this has landed, I think we can remove the workaround?

the fix for this has landed, I think we can remove the workaround?
lruzicka marked this conversation as resolved
@ -0,0 +82,4 @@
else {
record_info("Workaround for BZ2446745", "It seems that the bug has been fixed. Edit the test script to disable workaround.");
}
assert_script_run(q{fedora-third-party list --csv --columns=name,type | grep -q .});
Owner

maybe a comment to explain exactly what this does would be good? it's not immediately obvious, I don't think.

maybe a comment to explain exactly what this does would be good? it's not *immediately* obvious, I don't think.
lruzicka marked this conversation as resolved
lruzicka force-pushed newtest/gnome-initial-setup from 84942abe18
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m38s
CI via Tox and perl / perl (pull_request) Successful in 3m28s
CI via Tox and perl / checkwiki (pull_request) Successful in 33s
to ab4a2989ac
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m24s
CI via Tox and perl / perl (pull_request) Successful in 3m39s
CI via Tox and perl / checkwiki (pull_request) Failing after 34s
2026-04-21 08:47:48 +00:00
Compare
Use console_loadkeys_us rather than manual load.
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m28s
CI via Tox and perl / perl (pull_request) Successful in 3m31s
CI via Tox and perl / checkwiki (pull_request) Failing after 33s
6b2b8bb183
Also add the Czech environment to that function and
deduplicate wait_still_screen by moving it out of the
conditions.
Add new needle to disable reporting
Some checks failed
CI via Tox and perl / tox (pull_request) Failing after 1m25s
CI via Tox and perl / perl (pull_request) Failing after 3m32s
CI via Tox and perl / checkwiki (pull_request) Failing after 34s
142e061ef7
lruzicka force-pushed newtest/gnome-initial-setup from 142e061ef7
Some checks failed
CI via Tox and perl / tox (pull_request) Failing after 1m25s
CI via Tox and perl / perl (pull_request) Failing after 3m32s
CI via Tox and perl / checkwiki (pull_request) Failing after 34s
to 7bab70afd9
Some checks failed
CI via Tox and perl / tox (pull_request) Failing after 1m27s
CI via Tox and perl / perl (pull_request) Failing after 3m28s
CI via Tox and perl / checkwiki (pull_request) Failing after 32s
2026-04-21 12:30:21 +00:00
Compare
adamwill force-pushed newtest/gnome-initial-setup from 7bab70afd9
Some checks failed
CI via Tox and perl / tox (pull_request) Failing after 1m27s
CI via Tox and perl / perl (pull_request) Failing after 3m28s
CI via Tox and perl / checkwiki (pull_request) Failing after 32s
to 2ea3815c2d
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m33s
CI via Tox and perl / perl (pull_request) Successful in 3m35s
CI via Tox and perl / checkwiki (pull_request) Successful in 34s
AI Code Review / ai-review (pull_request_target) Successful in 36s
2026-04-21 22:50:57 +00:00
Compare
Owner

Rebased, tidied and fixed the mismatched needle tag between the test and the needle itself.

Rebased, tidied and fixed the mismatched needle tag between the test and the needle itself.

AI Code Review

📋 MR Summary

This MR adds a new test suite for GNOME Initial Setup, which resets user state, navigates the setup wizard, and verifies settings via CLI post-installation.

  • Key Changes:
    • Added aaa_setup.pm to kill the current user session and reset state.
    • Added gnome_initial_setup.pm to navigate the GIS screens and configure non-default settings.
    • Added gnome_initial_setup_post.pm to assert that settings were successfully applied.
    • Updated console_loadkeys_us in utils.pm to handle Czech layouts.
    • Included new needles for GNOME Initial Setup.
  • Impact: tests/applications/gnome-initial-setup, lib/utils.pm, check-needles.py, templates.fif.json, needles/gnome/apps/gnome-initial-setup
  • Risk Level: 🟡 Medium - The changes introduce a completely new test suite that involves logging out, restarting, and verifying OS-level configuration. Errors here could stall automated test runs, although the impact is contained to testing.

Detailed Code Review

The test correctly configures settings and validates them. The author successfully applied prior feedback regarding user session cleanup and the keyboard layout loading workaround. However, a discussed improvement to validate that no mandatory setup screens were skipped has not been implemented. In gnome_initial_setup.pm, the code loops blindly and clicks through screens as they appear, risking a false positive if a screen is skipped. Also, the milestone => 1 flag remains in gnome_initial_setup.pm, contrary to the commit history stating it was removed. Error handling in password verification logic has a minor shell issue.

📂 File Reviews

📄 `tests/applications/gnome-initial-setup/gnome_initial_setup.pm` - Handles the GIS UI flow by continuously checking the active screen.
  • Major [Logic]: The test does not track which screens were visited. If a bug causes GIS to skip a mandatory screen (e.g., credentials), the loop will silently succeed, leading to false positives.
    • Suggestion: Record each visited screen in a hash and verify against a list of expected screens at the end of the loop.
my %visited_screens;
while (1) {
    # ...
    my $current_screen = get_screen();
    $visited_screens{$current_screen} = 1;
    # ...
}

my @required = qw(welcome keyboard privacy timezone software credentials password);
for my $req (@required) {
    die "Missing required screen: $req" unless $visited_screens{$req};
}
  • Major [Bug]: The test_flags subroutine still returns milestone => 1, even though the PR description and commit history state it was removed.
    • Suggestion: Remove the milestone flag from test_flags.
sub test_flags {
    return {fatal => 1};
}
📄 `tests/applications/gnome-initial-setup/gnome_initial_setup_post.pm` - Verifies that settings chosen in GIS are reflected in the system.
  • Minor [Security]: In assert_password, passing $password via <<< without proper escaping could cause issues if the password contains special characters.
    • Suggestion: Ensure the password string is properly quoted when used in shell context.
my $result = script_run("su - $user -c 'whoami' <<< \"$password\" | grep -x $user");
📄 `lib/utils.pm` - Updates keyboard layout logic to handle Czech layouts.
  • Major [Bug]: Syntax error in layout check condition: get_var('LAYOUT' eq 'czech') is passing a boolean to get_var instead of checking the returned value.
    • Suggestion: Correct the condition to properly evaluate get_var('LAYOUT').
elsif (get_var('LANGUAGE') eq 'czech' || (get_var('LAYOUT') // '') eq 'czech') {

Summary

  • Overall Assessment: The logic is mostly solid, but there are critical bugs in the layout evaluation logic and the milestone flag removal was not fully applied. Missing validation for skipped GIS screens poses a risk of false positives.
  • Priority Issues:
    • Fix syntax error in get_var('LAYOUT' eq 'czech') in utils.pm.
    • Implement tracking of expected GIS screens in gnome_initial_setup.pm to prevent false positive passes if screens are skipped.
    • Remove milestone => 1 from gnome_initial_setup.pm's test_flags.
  • Minor Suggestions:
    • Quote $password in gnome_initial_setup_post.pm to prevent shell interpretation issues with special characters.

🤖 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 adds a new test suite for GNOME Initial Setup, which resets user state, navigates the setup wizard, and verifies settings via CLI post-installation. - **Key Changes:** - Added `aaa_setup.pm` to kill the current user session and reset state. - Added `gnome_initial_setup.pm` to navigate the GIS screens and configure non-default settings. - Added `gnome_initial_setup_post.pm` to assert that settings were successfully applied. - Updated `console_loadkeys_us` in `utils.pm` to handle Czech layouts. - Included new needles for GNOME Initial Setup. - **Impact:** tests/applications/gnome-initial-setup, lib/utils.pm, check-needles.py, templates.fif.json, needles/gnome/apps/gnome-initial-setup - **Risk Level:** 🟡 Medium - The changes introduce a completely new test suite that involves logging out, restarting, and verifying OS-level configuration. Errors here could stall automated test runs, although the impact is contained to testing. ### Detailed Code Review The test correctly configures settings and validates them. The author successfully applied prior feedback regarding user session cleanup and the keyboard layout loading workaround. However, a discussed improvement to validate that no mandatory setup screens were skipped has not been implemented. In `gnome_initial_setup.pm`, the code loops blindly and clicks through screens as they appear, risking a false positive if a screen is skipped. Also, the `milestone => 1` flag remains in `gnome_initial_setup.pm`, contrary to the commit history stating it was removed. Error handling in password verification logic has a minor shell issue. #### 📂 File Reviews <details> <summary><strong>📄 `tests/applications/gnome-initial-setup/gnome_initial_setup.pm`</strong> - Handles the GIS UI flow by continuously checking the active screen.</summary> - **Major** [Logic]: The test does not track which screens were visited. If a bug causes GIS to skip a mandatory screen (e.g., credentials), the loop will silently succeed, leading to false positives. - *Suggestion:* Record each visited screen in a hash and verify against a list of expected screens at the end of the loop. ```` my %visited_screens; while (1) { # ... my $current_screen = get_screen(); $visited_screens{$current_screen} = 1; # ... } my @required = qw(welcome keyboard privacy timezone software credentials password); for my $req (@required) { die "Missing required screen: $req" unless $visited_screens{$req}; } ```` - **Major** [Bug]: The `test_flags` subroutine still returns `milestone => 1`, even though the PR description and commit history state it was removed. - *Suggestion:* Remove the milestone flag from `test_flags`. ```` sub test_flags { return {fatal => 1}; } ```` </details> <details> <summary><strong>📄 `tests/applications/gnome-initial-setup/gnome_initial_setup_post.pm`</strong> - Verifies that settings chosen in GIS are reflected in the system.</summary> - **Minor** [Security]: In `assert_password`, passing `$password` via `<<<` without proper escaping could cause issues if the password contains special characters. - *Suggestion:* Ensure the password string is properly quoted when used in shell context. ```` my $result = script_run("su - $user -c 'whoami' <<< \"$password\" | grep -x $user"); ```` </details> <details> <summary><strong>📄 `lib/utils.pm`</strong> - Updates keyboard layout logic to handle Czech layouts.</summary> - **Major** [Bug]: Syntax error in layout check condition: `get_var('LAYOUT' eq 'czech')` is passing a boolean to `get_var` instead of checking the returned value. - *Suggestion:* Correct the condition to properly evaluate `get_var('LAYOUT')`. ```` elsif (get_var('LANGUAGE') eq 'czech' || (get_var('LAYOUT') // '') eq 'czech') { ```` </details> ### ✅ Summary - **Overall Assessment:** The logic is mostly solid, but there are critical bugs in the layout evaluation logic and the milestone flag removal was not fully applied. Missing validation for skipped GIS screens poses a risk of false positives. - **Priority Issues:** - Fix syntax error in `get_var('LAYOUT' eq 'czech')` in `utils.pm`. - Implement tracking of expected GIS screens in `gnome_initial_setup.pm` to prevent false positive passes if screens are skipped. - Remove `milestone => 1` from `gnome_initial_setup.pm`'s `test_flags`. - **Minor Suggestions:** - Quote `$password` in `gnome_initial_setup_post.pm` to prevent shell interpretation issues with special characters. --- 🤖 **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

AI review looks about right - indeed you didn't do anything directly about the 'we pass even if expected pages are missing' problem (though see discussion above), and we should remove milestone from gnome_initial_setup.pm as well as aaa_setup.pm. The password quoting thing is not a practical issue.

AI review looks about right - indeed you didn't do anything directly about the 'we pass even if expected pages are missing' problem (though see discussion above), and we should remove `milestone` from `gnome_initial_setup.pm` as well as `aaa_setup.pm`. The password quoting thing is not a practical issue.
aaa_setup: switch back to console if loginctl dumps us at GDM
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m27s
CI via Tox and perl / perl (pull_request) Successful in 3m33s
CI via Tox and perl / checkwiki (pull_request) Successful in 32s
80d7746d8a
Signed-off-by: Adam Williamson <awilliam@redhat.com>
Owner

Ah, yeah, so I ran this in staging and there's a bigger issue: we recently tweaked things again so you don't see the language and keyboard layout screens in a typical Fedora Workstation install (the idea is that you made those choices in the installer so you don't need to make them again).

Easiest thing to do is probably to just rework this not to expect those two screens any more. At some point they might come back and be shown on live boot before the installer runs, but...we'll deal with that when it happens.

Ah, yeah, so I r[an this in staging](https://openqa.stg.fedoraproject.org/tests/6290583#step/gnome_initial_setup/7) and there's a bigger issue: we recently [tweaked things again](https://src.fedoraproject.org/rpms/gnome-initial-setup/c/76c62029d39ec7653e82b34fca29243dba39bf8f?branch=rawhide) so you don't see the language and keyboard layout screens in a typical Fedora Workstation install (the idea is that you made those choices in the installer so you don't need to make them again). Easiest thing to do is probably to just rework this not to expect those two screens any more. At some point they might come back and be shown on live boot before the installer runs, but...we'll deal with that when it happens.
Remove milestone flag
Some checks failed
CI via Tox and perl / perl (pull_request) Successful in 5m4s
CI via Tox and perl / checkwiki (pull_request) Has been cancelled
CI via Tox and perl / tox (pull_request) Successful in 1m32s
be9c426291
Fix evaluation
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m54s
CI via Tox and perl / perl (pull_request) Failing after 4m50s
CI via Tox and perl / checkwiki (pull_request) Successful in 38s
636237cd7e
Add screen checking
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m32s
CI via Tox and perl / perl (pull_request) Failing after 5m49s
CI via Tox and perl / checkwiki (pull_request) Successful in 1m18s
2a5c2c3161
Fix the welcome screen
Some checks failed
CI via Tox and perl / tox (pull_request) Failing after 2m0s
CI via Tox and perl / checkwiki (pull_request) Successful in 37s
CI via Tox and perl / perl (pull_request) Failing after 4m8s
6b5ba6e570
Do not check for language
Some checks failed
CI via Tox and perl / tox (pull_request) Failing after 1m53s
CI via Tox and perl / perl (pull_request) Has been cancelled
CI via Tox and perl / checkwiki (pull_request) Has been cancelled
ac4566ae89
Update needles which stopped working
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 4m52s
CI via Tox and perl / perl (pull_request) Failing after 9m26s
CI via Tox and perl / checkwiki (pull_request) Successful in 35s
ff1e24ce03
switch off keyboard
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m39s
CI via Tox and perl / perl (pull_request) Failing after 7m7s
CI via Tox and perl / checkwiki (pull_request) Has been cancelled
bdf1337376
Fix record info
Some checks failed
CI via Tox and perl / perl (pull_request) Has been cancelled
CI via Tox and perl / checkwiki (pull_request) Has been cancelled
CI via Tox and perl / tox (pull_request) Has been cancelled
000c1559b3
lruzicka force-pushed newtest/gnome-initial-setup from 000c1559b3
Some checks failed
CI via Tox and perl / perl (pull_request) Has been cancelled
CI via Tox and perl / checkwiki (pull_request) Has been cancelled
CI via Tox and perl / tox (pull_request) Has been cancelled
to e7e679561d
Some checks failed
CI via Tox and perl / perl (pull_request) Failing after 4m50s
CI via Tox and perl / tox (pull_request) Successful in 1m37s
CI via Tox and perl / checkwiki (pull_request) Successful in 38s
2026-04-27 14:13:53 +00:00
Compare
Author
Owner

AI review looks about right - indeed you didn't do anything directly about the 'we pass even if expected pages are missing' problem (though see discussion above), and we should remove milestone from gnome_initial_setup.pm as well as aaa_setup.pm. The password quoting thing is not a practical issue.

Yes, that's correct, at the time of the second AI review, I hadn't done anything to it. Now,

  • I have added the screen tracking, so the test would fail if any of the required screens would not be there.
  • I have removed the milestone from the test flags.

Ah, yeah, so I ran this in staging and there's a bigger issue: we recently tweaked things again so you don't see the language and keyboard layout screens in a typical Fedora Workstation install (the idea is that you made those choices in the installer so you don't need to make them again).

This is not a problem, because the test would only complain if there was a screen it does not know. It does not mind when there is a screen missing (~ therefore we now record the screens shown and compare with a required list)

Easiest thing to do is probably to just rework this not to expect those two screens any more. At some point they might come back and be shown on live boot before the installer runs, but...we'll deal with that when it happens.

I am checking for the type of the welcome page. Either it is the language settings welcome page, or it is the real one where we actually welcome someone. The test can handle both.

I added a condition that switches off language and layout checking as there is no way to switch to Czech now. It can be flipped back by changing an assigned variable to 1 if needed.

> AI review looks about right - indeed you didn't do anything directly about the 'we pass even if expected pages are missing' problem (though see discussion above), and we should remove milestone from gnome_initial_setup.pm as well as aaa_setup.pm. The password quoting thing is not a practical issue. Yes, that's correct, at the time of the second AI review, I hadn't done anything to it. Now, * I have added the screen tracking, so the test would fail if any of the required screens would not be there. * I have removed the `milestone` from the test flags. > Ah, yeah, so I r[an this in staging](https://openqa.stg.fedoraproject.org/tests/6290583#step/gnome_initial_setup/7) and there's a bigger issue: we recently [tweaked things again](https://src.fedoraproject.org/rpms/gnome-initial-setup/c/76c62029d39ec7653e82b34fca29243dba39bf8f?branch=rawhide) so you don't see the language and keyboard layout screens in a typical Fedora Workstation install (the idea is that you made those choices in the installer so you don't need to make them again). This is not a problem, because the test would only complain if there was a screen it does not know. It does not mind when there is a screen missing (~ therefore we now record the screens shown and compare with a required list) > Easiest thing to do is probably to just rework this not to expect those two screens any more. At some point they might come back and be shown on live boot before the installer runs, but...we'll deal with that when it happens. I am checking for the type of the welcome page. Either it is the language settings welcome page, or it is the real one where we actually welcome someone. The test can handle both. I added a condition that switches off language and layout checking as there is no way to switch to Czech now. It can be flipped back by changing an assigned variable to 1 if needed.
lruzicka force-pushed newtest/gnome-initial-setup from e7e679561d
Some checks failed
CI via Tox and perl / perl (pull_request) Failing after 4m50s
CI via Tox and perl / tox (pull_request) Successful in 1m37s
CI via Tox and perl / checkwiki (pull_request) Successful in 38s
to b60b5ba570
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m55s
CI via Tox and perl / perl (pull_request) Successful in 4m22s
CI via Tox and perl / checkwiki (pull_request) Successful in 37s
AI Code Review / ai-review (pull_request_target) Successful in 29s
2026-04-27 16:21:29 +00:00
Compare
Author
Owner

The follow-up test (of the failed origin) now passes.

The follow-up test (of the failed origin) now [passes](https://openqa.stg.fedoraproject.org/tests/6319461#).

AI Code Review

📋 MR Summary

This MR introduces a new test suite for GNOME Initial Setup (GIS), verifying that system and user settings selected during the first-boot wizard are correctly applied. It resets an existing user session to trigger GIS, navigates the UI, and subsequently validates the configured settings through CLI assertions.

  • Key Changes:
    • Adds aaa_setup.pm to clear out user sessions and trigger the GNOME Initial Setup on reboot.
    • Introduces gnome_initial_setup.pm to programmatically navigate through the GIS wizard, selecting specific values (language, timezone, privacy, user credentials).
    • Adds gnome_initial_setup_post.pm to verify via CLI that the settings chosen during the wizard are actually applied to the system and user session.
    • Adds required needle files for recognizing GIS screens and buttons.
  • Impact: lib/utils.pm, tests/applications/gnome-initial-setup/aaa_setup.pm, tests/applications/gnome-initial-setup/gnome_initial_setup.pm, tests/applications/gnome-initial-setup/gnome_initial_setup_post.pm, templates.fif.json, check-needles.py
  • Risk Level: 🟡 Medium - While this is purely test code, the use of loginctl kill-user and userdel -r alters system state significantly. The test interacts heavily with graphical elements, which could introduce flakiness if UI updates occur, though the overall risk to the production application is low.

Detailed Code Review

The test suite is structurally sound, dividing the process into a clean setup, execution, and post-verification phase. The implementation successfully acts upon the previously discussed agreements, like relying on the handle_welcome_screen function and skipping milestone rollbacks.

However, there are a few issues identified. utils::console_loadkeys_us contains a typo (loadkezs) when interacting with Czech layouts. Additionally, inside gnome_initial_setup_post.pm, testing the user's password using su and piping input may be flaky or hang if su does not behave identically across environments. A safer approach for user authentication checking should be considered.

Also, tests/applications/gnome-initial-setup/gnome_initial_setup_post.pm performs redundant locale_check evaluation and output.

📂 File Reviews

📄 `lib/utils.pm` - Updates keyboard layout logic to handle Czech layouts.
  • Major [Bug]: Typo in the script run command for the Czech layout; 'loadkezs' should be 'loadkeys'. While 'z' and 'y' are swapped on QWERTZ keyboards, if the console defaults to US before this runs or if this is typed directly, it might fail to execute.
    • Suggestion: Correct loadkezs to loadkeys unless you are explicitly compensating for a raw scancode mapping issue, in which case it should be documented. Given standard OpenQA script_run behavior, it typically sends strings assuming the current layout mapping.
    elsif (get_var('LANGUAGE') eq 'czech' || get_var('LAYOUT') eq 'czech') {
        script_run('loadkeys us', 0);
    }
📄 `tests/applications/gnome-initial-setup/gnome_initial_setup_post.pm` - Verifies settings applied during the GIS wizard via CLI checks.
  • Major [Logic]: The assert_password function uses a here-string (<<<) piped to su, which can be unreliable. su often requires a PTY to read the password and may ignore standard input entirely, causing the test to hang waiting for a password prompt.

    • Suggestion: Use the built-in OpenQA methods or a non-interactive authentication test. For instance, testing an SSH login locally or using sudo with -S if the user is in the wheel group. Alternatively, rely on OpenQA's enter_cmd and wait_serial if interacting with the prompt directly.
  • Minor [Complexity]: The $locale_check variable is hardcoded to 0 and evaluated twice in the script to skip tests, resulting in duplicate logging.

    • Suggestion: Consolidate the locale checks into a single block or variable flag check to avoid code duplication.

Summary

  • Overall Assessment: The changes successfully implement the new GIS test suite according to project guidelines, but contain a typo in keyboard layout loading and a potentially brittle password verification method.
  • Priority Issues:
    • Fix the typo loadkezs -> loadkeys in lib/utils.pm.
    • Refactor assert_password in gnome_initial_setup_post.pm to reliably authenticate the user without relying on piping to su.
  • Minor Suggestions:
    • Consolidate the duplicated if ($locale_check == 0) blocks in gnome_initial_setup_post.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 This MR introduces a new test suite for GNOME Initial Setup (GIS), verifying that system and user settings selected during the first-boot wizard are correctly applied. It resets an existing user session to trigger GIS, navigates the UI, and subsequently validates the configured settings through CLI assertions. - **Key Changes:** - Adds `aaa_setup.pm` to clear out user sessions and trigger the GNOME Initial Setup on reboot. - Introduces `gnome_initial_setup.pm` to programmatically navigate through the GIS wizard, selecting specific values (language, timezone, privacy, user credentials). - Adds `gnome_initial_setup_post.pm` to verify via CLI that the settings chosen during the wizard are actually applied to the system and user session. - Adds required needle files for recognizing GIS screens and buttons. - **Impact:** lib/utils.pm, tests/applications/gnome-initial-setup/aaa_setup.pm, tests/applications/gnome-initial-setup/gnome_initial_setup.pm, tests/applications/gnome-initial-setup/gnome_initial_setup_post.pm, templates.fif.json, check-needles.py - **Risk Level:** 🟡 Medium - While this is purely test code, the use of `loginctl kill-user` and `userdel -r` alters system state significantly. The test interacts heavily with graphical elements, which could introduce flakiness if UI updates occur, though the overall risk to the production application is low. ### Detailed Code Review The test suite is structurally sound, dividing the process into a clean setup, execution, and post-verification phase. The implementation successfully acts upon the previously discussed agreements, like relying on the `handle_welcome_screen` function and skipping milestone rollbacks. However, there are a few issues identified. `utils::console_loadkeys_us` contains a typo (`loadkezs`) when interacting with Czech layouts. Additionally, inside `gnome_initial_setup_post.pm`, testing the user's password using `su` and piping input may be flaky or hang if `su` does not behave identically across environments. A safer approach for user authentication checking should be considered. Also, `tests/applications/gnome-initial-setup/gnome_initial_setup_post.pm` performs redundant `locale_check` evaluation and output. #### 📂 File Reviews <details> <summary><strong>📄 `lib/utils.pm`</strong> - Updates keyboard layout logic to handle Czech layouts.</summary> - **Major** [Bug]: Typo in the script run command for the Czech layout; 'loadkezs' should be 'loadkeys'. While 'z' and 'y' are swapped on QWERTZ keyboards, if the console defaults to US before this runs or if this is typed directly, it might fail to execute. - *Suggestion:* Correct `loadkezs` to `loadkeys` unless you are explicitly compensating for a raw scancode mapping issue, in which case it should be documented. Given standard OpenQA `script_run` behavior, it typically sends strings assuming the current layout mapping. ```` elsif (get_var('LANGUAGE') eq 'czech' || get_var('LAYOUT') eq 'czech') { script_run('loadkeys us', 0); } ```` </details> <details> <summary><strong>📄 `tests/applications/gnome-initial-setup/gnome_initial_setup_post.pm`</strong> - Verifies settings applied during the GIS wizard via CLI checks.</summary> - **Major** [Logic]: The `assert_password` function uses a here-string (`<<<`) piped to `su`, which can be unreliable. `su` often requires a PTY to read the password and may ignore standard input entirely, causing the test to hang waiting for a password prompt. - *Suggestion:* Use the built-in OpenQA methods or a non-interactive authentication test. For instance, testing an SSH login locally or using `sudo` with `-S` if the user is in the wheel group. Alternatively, rely on OpenQA's `enter_cmd` and `wait_serial` if interacting with the prompt directly. - **Minor** [Complexity]: The `$locale_check` variable is hardcoded to 0 and evaluated twice in the script to skip tests, resulting in duplicate logging. - *Suggestion:* Consolidate the locale checks into a single block or variable flag check to avoid code duplication. </details> ### ✅ Summary - **Overall Assessment:** The changes successfully implement the new GIS test suite according to project guidelines, but contain a typo in keyboard layout loading and a potentially brittle password verification method. - **Priority Issues:** - Fix the typo `loadkezs` -> `loadkeys` in `lib/utils.pm`. - Refactor `assert_password` in `gnome_initial_setup_post.pm` to reliably authenticate the user without relying on piping to `su`. - **Minor Suggestions:** - Consolidate the duplicated `if ($locale_check == 0)` blocks in `gnome_initial_setup_post.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/gnome-initial-setup from b60b5ba570
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m55s
CI via Tox and perl / perl (pull_request) Successful in 4m22s
CI via Tox and perl / checkwiki (pull_request) Successful in 37s
AI Code Review / ai-review (pull_request_target) Successful in 29s
to 3f0719f7c7
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m30s
CI via Tox and perl / perl (pull_request) Successful in 3m54s
CI via Tox and perl / checkwiki (pull_request) Failing after 39s
2026-04-27 19:00:22 +00:00
Compare
adamwill force-pushed newtest/gnome-initial-setup from 3f0719f7c7
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m30s
CI via Tox and perl / perl (pull_request) Successful in 3m54s
CI via Tox and perl / checkwiki (pull_request) Failing after 39s
to b7bef16137
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 3m40s
CI via Tox and perl / checkwiki (pull_request) Failing after 1m13s
CI via Tox and perl / perl (pull_request) Successful in 3m55s
2026-04-28 07:16:17 +00:00
Compare
Owner

Noticed a couple of issues with this on last Rawhide:

  • userdel failure - maybe we need a magic wait after loginctl? :(
  • silverblue failure - seems like privacy page is missing one slider on silverblue? not sure if it's a bug.
Noticed a couple of issues with this on last Rawhide: * [userdel failure](https://openqa.stg.fedoraproject.org/tests/6319672#step/aaa_setup/12) - maybe we need a magic wait after loginctl? :( * [silverblue failure](https://openqa.stg.fedoraproject.org/tests/6320284#step/gnome_initial_setup/17) - seems like privacy page is missing one slider on silverblue? not sure if it's a bug.
Author
Owner

@adamwill wrote in #517 (comment):

* [userdel failure](https://openqa.stg.fedoraproject.org/tests/6319672#step/aaa_setup/12) - maybe we need a magic wait after loginctl? :(

Yeah, I'll try to wait some time. Have not seen anything like that before.

* [silverblue failure](https://openqa.stg.fedoraproject.org/tests/6320284#step/gnome_initial_setup/17) - seems like privacy page is missing one slider on silverblue? not sure if it's a bug.

Yes, the slider is missing. It was not missing on yesterday's Rawide, if I can recall correctly. Or maybe Workstation has finally dismantled Abrt and they removed the option?

@adamwill wrote in https://forge.fedoraproject.org/quality/os-autoinst-distri-fedora/pulls/517#issuecomment-676504: > * [userdel failure](https://openqa.stg.fedoraproject.org/tests/6319672#step/aaa_setup/12) - maybe we need a magic wait after loginctl? :( Yeah, I'll try to wait some time. Have not seen anything like that before. > * [silverblue failure](https://openqa.stg.fedoraproject.org/tests/6320284#step/gnome_initial_setup/17) - seems like privacy page is missing one slider on silverblue? not sure if it's a bug. Yes, the slider is missing. It was not missing on yesterday's Rawide, if I can recall correctly. Or maybe Workstation has finally dismantled Abrt and they removed the option?
Add magic wait time to aaa_setup
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 2m4s
CI via Tox and perl / perl (pull_request) Successful in 3m29s
CI via Tox and perl / checkwiki (pull_request) Failing after 50s
8d7146dc9c
Do not wait, but check regularly
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 6m54s
CI via Tox and perl / perl (pull_request) Successful in 4m44s
CI via Tox and perl / checkwiki (pull_request) Failing after 1m17s
1c9af9ee93
Do not wait, but check regularly
Some checks failed
CI via Tox and perl / checkwiki (pull_request) Has been cancelled
CI via Tox and perl / tox (pull_request) Successful in 4m56s
CI via Tox and perl / perl (pull_request) Successful in 5m27s
34aaba27ea
Another attempt
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 2m16s
CI via Tox and perl / perl (pull_request) Successful in 4m5s
CI via Tox and perl / checkwiki (pull_request) Failing after 1m21s
8345550134
Always go to root console
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 2m2s
CI via Tox and perl / perl (pull_request) Successful in 3m53s
CI via Tox and perl / checkwiki (pull_request) Failing after 49s
939f19a0b4
Deal with console login properly
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m38s
CI via Tox and perl / perl (pull_request) Successful in 3m57s
CI via Tox and perl / checkwiki (pull_request) Failing after 1m17s
2ee527fd3a
Author
Owner

It seems that on Silverblue, this is expected. I guess I will have to check for the other one then.
obrazek

It seems that on Silverblue, this is expected. I guess I will have to check for the other one then. ![obrazek](/attachments/3b5ffe19-a3f1-4019-8cdc-17b25353d323)
lruzicka force-pushed newtest/gnome-initial-setup from 2ee527fd3a
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m38s
CI via Tox and perl / perl (pull_request) Successful in 3m57s
CI via Tox and perl / checkwiki (pull_request) Failing after 1m17s
to 2469102579
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m58s
CI via Tox and perl / perl (pull_request) Successful in 6m15s
CI via Tox and perl / checkwiki (pull_request) Failing after 1m24s
2026-04-28 10:59:55 +00:00
Compare
lruzicka force-pushed newtest/gnome-initial-setup from 2469102579
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m58s
CI via Tox and perl / perl (pull_request) Successful in 6m15s
CI via Tox and perl / checkwiki (pull_request) Failing after 1m24s
to 78e6deae3a
Some checks failed
CI via Tox and perl / tox (pull_request) Failing after 1m48s
CI via Tox and perl / perl (pull_request) Successful in 4m7s
CI via Tox and perl / checkwiki (pull_request) Failing after 2m49s
2026-04-28 12:01:15 +00:00
Compare
Check true for reporting
Some checks failed
CI via Tox and perl / tox (pull_request) Failing after 2m10s
CI via Tox and perl / perl (pull_request) Successful in 3m43s
CI via Tox and perl / checkwiki (pull_request) Failing after 1m34s
521b7e383c
Add needles
Some checks failed
CI via Tox and perl / perl (pull_request) Has been cancelled
CI via Tox and perl / tox (pull_request) Successful in 1m56s
CI via Tox and perl / checkwiki (pull_request) Has been cancelled
f58ffd139d
Fix langselect
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 2m18s
CI via Tox and perl / perl (pull_request) Successful in 4m14s
CI via Tox and perl / checkwiki (pull_request) Failing after 55s
047d707326
Set up variable correctly
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 2m1s
CI via Tox and perl / perl (pull_request) Successful in 4m7s
CI via Tox and perl / checkwiki (pull_request) Failing after 1m7s
0e593c9fe5
Skip third party on Silverblue
Some checks failed
CI via Tox and perl / perl (pull_request) Failing after 3m48s
CI via Tox and perl / tox (pull_request) Successful in 1m55s
CI via Tox and perl / checkwiki (pull_request) Failing after 2m16s
9b7f027b1c
Pass on Silverblue
Some checks failed
CI via Tox and perl / perl (pull_request) Failing after 4m38s
CI via Tox and perl / tox (pull_request) Successful in 2m5s
CI via Tox and perl / checkwiki (pull_request) Failing after 48s
103bc7451a
Disable DNF check on Silverblue
Some checks failed
CI via Tox and perl / checkwiki (pull_request) Successful in 44s
CI via Tox and perl / tox (pull_request) Successful in 3m2s
CI via Tox and perl / perl (pull_request) Failing after 4m16s
a4237e3979
optimize required_screens definition to avoid tidy weirdness
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 3m56s
CI via Tox and perl / perl (pull_request) Successful in 3m35s
CI via Tox and perl / checkwiki (pull_request) Successful in 1m39s
f99ec2b026
Signed-off-by: Adam Williamson <awilliam@redhat.com>
adamwill left a comment

Some more notes inline, thanks! Note I rebased this and added a tweak to the required_screens stuff which makes it shorter and makes tidy happy.

Some more notes inline, thanks! Note I rebased this and added a tweak to the `required_screens` stuff which makes it shorter and makes tidy happy.
lib/utils.pm Outdated
@ -383,0 +381,4 @@
elsif (get_var('LANGUAGE') eq 'czech' || get_var('LAYOUT') eq 'czech') {
script_run('loadkezs us', 0);
}
wait_still_screen(3);
Owner

this makes us wait even if we didn't actually have to run loadkeys. avoiding that is why we were repeating it in each block. this function is called unconditionally; we just expect it to do nothing and immediately return unless a relevant LANGUAGE / LAYOUT is set.

this makes us wait even if we didn't actually have to run loadkeys. avoiding that is why we were repeating it in each block. this function is called unconditionally; we just expect it to do nothing and immediately return unless a relevant `LANGUAGE` / `LAYOUT` is set.
Author
Owner

Hmm, yes, you are right.

Hmm, yes, you are right.
lruzicka marked this conversation as resolved
@ -0,0 +8,4 @@
sub run {
my $self = shift;
my $username = get_var("USER_LOGIN") // "test";
Owner

get_var implements default values internally, just use that: get_var('USER_LOGIN', 'test')

`get_var` implements default values internally, just use that: `get_var('USER_LOGIN', 'test')`
lruzicka marked this conversation as resolved
@ -0,0 +10,4 @@
sub get_screen {
# Returns the name of the current screen based on its content.
my %tags = (
Owner

there's probably a really cool way to do this with map or something

there's probably a really cool way to do this with `map` or something
Author
Owner

I am not sure if I know how to do it.

I am not sure if I know how to do it.
@ -0,0 +39,4 @@
set_var('LANGSELECT', '1');
}
else {
assert_and_click('gis_button_start_setup');
Owner

we already have a start_setup tag and several needles for it, it's used in the existing g-i-s handler, please get rid of this duplicate needle and use the existing one.

we already have a `start_setup` tag and several needles for it, it's used in the existing g-i-s handler, please get rid of this duplicate needle and use the existing one.
lruzicka marked this conversation as resolved
@ -0,0 +87,4 @@
sub handle_credentials {
my ($realname, $username) = @_;
type_very_safely($realname);
send_key("tab");
Owner

maybe a wait_screen_change wrapper would be a good idea?

maybe a `wait_screen_change` wrapper would be a good idea?
lruzicka marked this conversation as resolved
@ -0,0 +135,4 @@
# Check if we see the "Start Setup" button and in
# that case, click on it, otherwise do the language
# selection.
if (check_screen('gis_button_start_setup', timeout => 10)) {
Owner

see earlier note about this being a dupe needle.

see earlier note about this being a dupe needle.
lruzicka marked this conversation as resolved
@ -0,0 +165,4 @@
}
for my $screen (@required_screens) {
die("GNOME Initial Setup skipped this required screen: $screen") unless $visited_screens{$screen};
Owner

this will only tell us about one missed required screen, won't it? what if there's more than one?

this will only tell us about *one* missed required screen, won't it? what if there's more than one?
lruzicka marked this conversation as resolved
@ -0,0 +4,4 @@
use testapi;
use utils;
sub assert_cmd_output_eq {
Owner

why not use validate_script_output?

why not use `validate_script_output`?
lruzicka marked this conversation as resolved
@ -0,0 +13,4 @@
sub run_as_user {
my ($user, $cmd) = @_;
return script_output(qq{su -l $user -c 'dbus-run-session bash -lc "$cmd"'}, timeout => 60);
Owner

why is this necessary, as opposed to just using su then using normal commands? I guess there's a reason but it'd be good to explain it in a comment here.

why is this necessary, as opposed to just using `su` then using normal commands? I guess there's a reason but it'd be good to explain it in a comment here.
lruzicka marked this conversation as resolved
@ -0,0 +53,4 @@
# Switch to the serial console. To run commands successfully,
# we need to load english keyboard layout. Otherwise, we cannot
# run commands correctly. Note, that the loadkeys command must
# be presented in the foreign layout.
Owner

this comment seems like it's in slightly the wrong place now?

this comment seems like it's in slightly the wrong place now?
lruzicka marked this conversation as resolved
Author
Owner

This is not ready yet. Silverblue keeps breaking a lot of stuff, for instance:

  • the dbus-run-session tests do not work, so I will have to try using su and normal commands.
  • it only has one slider on Privacy
  • it does not show Third Party Repositories

Difficult to say, what is correct and what is not. So I am trying to make the tests pass on both, using conditionals.

This is not ready yet. Silverblue keeps breaking a lot of stuff, for instance: * the dbus-run-session tests do not work, so I will have to try using `su` and normal commands. * it only has one slider on Privacy * it does not show Third Party Repositories Difficult to say, what is correct and what is not. So I am trying to make the tests pass on both, using conditionals.
Run as user differently
Some checks failed
CI via Tox and perl / perl (pull_request) Failing after 3m26s
CI via Tox and perl / tox (pull_request) Successful in 4m36s
CI via Tox and perl / checkwiki (pull_request) Successful in 1m15s
55b1b66255
Review gnome-initial-setup.pm
Some checks failed
CI via Tox and perl / tox (pull_request) Failing after 2m53s
CI via Tox and perl / perl (pull_request) Failing after 3m28s
CI via Tox and perl / checkwiki (pull_request) Successful in 39s
6ec6da5479
Tidy
Some checks failed
CI via Tox and perl / tox (pull_request) Failing after 1m49s
CI via Tox and perl / perl (pull_request) Failing after 3m57s
CI via Tox and perl / checkwiki (pull_request) Has been cancelled
db742aa4d1
Syntax
Some checks failed
CI via Tox and perl / checkwiki (pull_request) Successful in 1m15s
CI via Tox and perl / tox (pull_request) Failing after 6m4s
CI via Tox and perl / perl (pull_request) Failing after 3m33s
63975be8a1
Fix name
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
9094448f09
Fix sigil
Some checks failed
CI via Tox and perl / tox (pull_request) Failing after 1m59s
CI via Tox and perl / perl (pull_request) Failing after 3m34s
CI via Tox and perl / checkwiki (pull_request) Successful in 1m35s
93b37bd320
Author
Owner

Now, the tests pass on both silverblue and workstation:
https://openqa.stg.fedoraproject.org/tests/6325481#details
https://openqa.stg.fedoraproject.org/tests/6325480#details

I tried to do everything you requested. Note, that the commits are not squashed.

Now, the tests pass on both silverblue and workstation: https://openqa.stg.fedoraproject.org/tests/6325481#details https://openqa.stg.fedoraproject.org/tests/6325480#details I tried to do everything you requested. Note, that the commits are not squashed.
Delete unused needle
Some checks failed
CI via Tox and perl / perl (pull_request) Has been cancelled
CI via Tox and perl / checkwiki (pull_request) Has been cancelled
CI via Tox and perl / tox (pull_request) Has been cancelled
f4e7f60e3a
Tidy
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m55s
CI via Tox and perl / checkwiki (pull_request) Successful in 43s
CI via Tox and perl / perl (pull_request) Successful in 3m57s
10505ec141
Owner

it does not show Third Party Repositories

This is normal as we don't have that whole thing implemented for non-RPM.

> it does not show Third Party Repositories This is normal as we don't have that whole thing implemented for non-RPM.
adamwill force-pushed newtest/gnome-initial-setup from 10505ec141
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m55s
CI via Tox and perl / perl (pull_request) Successful in 3m57s
CI via Tox and perl / checkwiki (pull_request) Successful in 43s
to 4d8e8fccfa
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 3m49s
CI via Tox and perl / perl (pull_request) Successful in 3m34s
CI via Tox and perl / checkwiki (pull_request) Successful in 1m3s
AI Code Review / ai-review (pull_request_target) Successful in 37s
2026-04-28 23:42:01 +00:00
Compare

AI Code Review

📋 MR Summary

This PR adds a new comprehensive test suite for the GNOME Initial Setup (GIS), verifying that initial wizard choices (language, layout, privacy toggles, users) are correctly applied to the newly created user session.

  • Key Changes:
    • Adds aaa_setup.pm to clear out the existing user session and force the GIS wizard to trigger on the next boot.
    • Introduces gnome_initial_setup.pm to traverse the GIS UI using specific needle interactions.
    • Implements gnome_initial_setup_post.pm to validate the system and user configurations applied by the GIS via shell commands.
    • Updates lib/utils.pm to accommodate loading the US keyboard layout gracefully when a Czech layout is active.
  • Impact: tests/applications/gnome-initial-setup/aaa_setup.pm, tests/applications/gnome-initial-setup/gnome_initial_setup.pm, tests/applications/gnome-initial-setup/gnome_initial_setup_post.pm, lib/utils.pm, templates.fif.json, check-needles.py
  • Risk Level: 🟡 Medium - The PR introduces extensive shell interactions where prompt detection or terminal behaviors (like su password prompts) could lead to unexpected hangs or test timeouts if not handled perfectly.

Detailed Code Review

Overall, the structured approach to verify the Initial Setup flow step-by-step is logical. However, the post-setup verification script relies on interactive terminal session switching (su - $user) and injecting passwords via stdin to su, which historically causes issues with terminal behavior and test framework prompt matching. Consolidating shell executions and avoiding interactive privilege changes will significantly increase test reliability.

📂 File Reviews

📄 `tests/applications/gnome-initial-setup/aaa_setup.pm` - Handles test preparations by terminating the current user session, deleting the user, and rebooting.
  • Minor [Suggestion]: Using type_string("reboot\n"); works, but using enter_cmd("reboot"); or standard framework power actions is generally safer and automatically handles newline appending properly.
    • Suggestion: Replace type_string with enter_cmd.
enter_cmd("reboot");
📄 `tests/applications/gnome-initial-setup/gnome_initial_setup_post.pm` - Validates that user selections made in the UI were correctly translated into system and user profile settings.
  • Critical [Bug]: The su command explicitly reads passwords from /dev/tty and ignores standard input (like <<< $password). This will cause the test to hang indefinitely waiting for interactive password entry.
    • Suggestion: Since you already have root access, use standard testing APIs or avoid needing the interactive password test. If testing the password is strict, you'll need to use type_password interactions or sudo -S which can read from stdin.
my $result = script_run("echo $password | sudo -S -u $user whoami | grep -x $user");
  • Major [Logic]: Switching users interactively via enter_cmd("su - $user") and then using script_output can confuse the testing framework's prompt detection (which usually expects a specific root prompt like #).
    • Suggestion: Instead of switching the interactive shell session, wrap the commands directly using su -c executed from the root shell. This maintains the expected root prompt.
sub run_as_user {
    my ($user, $cmd) = @_;
    return script_output("su - $user -c '$cmd'", timeout => 60);
}

Summary

  • Overall Assessment: Critical issues with shell prompt handling and su password injection exist and must be addressed to prevent the post-verification tests from hanging indefinitely.
  • Priority Issues:
    • Fix password input strategy in assert_password because su ignores stdin for passwords, which will stall the execution.
    • Refactor run_as_user to avoid interactive su sessions that break openQA's prompt matching algorithm.
  • Minor Suggestions:
    • Use enter_cmd instead of raw type_string for reboot execution.
    • Add a brief inline comment in utils.pm explaining why loadkezs is used for the Czech layout (z/y swapping) for future maintainers.

🤖 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 PR adds a new comprehensive test suite for the GNOME Initial Setup (GIS), verifying that initial wizard choices (language, layout, privacy toggles, users) are correctly applied to the newly created user session. - **Key Changes:** - Adds `aaa_setup.pm` to clear out the existing user session and force the GIS wizard to trigger on the next boot. - Introduces `gnome_initial_setup.pm` to traverse the GIS UI using specific needle interactions. - Implements `gnome_initial_setup_post.pm` to validate the system and user configurations applied by the GIS via shell commands. - Updates `lib/utils.pm` to accommodate loading the US keyboard layout gracefully when a Czech layout is active. - **Impact:** tests/applications/gnome-initial-setup/aaa_setup.pm, tests/applications/gnome-initial-setup/gnome_initial_setup.pm, tests/applications/gnome-initial-setup/gnome_initial_setup_post.pm, lib/utils.pm, templates.fif.json, check-needles.py - **Risk Level:** 🟡 Medium - The PR introduces extensive shell interactions where prompt detection or terminal behaviors (like `su` password prompts) could lead to unexpected hangs or test timeouts if not handled perfectly. ### Detailed Code Review Overall, the structured approach to verify the Initial Setup flow step-by-step is logical. However, the post-setup verification script relies on interactive terminal session switching (`su - $user`) and injecting passwords via `stdin` to `su`, which historically causes issues with terminal behavior and test framework prompt matching. Consolidating shell executions and avoiding interactive privilege changes will significantly increase test reliability. #### 📂 File Reviews <details> <summary><strong>📄 `tests/applications/gnome-initial-setup/aaa_setup.pm`</strong> - Handles test preparations by terminating the current user session, deleting the user, and rebooting.</summary> - **Minor** [Suggestion]: Using `type_string("reboot\n");` works, but using `enter_cmd("reboot");` or standard framework power actions is generally safer and automatically handles newline appending properly. - *Suggestion:* Replace `type_string` with `enter_cmd`. ```` enter_cmd("reboot"); ```` </details> <details> <summary><strong>📄 `tests/applications/gnome-initial-setup/gnome_initial_setup_post.pm`</strong> - Validates that user selections made in the UI were correctly translated into system and user profile settings.</summary> - **Critical** [Bug]: The `su` command explicitly reads passwords from `/dev/tty` and ignores standard input (like `<<< $password`). This will cause the test to hang indefinitely waiting for interactive password entry. - *Suggestion:* Since you already have root access, use standard testing APIs or avoid needing the interactive password test. If testing the password is strict, you'll need to use `type_password` interactions or `sudo -S` which can read from stdin. ```` my $result = script_run("echo $password | sudo -S -u $user whoami | grep -x $user"); ```` - **Major** [Logic]: Switching users interactively via `enter_cmd("su - $user")` and then using `script_output` can confuse the testing framework's prompt detection (which usually expects a specific root prompt like `#`). - *Suggestion:* Instead of switching the interactive shell session, wrap the commands directly using `su -c` executed from the root shell. This maintains the expected root prompt. ```` sub run_as_user { my ($user, $cmd) = @_; return script_output("su - $user -c '$cmd'", timeout => 60); } ```` </details> ### ✅ Summary - **Overall Assessment:** Critical issues with shell prompt handling and `su` password injection exist and must be addressed to prevent the post-verification tests from hanging indefinitely. - **Priority Issues:** - Fix password input strategy in `assert_password` because `su` ignores stdin for passwords, which will stall the execution. - Refactor `run_as_user` to avoid interactive `su` sessions that break openQA's prompt matching algorithm. - **Minor Suggestions:** - Use `enter_cmd` instead of raw `type_string` for reboot execution. - Add a brief inline comment in `utils.pm` explaining why `loadkezs` is used for the Czech layout (z/y swapping) for future maintainers. --- 🤖 **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

I've rebased this and deployed on staging to see what happens overnight. Please take a look at the AI review's suggestions but only do things that you think make sense.

I've rebased this and deployed on staging to see what happens overnight. Please take a look at the AI review's suggestions but only do things that you think make sense.
Use enter_cmd
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 2m39s
CI via Tox and perl / perl (pull_request) Successful in 5m0s
CI via Tox and perl / checkwiki (pull_request) Successful in 1m5s
6fd979edaa
Make fixes as requested by AI
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
92fbb0ca56
Comment inline
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m44s
CI via Tox and perl / perl (pull_request) Has been cancelled
CI via Tox and perl / checkwiki (pull_request) Has been cancelled
cedbfc8f07
lruzicka force-pushed newtest/gnome-initial-setup from cedbfc8f07
Some checks failed
CI via Tox and perl / tox (pull_request) Successful in 1m44s
CI via Tox and perl / perl (pull_request) Has been cancelled
CI via Tox and perl / checkwiki (pull_request) Has been cancelled
to 28ed6961f5
All checks were successful
CI via Tox and perl / checkwiki (pull_request) Successful in 1m6s
AI Code Review / ai-review (pull_request_target) Successful in 28s
CI via Tox and perl / tox (pull_request) Successful in 4m42s
CI via Tox and perl / perl (pull_request) Successful in 4m3s
2026-04-29 09:23:41 +00:00
Compare

AI Code Review

📋 MR Summary

Adds a new test suite to verify GNOME Initial Setup functionality by simulating user flow and validating the resulting system state.

  • Key Changes:
    • Created aaa_setup.pm to reset the user session and trigger GNOME Initial Setup on reboot.
    • Added gnome_initial_setup.pm to drive the GUI interactions through the initial setup wizard.
    • Introduced gnome_initial_setup_post.pm to verify system and user settings applied by the setup.
    • Updated lib/utils.pm to handle Czech keyboard layout mapping.
  • Impact: tests/applications/gnome-initial-setup/aaa_setup.pm, tests/applications/gnome-initial-setup/gnome_initial_setup.pm, tests/applications/gnome-initial-setup/gnome_initial_setup_post.pm, lib/utils.pm, templates.fif.json, check-needles.py
  • Risk Level: 🟡 Medium - Adds a new automated UI test suite with complex state verification and user session management, which can introduce flakiness if the UI interacts unexpectedly or environment preconditions fail.

Detailed Code Review

The implementation generally meets the intended goal of covering the GNOME Initial Setup flow. However, there are potential issues with how specific commands are asserted or parsed, particularly when verifying the user's password using sudo and managing list filtering. Additionally, the approach to checking language states post-setup has hardcoded environment variables that could cause silent failures if the system outputs unexpected strings. The structure looks well segmented into setup, execution, and verification steps.

📂 File Reviews

📄 `tests/applications/gnome-initial-setup/gnome_initial_setup.pm` - Drives the GNOME Initial Setup wizard using predefined UI elements.
  • Major [Bug]: In handle_privacy, the code uses check_screen with a timeout of 2 seconds to check if a setting is enabled. However, if the screen doesn't match immediately due to slight rendering delays, it logs a failure but continues execution, potentially clicking the wrong UI elements or failing subsequent checks.

    • Suggestion: Increase the timeout or ensure the baseline screen is fully loaded before checking individual elements.
  • Minor [Logic]: The logic to handle required screens in run builds an array qw(welcome privacy timezone credentials password) and optionally adds 'software'. It later checks against visited_screens. If the test breaks out of the loop early on 'final', it correctly reports missing screens. However, if get_screen fails to match anything, the script dies immediately and the missing screens logic is bypassed entirely.

📄 `tests/applications/gnome-initial-setup/gnome_initial_setup_post.pm` - Validates the system and user configurations applied during GNOME Initial Setup.
  • Major [Security]: The assert_password function echoes the password directly into a sudo command without properly sanitizing or quoting the variables. If $password or $user contained shell metacharacters, it could result in command injection.
    • Suggestion: Use proper quoting for shell variables when executing commands via script_run.
my $result = script_run("echo '". $password ."' | sudo -S -u ". $user ." whoami | grep -x ". $user);
  • Major [Bug]: The grep -x command requires an exact match for the whole line. The output of localectl status might have trailing spaces or slightly different formatting across versions, making the strict regex grep -x 'LANG=cs_CZ.UTF-8' /etc/locale.conf prone to failure.
    • Suggestion: Use a more flexible grep pattern or use validate_script_output.
assert_script_run("grep -E '^LANG=cs_CZ\\.UTF-8' /etc/locale.conf");

Summary

  • Overall Assessment: The code implements the intended functionality but contains security and stability issues in the post-setup verification script.
  • Priority Issues:
    • Fix command injection vulnerability in assert_password by quoting shell variables.
    • Relax strict grep -x matching when parsing configuration files and command outputs.
  • Minor Suggestions:
    • Consider increasing the timeout on privacy toggle checks in the setup script.

🤖 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 test suite to verify GNOME Initial Setup functionality by simulating user flow and validating the resulting system state. - **Key Changes:** - Created `aaa_setup.pm` to reset the user session and trigger GNOME Initial Setup on reboot. - Added `gnome_initial_setup.pm` to drive the GUI interactions through the initial setup wizard. - Introduced `gnome_initial_setup_post.pm` to verify system and user settings applied by the setup. - Updated `lib/utils.pm` to handle Czech keyboard layout mapping. - **Impact:** tests/applications/gnome-initial-setup/aaa_setup.pm, tests/applications/gnome-initial-setup/gnome_initial_setup.pm, tests/applications/gnome-initial-setup/gnome_initial_setup_post.pm, lib/utils.pm, templates.fif.json, check-needles.py - **Risk Level:** 🟡 Medium - Adds a new automated UI test suite with complex state verification and user session management, which can introduce flakiness if the UI interacts unexpectedly or environment preconditions fail. ### Detailed Code Review The implementation generally meets the intended goal of covering the GNOME Initial Setup flow. However, there are potential issues with how specific commands are asserted or parsed, particularly when verifying the user's password using `sudo` and managing list filtering. Additionally, the approach to checking language states post-setup has hardcoded environment variables that could cause silent failures if the system outputs unexpected strings. The structure looks well segmented into setup, execution, and verification steps. #### 📂 File Reviews <details> <summary><strong>📄 `tests/applications/gnome-initial-setup/gnome_initial_setup.pm`</strong> - Drives the GNOME Initial Setup wizard using predefined UI elements.</summary> - **Major** [Bug]: In `handle_privacy`, the code uses `check_screen` with a timeout of 2 seconds to check if a setting is enabled. However, if the screen doesn't match immediately due to slight rendering delays, it logs a failure but continues execution, potentially clicking the wrong UI elements or failing subsequent checks. - *Suggestion:* Increase the timeout or ensure the baseline screen is fully loaded before checking individual elements. - **Minor** [Logic]: The logic to handle required screens in `run` builds an array `qw(welcome privacy timezone credentials password)` and optionally adds 'software'. It later checks against `visited_screens`. If the test breaks out of the loop early on 'final', it correctly reports missing screens. However, if `get_screen` fails to match anything, the script dies immediately and the missing screens logic is bypassed entirely. </details> <details> <summary><strong>📄 `tests/applications/gnome-initial-setup/gnome_initial_setup_post.pm`</strong> - Validates the system and user configurations applied during GNOME Initial Setup.</summary> - **Major** [Security]: The `assert_password` function echoes the password directly into a `sudo` command without properly sanitizing or quoting the variables. If `$password` or `$user` contained shell metacharacters, it could result in command injection. - *Suggestion:* Use proper quoting for shell variables when executing commands via `script_run`. ```` my $result = script_run("echo '". $password ."' | sudo -S -u ". $user ." whoami | grep -x ". $user); ```` - **Major** [Bug]: The `grep -x` command requires an exact match for the whole line. The output of `localectl status` might have trailing spaces or slightly different formatting across versions, making the strict regex `grep -x 'LANG=cs_CZ.UTF-8' /etc/locale.conf` prone to failure. - *Suggestion:* Use a more flexible grep pattern or use `validate_script_output`. ```` assert_script_run("grep -E '^LANG=cs_CZ\\.UTF-8' /etc/locale.conf"); ```` </details> ### ✅ Summary - **Overall Assessment:** The code implements the intended functionality but contains security and stability issues in the post-setup verification script. - **Priority Issues:** - Fix command injection vulnerability in `assert_password` by quoting shell variables. - Relax strict `grep -x` matching when parsing configuration files and command outputs. - **Minor Suggestions:** - Consider increasing the timeout on privacy toggle checks in the setup script. --- 🤖 **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

The AI review is doing stuff in the circle. I would refrain from listening to it any more. We want to check for exact strings, I think. Also, I do not know why AI would not report this earlier if that was so severe. By the way the grep -x check was suggested as improvement by the same AI.

I will only bump the check_screen timeout just to be on the safe side.

The AI review is doing stuff in the circle. I would refrain from listening to it any more. We want to check for exact strings, I think. Also, I do not know why AI would not report this earlier if that was so severe. By the way the `grep -x` check was suggested as improvement by the same AI. I will only bump the `check_screen` timeout just to be on the safe side.
lruzicka force-pushed newtest/gnome-initial-setup from 28ed6961f5
All checks were successful
CI via Tox and perl / checkwiki (pull_request) Successful in 1m6s
AI Code Review / ai-review (pull_request_target) Successful in 28s
CI via Tox and perl / tox (pull_request) Successful in 4m42s
CI via Tox and perl / perl (pull_request) Successful in 4m3s
to a145cda3b9
All checks were successful
CI via Tox and perl / checkwiki (pull_request) Successful in 39s
CI via Tox and perl / tox (pull_request) Successful in 4m26s
CI via Tox and perl / perl (pull_request) Successful in 3m34s
2026-04-29 09:57:08 +00:00
Compare
Owner

As I said, feel free to ignore things the AI says that don't make sense. Use your judgment :D Making it happy isn't a requirement. It's just an input.

As I said, feel free to ignore things the AI says that don't make sense. Use your judgment :D Making it happy isn't a requirement. It's just an input.
adamwill force-pushed newtest/gnome-initial-setup from a145cda3b9
All checks were successful
CI via Tox and perl / checkwiki (pull_request) Successful in 39s
CI via Tox and perl / tox (pull_request) Successful in 4m26s
CI via Tox and perl / perl (pull_request) Successful in 3m34s
to 6aaf2fe58d
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 6m28s
CI via Tox and perl / perl (pull_request) Successful in 3m42s
CI via Tox and perl / checkwiki (pull_request) Successful in 1m53s
2026-04-30 19:02:06 +00:00
Compare
Owner

It looks like it now fails on Workstation because abrt is gone from Workstation too now.

It looks like it now fails on Workstation because abrt is [gone from Workstation too](https://pagure.io/fedora-comps/c/09d12457bb09ee5a6822c612f7b24655b4f1ee90?branch=main) now.
Drop abrt handling in privacy page entirely
Some checks failed
CI via Tox and perl / tox (pull_request) Failing after 1m42s
CI via Tox and perl / perl (pull_request) Successful in 3m55s
CI via Tox and perl / checkwiki (pull_request) Successful in 1m12s
9122affa2c
abrt has been removed from Workstation now, so let's just not
expect it here at all.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
adamwill force-pushed newtest/gnome-initial-setup from 9122affa2c
Some checks failed
CI via Tox and perl / tox (pull_request) Failing after 1m42s
CI via Tox and perl / perl (pull_request) Successful in 3m55s
CI via Tox and perl / checkwiki (pull_request) Successful in 1m12s
to d02ad52c7d
Some checks failed
CI via Tox and perl / tox (pull_request) Failing after 1m50s
CI via Tox and perl / perl (pull_request) Successful in 3m42s
CI via Tox and perl / checkwiki (pull_request) Successful in 54s
2026-04-30 20:01:59 +00:00
Compare
Owner

I fixed that. But now I'm noticing something else. It seems like we have duplicated versions of a lot of the needles - one in English, one in Czech. On further review this seems to be because we get the language selection screen on Silverblue, so we can configure Czech, but on Workstation we don't, so we are stuck in English.

I think the fact that the language and layout pages are shown in Silverblue may be unintended, so I've filed https://github.com/fedora-silverblue/issue-tracker/issues/700 . If that gets changed, we might have to reconsider this further. Do we try to force the appearance of the language and layout pages so we can test them, or do we say it's fine if they're broken as we can't encounter them on any typical Fedora install path any more? Do we still want to test the rest of g-i-s in Czech or is testing in English OK?

Also, @catanzaro says he's going to re-introduce live mode g-i-s soon, so we have that to look forward to.

I fixed that. But now I'm noticing something else. It seems like we have duplicated versions of a lot of the needles - one in English, one in Czech. On further review this seems to be because we get the language selection screen on Silverblue, so we can configure Czech, but on Workstation we don't, so we are stuck in English. I think the fact that the language and layout pages are shown in Silverblue may be unintended, so I've filed https://github.com/fedora-silverblue/issue-tracker/issues/700 . If that gets changed, we might have to reconsider this further. Do we try to force the appearance of the language and layout pages so we can test them, or do we say it's fine if they're broken as we can't encounter them on any typical Fedora install path any more? Do we still want to test the rest of g-i-s in Czech or is testing in English OK? Also, @catanzaro says he's going to re-introduce live mode g-i-s soon, so we have that to look forward to.
First-time contributor

Also, @catanzaro says he's going to re-introduce live mode g-i-s soon, so we have that to look forward to.

Notably, that's where language and keyboard layout selection will move to. So your tests will need to be reworked, but maybe don't completely delete them.

> Also, @catanzaro says he's going to re-introduce live mode g-i-s soon, so we have that to look forward to. Notably, that's where language and keyboard layout selection will move to. So your tests will need to be reworked, but maybe don't completely delete them.
lruzicka force-pushed newtest/gnome-initial-setup from d02ad52c7d
Some checks failed
CI via Tox and perl / tox (pull_request) Failing after 1m50s
CI via Tox and perl / perl (pull_request) Successful in 3m42s
CI via Tox and perl / checkwiki (pull_request) Successful in 54s
to aa5ba3ba1b
Some checks failed
CI via Tox and perl / tox (pull_request) Failing after 1m30s
CI via Tox and perl / perl (pull_request) Successful in 3m22s
CI via Tox and perl / checkwiki (pull_request) Successful in 35s
2026-05-04 10:41:06 +00:00
Compare
adamwill force-pushed newtest/gnome-initial-setup from aa5ba3ba1b
Some checks failed
CI via Tox and perl / tox (pull_request) Failing after 1m30s
CI via Tox and perl / perl (pull_request) Successful in 3m22s
CI via Tox and perl / checkwiki (pull_request) Successful in 35s
to 951618b9c0
Some checks failed
CI via Tox and perl / checkwiki (pull_request) Successful in 35s
CI via Tox and perl / tox (pull_request) Failing after 1m26s
CI via Tox and perl / perl (pull_request) Successful in 3m18s
2026-05-04 22:59:31 +00:00
Compare
adamwill force-pushed newtest/gnome-initial-setup from 951618b9c0
Some checks failed
CI via Tox and perl / checkwiki (pull_request) Successful in 35s
CI via Tox and perl / tox (pull_request) Failing after 1m26s
CI via Tox and perl / perl (pull_request) Successful in 3m18s
to bc098e5b57
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 3m16s
CI via Tox and perl / checkwiki (pull_request) Successful in 35s
2026-05-04 23:05:40 +00:00
Compare
adamwill force-pushed newtest/gnome-initial-setup from bc098e5b57
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 3m16s
CI via Tox and perl / checkwiki (pull_request) Successful in 35s
to e37761af0d
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 2m26s
CI via Tox and perl / perl (pull_request) Successful in 4m47s
CI via Tox and perl / checkwiki (pull_request) Successful in 33s
2026-05-08 07:02:13 +00:00
Compare
adamwill force-pushed newtest/gnome-initial-setup from e37761af0d
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 2m26s
CI via Tox and perl / perl (pull_request) Successful in 4m47s
CI via Tox and perl / checkwiki (pull_request) Successful in 33s
to e2599185d3
All checks were successful
CI via Tox and perl / tox (pull_request) Successful in 1m22s
CI via Tox and perl / perl (pull_request) Successful in 3m35s
CI via Tox and perl / checkwiki (pull_request) Successful in 30s
2026-05-11 22:30:52 +00:00
Compare
Owner

It seems it's gonna stay this way for at least a bit, so I think we can probably merge this as-is and refine it later.

It seems it's gonna stay this way for at least a bit, so I think we can probably merge this as-is and refine it later.
adamwill deleted branch newtest/gnome-initial-setup 2026-05-11 23:55:07 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
4 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!517
No description provided.