test: add a check that clicking add multiple times when adding network interface #1542
Conversation
|
Moving to draft while the test is still failing. Please add some description to the commit message and/or a comment what this should actually test. Thanks! |
yunmingyang
force-pushed
the
testClickAddMultipleTimes
branch
3 times, most recently
from
d6176db
to
a4bc664
Compare
test/check-machines-nics
Outdated
| self.browser.click("#vm-subVmTest1-add-iface-add") | ||
| self.browser.click("#vm-subVmTest1-add-iface-add") | ||
| self.browser.click("#vm-subVmTest1-add-iface-add") | ||
| except testlib.Error as e: | ||
| if not e.msg.startswith('timeout'): | ||
| raise |
There was a problem hiding this comment.
Sorry, this is too unreliable, and it also doesn't say what it actually expects. I don't know exactly what the behaviour should be, but AFAIU #819, the Add button now becomes disasbled after clicking on it. If it stays disabled indefinitely (or at least longer than 5 seconds or so), then please instead do
self.browser.wait_visible("#vm-subVmTest1-add-iface-add:disabled")
Doing three clicks without any wait in between doesn't make sense -- this is async, so this doesn't even give the React code time to actually re-render and disable the button.
If it only gets disabled quickly while the operation is in progress, then the disabled state is not testable, and all three add clicks are actually supposed to work. The validation below seems to indicate that you always expect this to work exactly once?
There was a problem hiding this comment.
Thanks, I will add comments if I finish something in the future.
The three click is about the reproducing steps for the original issue - https://bugzilla.redhat.com/show_bug.cgi?id=2129845. In the issue, click the "Add" button for multiple times during 1 ~ 2 seconds, then there will be multiple network interface attached, according that, I think the adding events should be sent and handled successfully although it is async for doing three clicks without any wait. So these steps here are copying the three click behavior to ensure that there should only be one network interface attached if clicking the "Add" button for multiple times. Not sure whether it makes sense. If it doesn't, I will change these steps. @martinpitt
There was a problem hiding this comment.
Yes, I understand the original bug report, but this test is still too under-defined. As I wrote above, this is broken in multiple ways:
- It does not reproduce the original bug. After reverting commit 3ef98df the test still passes. That's because the clicking happens way too fast, before React can, well, react to it.
- It does not test the expected behaviour after the fix: the button is expected to get disabled after clicking
- The indirection via expecting the timeout is very confusing. What happens is that the first click works, and closes the dialog. The second click then waits until the
-addelement is clickable, which isn't the case as the button first gets disabled, and then dialog just goes away and the button with it. So it will always wait in vain for 15 seconds.
I'm afraid there's simply no reliable way of testing this condition.
There was a problem hiding this comment.
Thanks a lot, I will modify it to verify the button is disabled
There was a problem hiding this comment.
No, you can't really -- this is only taking a very short time, so it's fundamentally racy. As I said, there is no way to test this reliably and properly. I'm afraid this PR can just be closed 😢
There was a problem hiding this comment.
Oh, although there are some difference with clicking multiple times, I think checking the button is disabled should also be needed for the test, as users could not be as fast as the code operations, check the button disabled could make sure users could not add a another unexpected NIC during the dialog is not closed. Do you think that make scene? @martinpitt
yunmingyang
force-pushed
the
testClickAddMultipleTimes
branch
from
a4bc664
to
20543d3
Compare
yunmingyang
force-pushed
the
testClickAddMultipleTimes
branch
from
20543d3
to
b730000
Compare
|
Close according to the discussion |
No description provided.