Extend navigation test to cover KDE #566
No reviewers
adamwill
Labels
No labels
Closed As
Duplicate
Closed As
Fixed
Closed As
Invalid
easyfix
freeipa
improvetest
iot
newtest
silverblue
tracker
ai-review-please
Backlog Status
Needs Review
Backlog Status
Ready
chore
documentation
points
01
points
02
points
03
points
05
points
08
points
13
pr2jira
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Sprint Status
Blocked
Sprint Status
Done
Sprint Status
In Progress
Sprint Status
Review
Sprint Status
To Do
Technical Debt
Work Item
Bug
Work Item
Epic
Work Item
Spike
Work Item
Task
Work Item
User Story
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
quality/os-autoinst-distri-fedora!566
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "enhance/kde-navigation"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Enable the navigation test suite for KDE and adapt the affected tests to
use KDE applications, shortcuts, workspace handling, video player dialogs,
and window controls. Add the required KDE needles, include System Monitor
in app startup checks, and support KDE update notification timestamps via
Discover notifier config.
Fixes: #322
AI Code Review
Detailed Code Review
The PR successfully extends the navigation test suite to cover KDE by abstracting desktop-specific behaviors, shortcuts, and application names. The author has thoughtfully mapped GNOME concepts to their KDE equivalents (e.g., using
super-pgdnfor minimizing instead ofsuper-h, adjusting workspace shortcuts, and using KRunner for configuration changes).The logic changes are sound, and the addition of KDE-specific needles ensures the tests can effectively interact with the KDE environment. One minor styling issue was spotted regarding indentation (the use of tabs instead of spaces), but functionally the code is well-implemented and maintains compatibility with the existing GNOME tests.
✅ Summary
switch_workplaces.pmto maintain code consistency.🤖 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.
fe878f13e0ddd00b3127ddd00b312730d36c4fa7I fixed the space/tab mismatch.
I see several issues here, please take a look. Thanks for the work!
@ -285,6 +286,12 @@ testtags.append("mwriter_mediagroup_official")# For gnome-initial-setupfor screen in ("credentials", "final", "keyboard", "password", "privacy", "software", "timezone"):testtags.append(f"gis_screen_{screen}")# For KDE navigationI think it'd be nice to put this section next to the GNOME navigation section, rather than down here at the bottom.
@ -288,0 +289,4 @@# For KDE navigationfor app in ("kcalc", "konsole", "dolphin", "kwrite", "system-monitor"):testtags.append(f"navigation_navibar_{app}")testtags.append(f"{app}_inactive")we only actually have dolphin_inactive and kwrite_inactive. Also, why does this PR remove the files_inactive and editor_inactive needles for GNOME? Surely these are still needed for the GNOME path? The
switch_applicationtest code looks like it will still assert them, for GNOME. If so, we need to add the needles back, and add exceptions for them to the GNOME section here in check-needles (since they're now asserted indirectly via a variable, not directly by name).@ -288,0 +290,4 @@for app in ("kcalc", "konsole", "dolphin", "kwrite", "system-monitor"):testtags.append(f"navigation_navibar_{app}")testtags.append(f"{app}_inactive")testtags.append(f"navigation_{app}_fullscreen")We only actually have navigation_konsole_fullscreen, we don't need the others.
@ -288,0 +291,4 @@testtags.append(f"navigation_navibar_{app}")testtags.append(f"{app}_inactive")testtags.append(f"navigation_{app}_fullscreen")testtags.append(f"{app}_maximized")We only have kcalc_maximized, we don't need the others.
@ -1965,0 +1966,4 @@type_very_safely("gsettings set org.gnome.software update-notification-timestamp $ep_time\n");}elsif ($desktop eq "kde") {type_very_safely("kwriteconfig6 --file ~/.local/state/discovernotifierstaterc --group Global --key LastNotificationTime $ep_time\n");this likely affects other tests too; should we check it doesn't cause unexpected consequences in any of them? it should be fine, but...
I checked and out of 17 files that call this subroutine, only one is KDE-related and it is this one.
This file has an explicit KDE branch, so it should not interfere with other tests.
@ -0,0 +10,4 @@],"properties": [],"tags": ["apps_run_system-monitor"we already have apps_run_systemmonitor . just add a tag to that, or tweak the invocation so it matches both here and in apps_startstop and they can both use the same needle tag?
Originally, I did not know how to make a needle automatically through menu_launch_type when it needed to words to invocate, because it would also produce a needle name with a space in it. That did not work. When hyphen is used, the two words still work and the needle name works correctly, too.
I changed the invocation in the startstop test, so this needle should now match both. I also added an
apps_run_systemmonitortag to it, just to be sure. For Gnome, we have been usingapps_run_monitorfor some time, so I have deleted theapps_run_system-monitorfor Gnome.@ -0,0 +10,4 @@],"properties": [],"tags": ["apps_run_video"It's interesting that we need to add this. It looks like 'video' runs Dragon Player, and we don't have that in apps_startstop. We should probably add it. Doesn't need to be part of this PR, though.
If
videois enough to start Dragon Player, we could keep it, and I will add it to the start/stop test in a separate commit as per #567@ -0,0 +10,4 @@],"properties": [],"tags": ["calculator_select_hidden"this needle doesn't exactly match the GNOME needle; the GNOME needle appears to match on part of the app's UI, this looks like it's matching on the app's entry in the bottom bar. Are we testing hiding the app from the desktop or hiding it from the app list here? Shouldn't the assertions match across the two desktops?
That seems to be correct. The code says
ergo it clicks on the hidden applicatino to unhide it. In Gnome, I can do it via Activities, so it is ok that it clicks the application. In KDE however, I have to click on the panel icon to unhide it.
@ -0,0 +24,4 @@],"properties": [],"tags": ["calculator_tiled_right"why do we have a 20260601 and a 20260602 needle here? are they dupes? can we lose one?
The older needle worked on one day and failed on the other. I am deleting the older one and we'll see how that goes.
@ -0,0 +10,4 @@],"properties": [],"tags": ["kde_filedialogue_video_select"this and 20260528 are essentially the same except the filename has a weird blue underline in this one, right? can we not just have one needle that doesn't include the area where the blue underline appears, so it matches both cases?
@ -0,0 +17,4 @@],"properties": [],"tags": ["navigation_navibar_dolphin"it looks like we need two versions of all these navibar needles because of transparency, but - can we be strategic and match only on completely solid areas? e.g. the app icons; those should be the same no matter what's behind the switcher UI?
Unfortunately, we can't. In KDE, the active item on the list is only distinguished by the bluish frame around it. It is not shaded (as it is in Gnome), so any attempt to match just the solid part ended up in a wrong item being expected. So I decided to match on the upper right corner of the item, where we also have a closing icon, then the solid application picture and a little of the bluish frame.
I deleted on of those needles and update the other one to match on the upper right corner.
@ -0,0 +10,4 @@],"properties": [],"tags": ["video_fullscreen_on"why do we need both this and the 20260528 one? why are the filenames different?
The filenames are a mistake, but we have three needles for this: two Gnome needles and one KDE needle. I believe that the Gnome needles do not match on KDE, and that the original one also stopped matching on Gnome some time before July 10th, 2024.
Renaming the KDE needle.
@ -0,0 +10,4 @@],"properties": [],"tags": ["video_second_stop"why do we need three different video_second_stop needles? can their filenames please be consistent?
KDE is very unreliable in running the video, so it occassionally stops at a different place. I unified the names of the KDE needles.
@ -0,0 +12,4 @@"tags": ["LANGUAGE-english","anaconda_select_install_lang_filtered","anaconda_select_install_lang_selected"why this needle in this test? does this test ever even run the installer?
Copy error. Deleting.
@ -0,0 +24,4 @@],"properties": [],"tags": ["calculator_maximized"the filename for this needle is inconsistent with all the other calculator needles.
@ -38,0 +46,4 @@assert_and_click('kde_add_desktop');assert_and_click('kde_add_desktop');assert_and_click('kde_button_apply');send_key("alt-f4");nitpick throughout: I think we should be trying to standardize on single quotes for strings with no variables, double quotes for strings with variables. in new code at least.
@ -15,3 +21,1 @@assert_screen("apps_run_texteditor");menu_launch_type("files", maximize => 1);assert_screen("apps_run_files");menu_launch_type($editor, maximize => 1, checkstart => 1);there does not appear to be a
apps_run_text-editorneedle, so I think this breaks GNOME, does it not? why not just domy $editor = "texteditor"above, to keep the GNOME behavior the same as it was before?texteditordoes not find the application in the Activities mode.text-editordoes. I have added the correct tag to the existing needle, so it won't break@ -21,2 +25,3 @@if (check_screen("apps_run_$manager")) {send_key("alt-tab");assert_screen "apps_run_texteditor";assert_screen "apps_run_$editor";ditto above note. this is broken unless we change tags or change the value of
$editor.@ -55,0 +64,4 @@$calculator = "kcalc";}menu_launch_type($manager, checkstart => 1);menu_launch_type($editor, checkstart => 1);per earlier note, this is going to break GNOME because it will expect an
apps_run_text-editorneedle that does not exist.@ -61,3 +74,3 @@# Switch backsend_key("alt-tab");assert_screen("apps_run_texteditor");assert_screen("apps_run_$editor");...and again.
@ -73,0 +82,4 @@assert_and_click("${manager}_inactive");assert_screen("apps_run_$manager");assert_and_click("${editor}_inactive");assert_screen("apps_run_$editor");and again, and note there's two variants here. The previous names are
editor_inactiveandapps_run_texteditor. neither of those is the same as the value of${editor}, which istext-editor.Tag added.
@ -105,3 +116,3 @@# Switch to Editorswitch_to_app("texteditor", "forward");switch_to_app($editor, "forward");...and yet another case.
@ -111,3 +122,3 @@# Switch to Editorswitch_to_app("texteditor", "forward");switch_to_app($editor, "forward");and again.
@ -118,2 +130,3 @@send_key("$combo");# Check that the application has minimised.check_hidden("texteditor");check_hidden($editor);and again.
@ -127,3 +140,3 @@# Switch to Editorswitch_to_app("texteditor", "forward");switch_to_app($editor, "forward");and again.
@ -34,0 +48,4 @@menu_launch_type("$files", maximize => 1);assert_screen("apps_run_$files");menu_launch_type("$editor", maximize => 1);assert_screen("apps_run_$editor");for some reason you didn't use
checkstarthere as you did in other files, but you still have the problem with the needle tags for the editor on GNOME, because you changed it fromtexteditortotext-editor. And in this file - but not the others - you changed "files" to "nautilus" which may also cause problems - theapps_run_filesneedles have both tags (apps_run_filesandapps_run_nautilus), but other needles likefiles_inactivedo not.Changed nautilus back to files.
@ -25,0 +30,4 @@$up = "pgup";$down = "pgdn";}$calculator = "kcalc" if ($desktop eq "kde");er, why not just put this in the block immediately above?
37118f2a5da3bf40f0f0174429c23a93990bf59a93990bf59a27f04e4368AI Code Review
Detailed Code Review
The PR accomplishes the goal of integrating KDE into the navigation test suite. The implementation cleanly distinguishes between GNOME and KDE applications, adjusts assertions dynamically via variables (
$editor,$manager, etc.), and updates needles properly. Author corrections such as preserving GNOME compatibility with thetext-editornaming and using-(hyphens) in names likesystem-monitorhave been handled successfully. The code formatting has been consistently standardized across files.📂 File Reviews
📄 `lib/utils.pm` - Added logic for setting the discover notification timestamp for KDE.
✅ Summary
$desktopchecks across all PM files to use a unified helper function inutils.pmfor evaluating desktop-specific app names to reduce duplicated lookup blocks.🤖 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.
Sorry, I kinda lost track of this one. I've rebased it and deployed it on staging and am doing a full compose run to check the impacts. If that looks good, I'll merge. Also need to check if it relies on any uncommitted needles.
7da89b4e1a38a7e0ddf438a7e0ddf4373d2e3d88373d2e3d88796d7e2b7e796d7e2b7e62e9b3119662e9b311961b013789e21b013789e21768719ddb1768719ddb934ed8a8db934ed8a8dbaea89141e1aea89141e1740dd7927b740dd7927b55660af64aI think this is good enough now, after I tweaked a few things. Let's go ahead and merge it. Thanks.