-
Notifications
You must be signed in to change notification settings - Fork 29
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
[WIP] Add Terminado tests #21
base: main
Are you sure you want to change the base?
Conversation
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.
Nice work on this @GeorgianaElena! I'm still wrapping my head around SSH, Terminado, etc but I understand that this test suite is slimmed enough to not include the complexities of JupyterHub - I think thats great!
I'd be happy to assist with review efforts etc to get this landed. I'm interested in trying to resolve #41, and having some additional tests to verify I'm not breaking things while doing so would be great!
@pytest.fixture | ||
async def notebook(): | ||
# Start the notebook | ||
notebook = NotebookTestBase() | ||
notebook.setup_class() | ||
notebook.wait_until_alive() | ||
yield notebook | ||
notebook.teardown_class() |
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.
I see that jupyter_server seem to have an equivalent. I'm not sure if its relevant to transition to that or not though.
Related issue: elyra-ai/elyra#831 (comment)
Related fixture from jupyter_server: https://github.com/jupyter-server/jupyter_server/blob/7d2f04f30e06e6efed90072df19824d416cb7129/jupyter_server/pytest_plugin.py#L185-L210
Thanks @consideRatio! I will refresh the info in this PR in my head and come back with more info on why/where I got blocked with it later today. Hopefully we can figure it out together! |
39c6cbd
to
20c4255
Compare
This PR:
Some things to consider:
URL
class fromyarl
seems to be escaping the urls if the flagencoded=True
is not provided.Because the
NotebookSSHServer
doesn't use this flag, when trying to connect tohttp://localhost:12341/a%40b/
, where the notebook runs, the url got escaped and got a 404.I wanted to open the PR early because I'm a bit stuck with testing writing to stdin (as suggested in #13), I don't seem to find the file I'm trying to create anywhere 😕
Fixes #13
Fixes 2i2c-org/upstream#27