test: Clean up keyboard input API #20563
Conversation
martinpitt
added
the
no-test
|
This is the hard bit. The remaining one is to rename |
martinpitt
removed
the
no-test
martinpitt
force-pushed
the
test-key
branch
2 times, most recently
from
d12eecf to
dd46ce5
Compare
There was a problem hiding this comment.
This is really cool! I expected that this would impact more of our tests and the reason why I never considered it a reasonable refactor. (Too much churn)
But this new API is nicer and higher level and a bit more explicit with input_text, I like it.
I made two small notes, feel free to address or ignore. Another thing which came to mind is that maybe adding a docstring to the new functions is worth the effort.
| "Escape": 27, | ||
| "ArrowDown": 40, | ||
| "Insert": 45, | ||
| } |
There was a problem hiding this comment.
Interesting, its a bit different from https://github.com/cockpit-project/cockpit-files/blob/main/test/check-application#L47
So it would be nice to add ArrowUp/ArrowLeft/ArrowRight if that is still needed
There was a problem hiding this comment.
That's what I hinted at in the commit message: c-storage-partitions already uses ArrowLeft and that seems to be fine. I don't see any pattern here yet. But I expect some more fallout from this indeed, we may have to adjust that a bit in the future still.
There was a problem hiding this comment.
I locally tested this branch against cockpit-project/cockpit-files#543 and it all passed.
| b.key_press(chr(37), use_ord=True) | ||
| b.key_press(chr(37), use_ord=True) | ||
| b.key_press(chr(37), use_ord=True) | ||
| b.key("ArrowLeft", 5) |
There was a problem hiding this comment.
kinda wondering if we want to be more explicity here repeat=5
This already helps to clean up some redundant code in TestHostSwitching and TestMenu, and will help to convert key_press() calls to key().
That makes the code easier to read/understand, and paves the way for deprecating key_press(). Add a `modifiers` kwarg to it too, as we need that for at least TestTerminal. Unfortunately Chromium is a bit buggy and doesn't understand some `key=` names like "Backspace" or "ArrowDown" in some (but not all!) circumstances/tests. It works fine for most other, like "Tab", or (really curiously) "ArrowLeft". Add a workaround by translating these to `windowsVirtualKeyCode`, as `key_press()` does.
`input_text()` is now solely for plain characters, no control ones or modifiers. That keeps the implementation very simple and browser independent. Fixes cockpit-project#20344
|
@jelly Thanks for the review! I already sent a draft PR for files to use this (verified locally), and added docstrings. |
Fixes #20344