Skip to content

Fix tests against jupyter_server 2 #366

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
Merged
merged 6 commits into from
Jan 3, 2023
Merged

Fix tests against jupyter_server 2 #366

merged 6 commits into from
Jan 3, 2023

Conversation

bollwyvl
Copy link
Collaborator

@bollwyvl bollwyvl commented Jan 2, 2023 •
edited
Loading

References

Changes

  • adds some explicit detection of jupyter_server version info
  • sets a bottom pin for robotframework-jupyterlibrary

Notes

  • i don't really know what's going on with the jupyter-notebook CLI command with jupyter_server@2, but jupyter-server and jupyter-lab seem to work

This was referenced Jan 2, 2023
Closed
Closed
@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Jan 2, 2023

I'd probably just skip the browser_check, not really sure what value it provides over the actual browser-based acceptance tests.

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Jan 2, 2023

Ok, skipping browser_check on py311/lab2 comes up clean without changing the package name.

@bollwyvl bollwyvl marked this pull request as ready for review January 2, 2023 18:59
bollwyvl
bollwyvl commented Jan 2, 2023
View reviewed changes
.github/workflows/test.yaml
@@ -111,7 +112,7 @@ jobs:
- name: Install Acceptance test dependencies
run: |
# the acceptance test requires notebook to run
pip install "notebook<7" robotframework-jupyterlibrary
pip install "notebook<7" "robotframework-jupyterlibrary>=0.4.2"
Copy link
Collaborator Author

@bollwyvl bollwyvl Jan 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

alternately we could hoist notebook to the [acceptance] extra and install that

manics
manics approved these changes Jan 2, 2023
View reviewed changes
Copy link
Member

@manics manics left a comment

Choose a reason for hiding this comment

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

Seems fine to me! Merge if you're happy

@manics
Copy link
Member

manics commented Jan 2, 2023 •
edited
Loading

Oh hang on, the https://github.com/jupyterhub/jupyter-server-proxy/blob/main/.github/workflows/test.yaml workflow isn't running
image

@consideRatio consideRatio reopened this Jan 2, 2023
@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Jan 2, 2023 via email

@consideRatio
Copy link
Member

Thank you soo much for your work on this @bollwyvl!!!! ❤️ 🎉!

@manics this was the issue it seems. I'll quit working for today I think but I'm happy to help resolve this later!

image

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Jan 2, 2023 via email

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Jan 3, 2023

The web editor liked the${{}} form better (as there were already quotes, and it doesn't seem to like " inside expressions.

consideRatio
consideRatio approved these changes Jan 3, 2023
View reviewed changes
Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

Added comments and renamed a constant for readability, but this LGTM!

@consideRatio consideRatio changed the title Handle Jupyter Server 2 Fix tests against jupyter_server 2 Jan 3, 2023
@consideRatio consideRatio added the ci label Jan 3, 2023
@consideRatio consideRatio force-pushed the gh-362-explicit-jp-server-2-robot branch from 53fa0d4 to 292ea2d Compare January 3, 2023 06:55
@consideRatio consideRatio force-pushed the gh-362-explicit-jp-server-2-robot branch from 8999d8d to 9484cc7 Compare January 3, 2023 07:04
@consideRatio
Copy link
Member

I updated the title to clarify that this PR is only adjusting the tests, not the software itself.

I also pushed two commits with:

  • some inline comments
  • a variable name update (SERVER_INFO -> JUPYTER_SERVER_INFO)
  • fix of an existing reference to matrix.python to matrix.python-version influencing uploaded artifact names

THANK YOU @bollwyvl for your work on this!!! ❤️ 🎉 🌻

@consideRatio consideRatio merged commit ab5ffa6 into jupyterhub:main Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manics manics manics approved these changes

@consideRatio consideRatio consideRatio approved these changes

Assignees
No one assigned
Labels
ci
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tests broken for jupyter_server 2 - help wanted!
3 participants
@bollwyvl @manics @consideRatio