Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Makefile: Update Cockpit lib to e8cf93050356f; fix adding disks to transient VM, and lots of test bugs #1778

Merged
merged 5 commits into from
Aug 23, 2024

Conversation

github-actions[bot]
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the bot label Aug 22, 2024
@github-actions github-actions bot changed the title [no-test] Makefile: Update Cockpit lib to e8cf93050356f3cecdc229f170e5c4a7 Makefile: Update Cockpit lib to e8cf93050356f3cecdc229f170e5c4a7 Aug 22, 2024
@allisonkarlitskaya allisonkarlitskaya force-pushed the cockpit-lib-update-cockpit-lib-20240822-023242 branch from cf2069d to d0b5402 Compare August 22, 2024 02:32
@martinpitt
Copy link
Member

The TestMachinesLifecycle.testRename regression looks real. Checking..

@martinpitt martinpitt self-assigned this Aug 22, 2024
@martinpitt
Copy link
Member

Most failures are shallow (unclosed dialogs and such). TestMachinesDisks.testAddDiskDirPool is much harder: It's a race condition which happens to work most of the time with Chromium, but if you wait/sit after the virsh undefine subVmTest1 it fails as well. Setting permanent=False in the self.VMAddDiskDialog() call there is wrong, as the dialog does not have a "persistent" checkbox (as we just made the VM transient), and the next step even checks that. This needs to be rewired, but that's obvious.

What's not obvious is how to properly wait for the virsh undefine to take effect, i.e. bubble to the UI. There is no DOM change on that. I'll introduce some (invisible) attribute or class or so, so that the test has something to wait on.

@martinpitt martinpitt marked this pull request as draft August 22, 2024 09:01
@martinpitt martinpitt force-pushed the cockpit-lib-update-cockpit-lib-20240822-023242 branch from d0b5402 to 5960bb9 Compare August 22, 2024 09:27
@martinpitt
Copy link
Member

Not done yet, but pushing what I have as I need to run out for a bit.

Selecting a new action from the main page while a dialog is open only
worked with our `ph_mouse()` cheat, not with proper clicks.
Otherwise the list obscures the dialog's action buttons, so they are not
clickable (only with `ph_mouse()` which circumvents the browser).
@martinpitt martinpitt force-pushed the cockpit-lib-update-cockpit-lib-20240822-023242 branch from 5960bb9 to e794803 Compare August 22, 2024 12:51
@martinpitt
Copy link
Member

Meh, this is a completely stupid failure:

# test/machineslib.py:353: error: "VirtualMachinesCaseHelpers" has no attribute "browser"  [attr-defined]

That's nonsense -- plenty of other methods in the same class use self.browser. /me just adds an override

The disk kebabs keep jumping around while the Disk table initializes
and re-renders, which is a race condition when trying to click on them
with the actual browser mouse.

So robustify the tests to wait for the table to be complete. This
mitigates the issue, and also provides some nice extra validation. This
was already done in a few places, generalize this into a `waitDisks()`
helper.

Unfortunately in some cases waiting for the entries is still not enough,
but I can't figure out what else to wait on.. Do an extra sleep for the
table to settle down.
@martinpitt martinpitt force-pushed the cockpit-lib-update-cockpit-lib-20240822-023242 branch from e794803 to cce3bc3 Compare August 22, 2024 13:00
@martinpitt martinpitt force-pushed the cockpit-lib-update-cockpit-lib-20240822-023242 branch from cce3bc3 to a07c76a Compare August 22, 2024 14:42
@martinpitt
Copy link
Member

phew I figured out testAddDiskDirPool. That was an interesting chain of test race hiding test bug hiding code bug. I just saw it was even reported: https://issues.redhat.com/browse/RHEL-17677

martinpitt and others added 2 commits August 22, 2024 16:46
TestMachinesDisks.testAddDiskDirPool's "transient VM" test case
ommediately opened the "Add Disk" dialog after undefining the VM. This
was a race condition which the test actually "won" in most cases: The
`UNDEFINED` event did not yet bubble through the UI, so the "Add Disk"
dialog still thought that the VM was persistent. Fix that by waiting
until the UI picked up the event and considers the VM transient.

This exposed a bug in the dialog's fill() function: It tried to check
the value of the "Permanent" checkbox, but that doesn't exist at all for
transient VMs. Fix up the logic and assert the latter for transient VMs.

Now that this test is working correctly, it uncovered a bug in the code:
Trying to add a disk to a transient VM fails with

> Requested operation is not valid: transient domains do not have any persistent config

because the `persistent` state was initialized to "true". Fix that.

https://issues.redhat.com/browse/RHEL-17677
@martinpitt martinpitt force-pushed the cockpit-lib-update-cockpit-lib-20240822-023242 branch from a07c76a to 504c26b Compare August 22, 2024 14:46
@martinpitt martinpitt changed the title Makefile: Update Cockpit lib to e8cf93050356f3cecdc229f170e5c4a7 Makefile: Update Cockpit lib to e8cf93050356f; fix adding disks to transient VM, and lots of test races Aug 22, 2024
@martinpitt martinpitt changed the title Makefile: Update Cockpit lib to e8cf93050356f; fix adding disks to transient VM, and lots of test races Makefile: Update Cockpit lib to e8cf93050356f; fix adding disks to transient VM, and lots of test bugs Aug 22, 2024
@martinpitt martinpitt marked this pull request as ready for review August 22, 2024 15:21
Copy link
Collaborator

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

Thanks, all changes make sense.

@martinpitt martinpitt merged commit c050b20 into main Aug 23, 2024
26 checks passed
@martinpitt martinpitt deleted the cockpit-lib-update-cockpit-lib-20240822-023242 branch August 23, 2024 04:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants