-
Notifications
You must be signed in to change notification settings - Fork 74
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
Re-add frame-src CSP to allow downloading the desktop viewer file #1801
Re-add frame-src CSP to allow downloading the desktop viewer file #1801
Conversation
@@ -44,13 +56,16 @@ class TestMachinesConsoles(machineslib.VirtualMachinesCase): | |||
b.wait_in_text(".pf-v5-c-console__manual-connection dl > div:nth-child(2) dd", "5900") | |||
|
|||
b.click(".pf-v5-c-console__remote-viewer-launch-vv") # "Launch Remote Viewer" button | |||
b.wait_visible("#dynamically-generated-file") # is .vv file generated for download? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For context, this just checked that there is an HTML element which the file-saver
library creates for downloading. As we now wait on the download this is not needed anymore and an implementation detail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually not the file-saver
library but our own code...
As the integration test is the main part of this PR and it fails, I'll defer review. |
No problem, it worked locally so I'll need to debug CI maybe headless testing get's in the way. |
a446683
to
96a448b
Compare
|
Wild guess: That looks like it might be a scrollbar gutter? 🤨 Odd. |
Ugh, Firefox downloads it as a randm file, while chromium as "download".
|
e206a98
to
123205f
Compare
This was removed 0dcf864 under the impression it wasn't needed as all integration tests but the tests never validated if the desktop viewer file could be downloaded. Additionally add an integration test which verifies that the file can be downloaded successfully. Hide the iframe to avoid showing a temporary scrollbar which flakes the pixel test. Closes: cockpit-project#1799
123205f
to
319ab81
Compare
else: | ||
b.wait_visible("#dynamically-generated-file") # is .vv file generated for download? | ||
self.assertEqual(b.attr("#dynamically-generated-file", "href"), | ||
u"data:application/x-virt-viewer,%5Bvirt-viewer%5D%0Atype%3Dspice%0Ahost%3D127.0.0.1%0Aport%3D5900%0Adelete-this-file%3D1%0Afullscreen%3D0%0A") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of interest, why doesn't this include the funny [........GraphicsConsole]
part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This was removed 0dcf864 under the impression it wasn't needed as all integration tests but the tests never validated if the desktop viewer file could be downloaded.
Additionally add an integration test which verifies that the file can be downloaded successfully.
Closes: #1799