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.

Sign up for GitHub

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

Jump to bottom

Remove websockify, add Playwright test #119

Merged
merged 8 commits into from
Jul 8, 2024
Merged

Remove websockify, add Playwright test #119

merged 8 commits into from
Jul 8, 2024

Conversation

manics
Copy link
Member

@manics manics commented Jul 4, 2024 •
edited
Loading

jupyter-server-proxy 4.3.0 includes jupyterhub/jupyter-server-proxy#447 so we can remove the dependency on websockify

I've removed the old websockify test (closes #115) and added a Playwright test. Screenshots are uploaded as GitHub artifacts.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 4, 2024 •
edited
Loading

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

I will automatically update this comment whenever this PR is modified

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

Binder 👈 Launch a binder notebook on this branch for commit 13d2d49

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

Binder 👈 Launch a binder notebook on this branch for commit 12eb171

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

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

Binder 👈 Launch a binder notebook on this branch for commit 3952c14

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

Binder 👈 Launch a binder notebook on this branch for commit 857237c

Binder 👈 Launch a binder notebook on this branch for commit 70393e5

@manics manics force-pushed the raw_socket_proxy branch from c197f3d to 13d2d49 Compare July 5, 2024 20:22
@manics manics force-pushed the raw_socket_proxy branch from 13d2d49 to fddffc3 Compare July 5, 2024 20:28
@manics manics force-pushed the raw_socket_proxy branch 2 times, most recently from e5e4e5c to ccafba7 Compare July 5, 2024 20:52
@manics manics force-pushed the raw_socket_proxy branch from ccafba7 to 3952c14 Compare July 5, 2024 20:59
@manics manics changed the title WIP: Remove websockify Remove websockify, add Playwright test Jul 5, 2024
@manics manics marked this pull request as ready for review July 5, 2024 21:30
manics
manics commented Jul 5, 2024
View reviewed changes
Dockerfile Show resolved Hide resolved
package.json
@@ -1,7 +1,7 @@
{
"dependencies": {
"@floating-ui/dom": "^1.6.1",
"@novnc/novnc": "^1.4.0",
"@novnc/novnc": "~1.4.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

See #117

tests/test_browser.py
page1.wait_for_timeout(5000)
# Use a non temporary folder so we can check it manually if necessary
screenshot = Path("screenshots") / "desktop.png"
page1.locator("canvas").screenshot(path=screenshot)
Copy link
Member Author

Choose a reason for hiding this comment

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

This just screenshots the canvas element which contains only the desktop, and not the surrounding NoVNC elements to help minimise changes. We can change this to page1.screenshot(path=screenshot) to screenshot the whole page though.

tests/test_browser.py
JUPYTER_TOKEN = getenv("JUPYTER_TOKEN", "secret")


def compare_screenshot(test_image, threshold=2):
Copy link
Member Author

Choose a reason for hiding this comment

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

https://playwright.dev/docs/test-snapshots is not available in Python, this is just a very basic image diff.

@manics manics mentioned this pull request Jul 5, 2024
Closed
manics
manics commented Jul 8, 2024
View reviewed changes
tests/conftest.py Outdated Show resolved Hide resolved
@consideRatio
Copy link
Member

consideRatio commented Jul 8, 2024 •
edited
Loading

@manics this looks amazing! Do you expect this to be non-breaking besides needing jupyter-server-proxy 4.3+? (EDIT: oh we bump jupyter-server-proxy only from 4.1.1 !)

@consideRatio
Copy link
Member

consideRatio commented Jul 8, 2024 •
edited
Loading

Added maintenance label, uncertain what fits best - enhancement? Users no longer need to install websockify with this - a good thing.

EDIT: Added enhancement instead - this makes it fit to release as v2.1.0 instead of v2.0.2, which feels more appropriate.

@consideRatio consideRatio added enhancement New feature or request and removed maintenance labels Jul 8, 2024
@manics @consideRatio
Update Dockerfile
70393e5 
Co-authored-by: Erik Sundell <[email protected]>
@consideRatio consideRatio merged commit 7f16655 into jupyterhub:main Jul 8, 2024
7 checks passed
@manics manics deleted the raw_socket_proxy branch July 8, 2024 17:41
@yuvipanda yuvipanda mentioned this pull request Jul 19, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@consideRatio consideRatio consideRatio approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tests failing
2 participants
@manics @consideRatio