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

Add an integration test for VNC + websockify #93

Closed
wants to merge 23 commits into from
Closed

Add an integration test for VNC + websockify #93

wants to merge 23 commits into from

Conversation

yuvipanda
Copy link
Contributor

This tests the following components:

  1. The VNC server
  2. websockify
  3. Jupyter Server Proxy
  4. The desktop environment itself

It does so by:

  1. Building the container image
  2. Starting a container with that image, with jupyter server running
    inside.
  3. Connecting to the container via websockets, authenticating and
    exposing the websocket connection as a regular TCP socket on
    localhost via websocat
  4. Taking a screenshot of the remote desktop by connecting to it
    through vncdotool. This requires a tcp connection, hence the
    double proxying with websocat
  5. Verify that the image produced by the screenshot is the same as a
    reference image we have.

This only misses the noVNC + JS from testing, but otherwise represents
a massive improvement to the status quo!

Copy link

github-actions bot commented Feb 15, 2024

Binder 👈 Launch a binder notebook on this branch for commit 7436a5b

I will automatically update this comment whenever this PR is modified

Binder 👈 Launch a binder notebook on this branch for commit 942b910

Binder 👈 Launch a binder notebook on this branch for commit a33a447

Binder 👈 Launch a binder notebook on this branch for commit 3a2b964

Binder 👈 Launch a binder notebook on this branch for commit 8ea28eb

Binder 👈 Launch a binder notebook on this branch for commit c285c0b

Binder 👈 Launch a binder notebook on this branch for commit 7a59e95

sleep 10
curl 'http://localhost:8888/desktop/?token=secret' | grep 'Jupyter Remote Desktop Proxy'
# Test if the built JS file is present in the image
curl 'http://localhost:8888/desktop/dist/viewer.js?token=secret' > /dev/null
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These curl tests have been converted into a pytest as well, and in fact actually caught the fact that the second viewer.js test doesn't actually work (wrong URL in the test, URL has been corrected because pytest caught it.)

@yuvipanda
Copy link
Contributor Author

This works perfectly fine locally, but unfortunately something is causing it to hang when the VNC client is trying to connect to the server on GitHub actions. will have to debug next time i get a chance.

This tests the following components:

1. The VNC server
2. websockify
3. Jupyter Server Proxy
4. The desktop environment itself

It does so by:

1. Building the container image
2. Starting a container with that image, with jupyter server running
   inside.
3. Connecting to the container via websockets, authenticating and
   exposing the websocket connection as a regular TCP socket on
   localhost via [websocat](https://github.com/vi/websocat)
4. Taking a screenshot of the remote desktop by connecting to it
   through `vncdotool`. This requires a tcp connection, hence the
   double proxying with websocat
5. Verify that the image produced by the screenshot is the same as a
   reference image we have.

This only misses the noVNC + JS from testing, but otherwise represents
a massive improvement to the status quo!
Prevents clashes between multiple runs

Also remove some old commented out code
Doesn't actually need to be interactive, we primarily care
about sharing stdout
Maybe this is what github actions needs
It did nothing

This reverts commit a33a447.
@yuvipanda
Copy link
Contributor Author

Not needed, now that we have #119

@yuvipanda yuvipanda closed this Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants